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 bfcde049c1..4cb102873e 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 @@ public function save($pFilename): void { if ($this->spreadSheet !== null) { // garbage collect + $this->pathNames = []; $this->spreadSheet->garbageCollect(); $this->openFileHandle($pFilename); @@ -203,73 +211,73 @@ public function save($pFilename): void $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 @@ public function save($pFilename): void // 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 @@ public function save($pFilename): void // 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 @@ public function save($pFilename): void $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 @@ public function save($pFilename): void // 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 @@ public function save($pFilename): void $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 @@ public function save($pFilename): void $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 @@ public function save($pFilename): void // Close file try { - $zip->finish(); + $this->zip->finish(); } catch (OverflowException $e) { throw new WriterException('Could not close resource.'); } @@ -535,4 +543,14 @@ public function setOffice2003Compatibility($pValue) 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 0000000000..d2afb4239c --- /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()); + } +}