Throw exception for invalid range to prevent infinite loop

Fixes PHPOffice#519 Closes PHPOffice#521
Dfred · Oct 29, 2018 · e248cb1 · e248cb1
1 parent b61d3fc
commit e248cb1
Show file tree

Hide file tree

Showing 3 changed files with 119 additions and 40 deletions.
diff --git a/CHANGELOG.md b/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
 

diff --git a/src/PhpSpreadsheet/Cell/Coordinate.php b/src/PhpSpreadsheet/Cell/Coordinate.php
@@ -327,57 +327,20 @@ public static function stringFromColumnIndex($columnIndex)
     }
 
     /**
-     * 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)
      *
      * @return array Array containing single cell references
      */
     public static function extractAllCellReferencesInRange($pRange)
     {
-        // Returnvalue
         $returnValue = [];
 
         // Explode spaces
-        $cellBlocks = explode(' ', str_replace('$', '', strtoupper($pRange)));
+        $cellBlocks = self::getCellBlocksFromRangeString($pRange);
         foreach ($cellBlocks as $cellBlock) {
-            // Single cell?
-            if (!self::coordinateIsRange($cellBlock)) {
-                $returnValue[] = $cellBlock;
-
-                continue;
-            }
-
-            // Range...
-            $ranges = self::splitRange($cellBlock);
-            foreach ($ranges as $range) {
-                // Single cell?
-                if (!isset($range[1])) {
-                    $returnValue[] = $range[0];
-
-                    continue;
-                }
-
-                // Range...
-                list($rangeStart, $rangeEnd) = $range;
-                sscanf($rangeStart, '%[A-Z]%d', $startCol, $startRow);
-                sscanf($rangeEnd, '%[A-Z]%d', $endCol, $endRow);
-                ++$endCol;
-
-                // Current data
-                $currentCol = $startCol;
-                $currentRow = $startRow;
-
-                // Loop cells
-                while ($currentCol != $endCol) {
-                    while ($currentRow <= $endRow) {
-                        $returnValue[] = $currentCol . $currentRow;
-                        ++$currentRow;
-                    }
-                    ++$currentCol;
-                    $currentRow = $startRow;
-                }
-            }
+            $returnValue = array_merge($returnValue, static::getReferencesForCellBlock($cellBlock));
         }
 
         //    Sort the result by column and row
@@ -392,6 +355,72 @@ public static function extractAllCellReferencesInRange($pRange)
         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)) {
+            return (array) $cellBlock;
+        }
+
+        // Range...
+        $ranges = self::splitRange($cellBlock);
+        foreach ($ranges as $range) {
+            // Single cell?
+            if (!isset($range[1])) {
+                $returnValue[] = $range[0];
+
+                continue;
+            }
+
+            // Range...
+            list($rangeStart, $rangeEnd) = $range;
+            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 ($currentRow <= $endRow) {
+                    $returnValue[] = $currentCol . $currentRow;
+                    ++$currentRow;
+                }
+                ++$currentCol;
+                $currentRow = $startRow;
+            }
+        }
+
+        return $returnValue;
+    }
+
+    /**
+     * 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');
+    }
+
     /**
      * Convert an associative array of single cell coordinates to values to an associative array
      * of cell ranges to values.  Only adjacent cell coordinates with the same
@@ -477,4 +506,35 @@ public static function mergeRangesInCollection(array $pCoordCollection)
 
         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 . '"');
+        }
+    }
 }
diff --git a/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php b/tests/PhpSpreadsheetTests/Cell/CoordinateTest.php
@@ -315,6 +315,24 @@ public function providerExtractAllCellReferencesInRange()
         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
      *