From 82ea1d559647f357b5ce04e60a0bb33ccaa6de77 Mon Sep 17 00:00:00 2001 From: oleibman Date: Fri, 19 Jun 2020 11:26:02 -0700 Subject: [PATCH] Fix for #1516 (#1530) This problem is that ZipStream, in contrast to ZipArchive, is saving 2 files with the same path. I have opened an issue with ZipStream, who agree that this appears to be a bug. For the case in question, PhpSpreadsheet is attempting to save a file with the same path twice (and unexpectedly succeeding) because of a clone operation. This fix attempts to rectify the problem by keeping track of all the paths being saved in the zip file, and not attempting to save any duplicate paths. The problem case attempted to save printersettings1.bin twice, but there are other possible exposures, e.g. by cloning a sheet with a drawing.The new test cases clone an existing sample which has both printer settings and drawings. --- src/PhpSpreadsheet/Writer/Xlsx.php | 88 +++++++++------ .../Writer/Xlsx/UnparsedDataCloneTest.php | 101 ++++++++++++++++++ 2 files changed, 154 insertions(+), 35 deletions(-) create mode 100644 tests/PhpSpreadsheetTests/Writer/Xlsx/UnparsedDataCloneTest.php diff --git a/src/PhpSpreadsheet/Writer/Xlsx.php b/src/PhpSpreadsheet/Writer/Xlsx.php index bfcde049..4cb10287 100644 --- a/src/PhpSpreadsheet/Writer/Xlsx.php +++ b/src/PhpSpreadsheet/Writer/Xlsx.php @@ -107,6 +107,13 @@ class Xlsx extends BaseWriter */ private $drawingHashTable; + /** + * Private handle for zip stream. + * + * @var ZipStream + */ + private $zip; + /** * Create a new Xlsx Writer. */ @@ -173,6 +180,7 @@ class Xlsx extends BaseWriter { if ($this->spreadSheet !== null) { // garbage collect + $this->pathNames = []; $this->spreadSheet->garbageCollect(); $this->openFileHandle($pFilename); @@ -203,73 +211,73 @@ class Xlsx extends BaseWriter $options->setEnableZip64(false); $options->setOutputStream($this->fileHandle); - $zip = new ZipStream(null, $options); + $this->zip = new ZipStream(null, $options); // Add [Content_Types].xml to ZIP file - $zip->addFile('[Content_Types].xml', $this->getWriterPart('ContentTypes')->writeContentTypes($this->spreadSheet, $this->includeCharts)); + $this->addZipFile('[Content_Types].xml', $this->getWriterPart('ContentTypes')->writeContentTypes($this->spreadSheet, $this->includeCharts)); //if hasMacros, add the vbaProject.bin file, Certificate file(if exists) if ($this->spreadSheet->hasMacros()) { $macrosCode = $this->spreadSheet->getMacrosCode(); if ($macrosCode !== null) { // we have the code ? - $zip->addFile('xl/vbaProject.bin', $macrosCode); //allways in 'xl', allways named vbaProject.bin + $this->addZipFile('xl/vbaProject.bin', $macrosCode); //allways in 'xl', allways named vbaProject.bin if ($this->spreadSheet->hasMacrosCertificate()) { //signed macros ? // Yes : add the certificate file and the related rels file - $zip->addFile('xl/vbaProjectSignature.bin', $this->spreadSheet->getMacrosCertificate()); - $zip->addFile('xl/_rels/vbaProject.bin.rels', $this->getWriterPart('RelsVBA')->writeVBARelationships($this->spreadSheet)); + $this->addZipFile('xl/vbaProjectSignature.bin', $this->spreadSheet->getMacrosCertificate()); + $this->addZipFile('xl/_rels/vbaProject.bin.rels', $this->getWriterPart('RelsVBA')->writeVBARelationships($this->spreadSheet)); } } } //a custom UI in this workbook ? add it ("base" xml and additional objects (pictures) and rels) if ($this->spreadSheet->hasRibbon()) { $tmpRibbonTarget = $this->spreadSheet->getRibbonXMLData('target'); - $zip->addFile($tmpRibbonTarget, $this->spreadSheet->getRibbonXMLData('data')); + $this->addZipFile($tmpRibbonTarget, $this->spreadSheet->getRibbonXMLData('data')); if ($this->spreadSheet->hasRibbonBinObjects()) { $tmpRootPath = dirname($tmpRibbonTarget) . '/'; $ribbonBinObjects = $this->spreadSheet->getRibbonBinObjects('data'); //the files to write foreach ($ribbonBinObjects as $aPath => $aContent) { - $zip->addFile($tmpRootPath . $aPath, $aContent); + $this->addZipFile($tmpRootPath . $aPath, $aContent); } //the rels for files - $zip->addFile($tmpRootPath . '_rels/' . basename($tmpRibbonTarget) . '.rels', $this->getWriterPart('RelsRibbonObjects')->writeRibbonRelationships($this->spreadSheet)); + $this->addZipFile($tmpRootPath . '_rels/' . basename($tmpRibbonTarget) . '.rels', $this->getWriterPart('RelsRibbonObjects')->writeRibbonRelationships($this->spreadSheet)); } } // Add relationships to ZIP file - $zip->addFile('_rels/.rels', $this->getWriterPart('Rels')->writeRelationships($this->spreadSheet)); - $zip->addFile('xl/_rels/workbook.xml.rels', $this->getWriterPart('Rels')->writeWorkbookRelationships($this->spreadSheet)); + $this->addZipFile('_rels/.rels', $this->getWriterPart('Rels')->writeRelationships($this->spreadSheet)); + $this->addZipFile('xl/_rels/workbook.xml.rels', $this->getWriterPart('Rels')->writeWorkbookRelationships($this->spreadSheet)); // Add document properties to ZIP file - $zip->addFile('docProps/app.xml', $this->getWriterPart('DocProps')->writeDocPropsApp($this->spreadSheet)); - $zip->addFile('docProps/core.xml', $this->getWriterPart('DocProps')->writeDocPropsCore($this->spreadSheet)); + $this->addZipFile('docProps/app.xml', $this->getWriterPart('DocProps')->writeDocPropsApp($this->spreadSheet)); + $this->addZipFile('docProps/core.xml', $this->getWriterPart('DocProps')->writeDocPropsCore($this->spreadSheet)); $customPropertiesPart = $this->getWriterPart('DocProps')->writeDocPropsCustom($this->spreadSheet); if ($customPropertiesPart !== null) { - $zip->addFile('docProps/custom.xml', $customPropertiesPart); + $this->addZipFile('docProps/custom.xml', $customPropertiesPart); } // Add theme to ZIP file - $zip->addFile('xl/theme/theme1.xml', $this->getWriterPart('Theme')->writeTheme($this->spreadSheet)); + $this->addZipFile('xl/theme/theme1.xml', $this->getWriterPart('Theme')->writeTheme($this->spreadSheet)); // Add string table to ZIP file - $zip->addFile('xl/sharedStrings.xml', $this->getWriterPart('StringTable')->writeStringTable($this->stringTable)); + $this->addZipFile('xl/sharedStrings.xml', $this->getWriterPart('StringTable')->writeStringTable($this->stringTable)); // Add styles to ZIP file - $zip->addFile('xl/styles.xml', $this->getWriterPart('Style')->writeStyles($this->spreadSheet)); + $this->addZipFile('xl/styles.xml', $this->getWriterPart('Style')->writeStyles($this->spreadSheet)); // Add workbook to ZIP file - $zip->addFile('xl/workbook.xml', $this->getWriterPart('Workbook')->writeWorkbook($this->spreadSheet, $this->preCalculateFormulas)); + $this->addZipFile('xl/workbook.xml', $this->getWriterPart('Workbook')->writeWorkbook($this->spreadSheet, $this->preCalculateFormulas)); $chartCount = 0; // Add worksheets for ($i = 0; $i < $this->spreadSheet->getSheetCount(); ++$i) { - $zip->addFile('xl/worksheets/sheet' . ($i + 1) . '.xml', $this->getWriterPart('Worksheet')->writeWorksheet($this->spreadSheet->getSheet($i), $this->stringTable, $this->includeCharts)); + $this->addZipFile('xl/worksheets/sheet' . ($i + 1) . '.xml', $this->getWriterPart('Worksheet')->writeWorksheet($this->spreadSheet->getSheet($i), $this->stringTable, $this->includeCharts)); if ($this->includeCharts) { $charts = $this->spreadSheet->getSheet($i)->getChartCollection(); if (count($charts) > 0) { foreach ($charts as $chart) { - $zip->addFile('xl/charts/chart' . ($chartCount + 1) . '.xml', $this->getWriterPart('Chart')->writeChart($chart, $this->preCalculateFormulas)); + $this->addZipFile('xl/charts/chart' . ($chartCount + 1) . '.xml', $this->getWriterPart('Chart')->writeChart($chart, $this->preCalculateFormulas)); ++$chartCount; } } @@ -280,19 +288,19 @@ class Xlsx extends BaseWriter // Add worksheet relationships (drawings, ...) for ($i = 0; $i < $this->spreadSheet->getSheetCount(); ++$i) { // Add relationships - $zip->addFile('xl/worksheets/_rels/sheet' . ($i + 1) . '.xml.rels', $this->getWriterPart('Rels')->writeWorksheetRelationships($this->spreadSheet->getSheet($i), ($i + 1), $this->includeCharts)); + $this->addZipFile('xl/worksheets/_rels/sheet' . ($i + 1) . '.xml.rels', $this->getWriterPart('Rels')->writeWorksheetRelationships($this->spreadSheet->getSheet($i), ($i + 1), $this->includeCharts)); // Add unparsedLoadedData $sheetCodeName = $this->spreadSheet->getSheet($i)->getCodeName(); $unparsedLoadedData = $this->spreadSheet->getUnparsedLoadedData(); if (isset($unparsedLoadedData['sheets'][$sheetCodeName]['ctrlProps'])) { foreach ($unparsedLoadedData['sheets'][$sheetCodeName]['ctrlProps'] as $ctrlProp) { - $zip->addFile($ctrlProp['filePath'], $ctrlProp['content']); + $this->addZipFile($ctrlProp['filePath'], $ctrlProp['content']); } } if (isset($unparsedLoadedData['sheets'][$sheetCodeName]['printerSettings'])) { foreach ($unparsedLoadedData['sheets'][$sheetCodeName]['printerSettings'] as $ctrlProp) { - $zip->addFile($ctrlProp['filePath'], $ctrlProp['content']); + $this->addZipFile($ctrlProp['filePath'], $ctrlProp['content']); } } @@ -305,13 +313,13 @@ class Xlsx extends BaseWriter // Add drawing and image relationship parts if (($drawingCount > 0) || ($chartCount > 0)) { // Drawing relationships - $zip->addFile('xl/drawings/_rels/drawing' . ($i + 1) . '.xml.rels', $this->getWriterPart('Rels')->writeDrawingRelationships($this->spreadSheet->getSheet($i), $chartRef1, $this->includeCharts)); + $this->addZipFile('xl/drawings/_rels/drawing' . ($i + 1) . '.xml.rels', $this->getWriterPart('Rels')->writeDrawingRelationships($this->spreadSheet->getSheet($i), $chartRef1, $this->includeCharts)); // Drawings - $zip->addFile('xl/drawings/drawing' . ($i + 1) . '.xml', $this->getWriterPart('Drawing')->writeDrawings($this->spreadSheet->getSheet($i), $this->includeCharts)); + $this->addZipFile('xl/drawings/drawing' . ($i + 1) . '.xml', $this->getWriterPart('Drawing')->writeDrawings($this->spreadSheet->getSheet($i), $this->includeCharts)); } elseif (isset($unparsedLoadedData['sheets'][$sheetCodeName]['drawingAlternateContents'])) { // Drawings - $zip->addFile('xl/drawings/drawing' . ($i + 1) . '.xml', $this->getWriterPart('Drawing')->writeDrawings($this->spreadSheet->getSheet($i), $this->includeCharts)); + $this->addZipFile('xl/drawings/drawing' . ($i + 1) . '.xml', $this->getWriterPart('Drawing')->writeDrawings($this->spreadSheet->getSheet($i), $this->includeCharts)); } // Add unparsed drawings @@ -320,7 +328,7 @@ class Xlsx extends BaseWriter $drawingFile = array_search($relId, $unparsedLoadedData['sheets'][$sheetCodeName]['drawingOriginalIds']); if ($drawingFile !== false) { $drawingFile = ltrim($drawingFile, '.'); - $zip->addFile('xl' . $drawingFile, $drawingXml); + $this->addZipFile('xl' . $drawingFile, $drawingXml); } } } @@ -328,30 +336,30 @@ class Xlsx extends BaseWriter // Add comment relationship parts if (count($this->spreadSheet->getSheet($i)->getComments()) > 0) { // VML Comments - $zip->addFile('xl/drawings/vmlDrawing' . ($i + 1) . '.vml', $this->getWriterPart('Comments')->writeVMLComments($this->spreadSheet->getSheet($i))); + $this->addZipFile('xl/drawings/vmlDrawing' . ($i + 1) . '.vml', $this->getWriterPart('Comments')->writeVMLComments($this->spreadSheet->getSheet($i))); // Comments - $zip->addFile('xl/comments' . ($i + 1) . '.xml', $this->getWriterPart('Comments')->writeComments($this->spreadSheet->getSheet($i))); + $this->addZipFile('xl/comments' . ($i + 1) . '.xml', $this->getWriterPart('Comments')->writeComments($this->spreadSheet->getSheet($i))); } // Add unparsed relationship parts if (isset($unparsedLoadedData['sheets'][$sheetCodeName]['vmlDrawings'])) { foreach ($unparsedLoadedData['sheets'][$sheetCodeName]['vmlDrawings'] as $vmlDrawing) { - $zip->addFile($vmlDrawing['filePath'], $vmlDrawing['content']); + $this->addZipFile($vmlDrawing['filePath'], $vmlDrawing['content']); } } // Add header/footer relationship parts if (count($this->spreadSheet->getSheet($i)->getHeaderFooter()->getImages()) > 0) { // VML Drawings - $zip->addFile('xl/drawings/vmlDrawingHF' . ($i + 1) . '.vml', $this->getWriterPart('Drawing')->writeVMLHeaderFooterImages($this->spreadSheet->getSheet($i))); + $this->addZipFile('xl/drawings/vmlDrawingHF' . ($i + 1) . '.vml', $this->getWriterPart('Drawing')->writeVMLHeaderFooterImages($this->spreadSheet->getSheet($i))); // VML Drawing relationships - $zip->addFile('xl/drawings/_rels/vmlDrawingHF' . ($i + 1) . '.vml.rels', $this->getWriterPart('Rels')->writeHeaderFooterDrawingRelationships($this->spreadSheet->getSheet($i))); + $this->addZipFile('xl/drawings/_rels/vmlDrawingHF' . ($i + 1) . '.vml.rels', $this->getWriterPart('Rels')->writeHeaderFooterDrawingRelationships($this->spreadSheet->getSheet($i))); // Media foreach ($this->spreadSheet->getSheet($i)->getHeaderFooter()->getImages() as $image) { - $zip->addFile('xl/media/' . $image->getIndexedFilename(), file_get_contents($image->getPath())); + $this->addZipFile('xl/media/' . $image->getIndexedFilename(), file_get_contents($image->getPath())); } } } @@ -374,7 +382,7 @@ class Xlsx extends BaseWriter $imageContents = file_get_contents($imagePath); } - $zip->addFile('xl/media/' . str_replace(' ', '_', $this->getDrawingHashTable()->getByIndex($i)->getIndexedFilename()), $imageContents); + $this->addZipFile('xl/media/' . str_replace(' ', '_', $this->getDrawingHashTable()->getByIndex($i)->getIndexedFilename()), $imageContents); } elseif ($this->getDrawingHashTable()->getByIndex($i) instanceof MemoryDrawing) { ob_start(); call_user_func( @@ -384,7 +392,7 @@ class Xlsx extends BaseWriter $imageContents = ob_get_contents(); ob_end_clean(); - $zip->addFile('xl/media/' . str_replace(' ', '_', $this->getDrawingHashTable()->getByIndex($i)->getIndexedFilename()), $imageContents); + $this->addZipFile('xl/media/' . str_replace(' ', '_', $this->getDrawingHashTable()->getByIndex($i)->getIndexedFilename()), $imageContents); } } @@ -393,7 +401,7 @@ class Xlsx extends BaseWriter // Close file try { - $zip->finish(); + $this->zip->finish(); } catch (OverflowException $e) { throw new WriterException('Could not close resource.'); } @@ -535,4 +543,14 @@ class Xlsx extends BaseWriter return $this; } + + private $pathNames = []; + + private function addZipFile(string $path, string $content): void + { + if (!in_array($path, $this->pathNames)) { + $this->pathNames[] = $path; + $this->zip->addFile($path, $content); + } + } } diff --git a/tests/PhpSpreadsheetTests/Writer/Xlsx/UnparsedDataCloneTest.php b/tests/PhpSpreadsheetTests/Writer/Xlsx/UnparsedDataCloneTest.php new file mode 100644 index 00000000..d2afb423 --- /dev/null +++ b/tests/PhpSpreadsheetTests/Writer/Xlsx/UnparsedDataCloneTest.php @@ -0,0 +1,101 @@ +load($sampleFilename); + $spreadsheet->setActiveSheetIndex(1); + $sheet = $spreadsheet->getActiveSheet(); + $drawings = $sheet->getDrawingCollection(); + self::assertCount(1, $drawings); + $sheetCodeName = $sheet->getCodeName(); + $unparsedLoadedData = $spreadsheet->getUnparsedLoadedData(); + self::assertArrayHasKey('printerSettings', $unparsedLoadedData['sheets'][$sheetCodeName]); + self::assertCount(1, $unparsedLoadedData['sheets'][$sheetCodeName]['printerSettings']); + + $clonedSheet = clone $spreadsheet->getActiveSheet(); + $clonedSheet->setTitle('Clone'); + $spreadsheet->addSheet($clonedSheet); + + $writer = new \PhpOffice\PhpSpreadsheet\Writer\Xlsx($spreadsheet); + $writer->save($resultFilename); + $dupname = 'Unable to open saved file'; + $zip = zip_open($resultFilename); + if (is_resource($zip)) { + $names = []; + $dupname = ''; + while ($zip_entry = zip_read($zip)) { + $zipname = zip_entry_name($zip_entry); + if (in_array($zipname, $names)) { + $dupname .= "$zipname,"; + } else { + $names[] = $zipname; + } + } + zip_close($zip); + } + unlink($resultFilename); + self::assertEquals('', $dupname); + } + + /** + * Test that saving twice with same writer works. + */ + public function testSaveTwice(): void + { + $sampleFilename = 'tests/data/Writer/XLSX/drawing_on_2nd_page.xlsx'; + $resultFilename1 = tempnam(File::sysGetTempDir(), 'phpspreadsheet-test1'); + $resultFilename2 = tempnam(File::sysGetTempDir(), 'phpspreadsheet-test2'); + self::assertNotEquals($resultFilename1, $resultFilename2); + Settings::setLibXmlLoaderOptions(null); // reset to default options + $reader = new \PhpOffice\PhpSpreadsheet\Reader\Xlsx(); + $spreadsheet = $reader->load($sampleFilename); + $sheet = $spreadsheet->setActiveSheetIndex(1); + $sheet->setTitle('Original'); + + $clonedSheet = clone $spreadsheet->getActiveSheet(); + $clonedSheet->setTitle('Clone'); + $spreadsheet->addSheet($clonedSheet); + $clonedSheet->getCell('A8')->setValue('cloned'); + $sheet->getCell('A8')->setValue('original'); + + $writer = new \PhpOffice\PhpSpreadsheet\Writer\Xlsx($spreadsheet); + $writer->save($resultFilename1); + $reader1 = new \PhpOffice\PhpSpreadsheet\Reader\Xlsx(); + $spreadsheet1 = $reader1->load($resultFilename1); + unlink($resultFilename1); + $sheet1c = $spreadsheet1->getSheetByName('Clone'); + $sheet1o = $spreadsheet1->getSheetByName('Original'); + + $writer->save($resultFilename2); + $reader2 = new \PhpOffice\PhpSpreadsheet\Reader\Xlsx(); + $spreadsheet2 = $reader2->load($resultFilename2); + unlink($resultFilename2); + $sheet2c = $spreadsheet2->getSheetByName('Clone'); + $sheet2o = $spreadsheet2->getSheetByName('Original'); + + self::assertEquals($spreadsheet1->getSheetCount(), $spreadsheet2->getSheetCount()); + self::assertCount(1, $sheet1c->getDrawingCollection()); + self::assertCount(1, $sheet1o->getDrawingCollection()); + self::assertCount(1, $sheet2c->getDrawingCollection()); + self::assertCount(1, $sheet2o->getDrawingCollection()); + self::assertEquals('original', $sheet1o->getCell('A8')->getValue()); + self::assertEquals('original', $sheet2o->getCell('A8')->getValue()); + self::assertEquals('cloned', $sheet1c->getCell('A8')->getValue()); + self::assertEquals('cloned', $sheet2c->getCell('A8')->getValue()); + } +}