Throw exception for invalid range to prevent infinite loop

Fixes #519
Closes #521

CHANGELOG.md

@@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Xlsx loaded an extra empty comment for each real comment - [#375](https://github.com/PHPOffice/PhpSpreadsheet/issues/375)
 - Xlsx reader do not read rows and columns filtered out in readFilter at all - [#370](https://github.com/PHPOffice/PhpSpreadsheet/issues/370)
 - Make newer Excel versions properly recalculate formulas on document open - [#456](https://github.com/PHPOffice/PhpSpreadsheet/issues/456)
+- `Coordinate::extractAllCellReferencesInRange()` throws an exception for an invalid range – [#519](https://github.com/PHPOffice/PhpSpreadsheet/issues/519)
 
 ## [1.2.1] - 2018-04-10
 

src/PhpSpreadsheet/Cell/Coordinate.php

@@ -327,7 +327,7 @@ abstract class Coordinate
     }
 
     /**
-     * Extract all cell references in range.
+     * Extract all cell references in range, which may be comprised of multiple cell ranges.
      *
      * @param string $pRange Range (e.g. A1 or A1:C10 or A1:E10 A20:E25)
      *
@@ -335,17 +335,42 @@ abstract class Coordinate
      */
     public static function extractAllCellReferencesInRange($pRange)
     {
-        // Returnvalue
         $returnValue = [];
 
         // Explode spaces
-        $cellBlocks = explode(' ', str_replace('$', '', strtoupper($pRange)));
+        $cellBlocks = self::getCellBlocksFromRangeString($pRange);
         foreach ($cellBlocks as $cellBlock) {
+            $returnValue = array_merge($returnValue, static::getReferencesForCellBlock($cellBlock));
+        }
+
+        //    Sort the result by column and row
+        $sortKeys = [];
+        foreach (array_unique($returnValue) as $coord) {
+            sscanf($coord, '%[A-Z]%d', $column, $row);
+            $sortKeys[sprintf('%3s%09d', $column, $row)] = $coord;
+        }
+        ksort($sortKeys);
+
+        // Return value
+        return array_values($sortKeys);
+    }
+
+    /**
+     * Get all cell references for an individual cell block.
+     *
+     * @param string $cellBlock A cell range e.g. A4:B5
+     *
+     * @throws Exception
+     *
+     * @return array All individual cells in that range
+     */
+    private static function getReferencesForCellBlock($cellBlock)
+    {
+        $returnValue = [];
+
         // Single cell?
         if (!self::coordinateIsRange($cellBlock)) {
-                $returnValue[] = $cellBlock;
-
-                continue;
+            return (array) $cellBlock;
         }
 
         // Range...
@@ -360,16 +385,18 @@ abstract class Coordinate
 
             // Range...
             list($rangeStart, $rangeEnd) = $range;
-                sscanf($rangeStart, '%[A-Z]%d', $startCol, $startRow);
-                sscanf($rangeEnd, '%[A-Z]%d', $endCol, $endRow);
+            list($startCol, $startRow) = static::extractColumnAndRow($rangeStart);
+            list($endCol, $endRow) = static::extractColumnAndRow($rangeEnd);
             ++$endCol;
 
             // Current data
             $currentCol = $startCol;
             $currentRow = $startRow;
 
+            static::validateRange($cellBlock, $startCol, $endCol, $currentRow, $endRow);
+
             // Loop cells
-                while ($currentCol != $endCol) {
+            while ($currentCol < $endCol) {
                 while ($currentRow <= $endRow) {
                     $returnValue[] = $currentCol . $currentRow;
                     ++$currentRow;
@@ -378,18 +405,20 @@ abstract class Coordinate
                 $currentRow = $startRow;
             }
         }
+
+        return $returnValue;
     }
 
-        //    Sort the result by column and row
-        $sortKeys = [];
-        foreach (array_unique($returnValue) as $coord) {
-            sscanf($coord, '%[A-Z]%d', $column, $row);
-            $sortKeys[sprintf('%3s%09d', $column, $row)] = $coord;
-        }
-        ksort($sortKeys);
-
-        // Return value
-        return array_values($sortKeys);
+    /**
+     * Extract the column and row from a cell reference in the format [$column, $row].
+     *
+     * @param string $cell
+     *
+     * @return array
+     */
+    private static function extractColumnAndRow($cell)
+    {
+        return sscanf($cell, '%[A-Z]%d');
     }
 
     /**
@@ -477,4 +506,35 @@ abstract class Coordinate
 
         return $mergedCoordCollection;
     }
+
+    /**
+     * Get the individual cell blocks from a range string, splitting by space and removing any $ characters.
+     *
+     * @param string $pRange
+     *
+     * @return string[]
+     */
+    private static function getCellBlocksFromRangeString($pRange)
+    {
+        return explode(' ', str_replace('$', '', strtoupper($pRange)));
+    }
+
+    /**
+     * Check that the given range is valid, i.e. that the start column and row are not greater than the end column and
+     * row.
+     *
+     * @param string $cellBlock The original range, for displaying a meaningful error message
+     * @param string $startCol
+     * @param string $endCol
+     * @param int $currentRow
+     * @param int $endRow
+     *
+     * @throws Exception
+     */
+    private static function validateRange($cellBlock, $startCol, $endCol, $currentRow, $endRow)
+    {
+        if ($startCol >= $endCol || $currentRow > $endRow) {
+            throw new Exception('Invalid range: "' . $cellBlock . '"');
+        }
+    }
 }

tests/PhpSpreadsheetTests/Cell/CoordinateTest.php

@@ -315,6 +315,24 @@ class CoordinateTest extends TestCase
         return require 'data/CellExtractAllCellReferencesInRange.php';
     }
 
+    /**
+     * @dataProvider providerInvalidRange
+     *
+     * @param string $range
+     */
+    public function testExtractAllCellReferencesInRangeInvalidRange($range)
+    {
+        $this->expectException(Exception::class);
+        $this->expectExceptionMessage('Invalid range: "' . $range . '"');
+
+        Coordinate::extractAllCellReferencesInRange($range);
+    }
+
+    public function providerInvalidRange()
+    {
+        return [['Z1:A1'], ['A4:A1'], ['B1:A1'], ['AA1:Z1']];
+    }
+
     /**
      * @dataProvider providerMergeRangesInCollection
      *