Fix for bug #1592 (UPDATED) (#1623)

* Fix for Xls when BIFF8 SST (FCh) has bad Shared string length

src/PhpSpreadsheet/Reader/Xls.php

@@ -2973,6 +2973,9 @@ class Xls extends BaseReader
         // offset within (spliced) record data
         $pos = 0;
 
+        // Limit global SST position, further control for bad SST Length in BIFF8 data
+        $limitposSST = 0;
+
         // get spliced record data
         $splicedRecordData = $this->getSplicedRecordData();
 
@@ -2986,8 +2989,17 @@ class Xls extends BaseReader
         $nm = self::getInt4d($recordData, 4);
         $pos += 4;
 
+        // look up limit position
+        foreach ($spliceOffsets as $spliceOffset) {
+            // it can happen that the string is empty, therefore we need
+            // <= and not just <
+            if ($pos <= $spliceOffset) {
+                $limitposSST = $spliceOffset;
+            }
+        }
+
         // loop through the Unicode strings (16-bit length)
-        for ($i = 0; $i < $nm; ++$i) {
+        for ($i = 0; $i < $nm && $pos < $limitposSST; ++$i) {
             // number of characters in the Unicode string
             $numChars = self::getUInt2d($recordData, $pos);
             $pos += 2;
@@ -3020,7 +3032,7 @@ class Xls extends BaseReader
             // expected byte length of character array if not split
             $len = ($isCompressed) ? $numChars : $numChars * 2;
 
-            // look up limit position
+            // look up limit position - Check it again to be sure that no error occurs when parsing SST structure
             foreach ($spliceOffsets as $spliceOffset) {
                 // it can happen that the string is empty, therefore we need
                 // <= and not just <

tests/PhpSpreadsheetTests/Reader/XlsTest.php

@@ -43,4 +43,39 @@ class XlsTest extends AbstractFunctional
         self::assertEquals($sheet->getCell('A1')->getFormattedValue(), $newsheet->getCell('A1')->getFormattedValue());
         self::assertEquals($sheet->getCell("$col$row")->getFormattedValue(), $newsheet->getCell("$col$row")->getFormattedValue());
     }
+
+    /**
+     * Test load Xls file with invalid length in SST map.
+     */
+    public function testLoadXlsBug1592(): void
+    {
+        $filename = 'tests/data/Reader/XLS/bug1592.xls';
+        $reader = new Xls();
+        // When no fix applied, spreadsheet is not loaded
+        $spreadsheet = $reader->load($filename);
+        $sheet = $spreadsheet->getActiveSheet();
+        $col = $sheet->getHighestColumn();
+        $row = $sheet->getHighestRow();
+
+        $newspreadsheet = $this->writeAndReload($spreadsheet, 'Xlsx');
+        $newsheet = $newspreadsheet->getActiveSheet();
+        $newcol = $newsheet->getHighestColumn();
+        $newrow = $newsheet->getHighestRow();
+
+        self::assertEquals($spreadsheet->getSheetCount(), $newspreadsheet->getSheetCount());
+        self::assertEquals($sheet->getTitle(), $newsheet->getTitle());
+        self::assertEquals($sheet->getColumnDimensions(), $newsheet->getColumnDimensions());
+        self::assertEquals($col, $newcol);
+        self::assertEquals($row, $newrow);
+
+        $rowIterator = $sheet->getRowIterator();
+
+        foreach ($rowIterator as $row) {
+            foreach ($row->getCellIterator() as $cell) {
+                $valOld = $cell->getFormattedValue();
+                $valNew = $newsheet->getCell($cell->getCoordinate())->getFormattedValue();
+                self::assertEquals($valOld, $valNew);
+            }
+        }
+    }
 }