From 38fab4e6329df8ba857afa24fd686bc5369a116b Mon Sep 17 00:00:00 2001 From: oleibman Date: Fri, 19 Jun 2020 12:01:18 -0700 Subject: [PATCH] Fix for #1505 (#1525) This problem is the same as #1238, which was resolved by #1239. For that issue, the fix was to check in one place whether $this->mapCellXfIndex[$xfIndex] was set before using it. The sample spreadsheet supplied as a description for this problem had exactly the same problem in 2 other places in the code. In addition, there were 7 other places in the code where that particular item was used unchecked. This fix corrects all 9 locations. The spreadsheet supplied with the problem is used as the basis for some new tests, which particularly test column dimensions and styles, the problems involved in this case. --- src/PhpSpreadsheet/Reader/Xls.php | 22 ++++++++------ tests/PhpSpreadsheetTests/Reader/XlsTest.php | 30 +++++++++++++++++-- tests/data/Reader/XLS/bug1505.xls | Bin 0 -> 20992 bytes 3 files changed, 41 insertions(+), 11 deletions(-) create mode 100644 tests/data/Reader/XLS/bug1505.xls diff --git a/src/PhpSpreadsheet/Reader/Xls.php b/src/PhpSpreadsheet/Reader/Xls.php index b206f8ac..3f383b9e 100644 --- a/src/PhpSpreadsheet/Reader/Xls.php +++ b/src/PhpSpreadsheet/Reader/Xls.php @@ -3622,7 +3622,9 @@ class Xls extends BaseReader $this->phpSheet->getColumnDimensionByColumn($i)->setVisible(!$isHidden); $this->phpSheet->getColumnDimensionByColumn($i)->setOutlineLevel($level); $this->phpSheet->getColumnDimensionByColumn($i)->setCollapsed($isCollapsed); - $this->phpSheet->getColumnDimensionByColumn($i)->setXfIndex($this->mapCellXfIndex[$xfIndex]); + if (isset($this->mapCellXfIndex[$xfIndex])) { + $this->phpSheet->getColumnDimensionByColumn($i)->setXfIndex($this->mapCellXfIndex[$xfIndex]); + } } } } @@ -3731,7 +3733,7 @@ class Xls extends BaseReader $numValue = self::getIEEE754($rknum); $cell = $this->phpSheet->getCell($columnString . ($row + 1)); - if (!$this->readDataOnly) { + if (!$this->readDataOnly && isset($this->mapCellXfIndex[$xfIndex])) { // add style information $cell->setXfIndex($this->mapCellXfIndex[$xfIndex]); } @@ -3866,7 +3868,7 @@ class Xls extends BaseReader // offset: var; size: 4; RK value $numValue = self::getIEEE754(self::getInt4d($recordData, $offset + 2)); $cell = $this->phpSheet->getCell($columnString . ($row + 1)); - if (!$this->readDataOnly) { + if (!$this->readDataOnly && isset($this->mapCellXfIndex[$xfIndex])) { // add style $cell->setXfIndex($this->mapCellXfIndex[$xfIndex]); } @@ -3910,7 +3912,7 @@ class Xls extends BaseReader $numValue = self::extractNumber(substr($recordData, 6, 8)); $cell = $this->phpSheet->getCell($columnString . ($row + 1)); - if (!$this->readDataOnly) { + if (!$this->readDataOnly && isset($this->mapCellXfIndex[$xfIndex])) { // add cell style $cell->setXfIndex($this->mapCellXfIndex[$xfIndex]); } @@ -4018,7 +4020,7 @@ class Xls extends BaseReader } $cell = $this->phpSheet->getCell($columnString . ($row + 1)); - if (!$this->readDataOnly) { + if (!$this->readDataOnly && isset($this->mapCellXfIndex[$xfIndex])) { // add cell style $cell->setXfIndex($this->mapCellXfIndex[$xfIndex]); } @@ -4156,7 +4158,7 @@ class Xls extends BaseReader break; } - if (!$this->readDataOnly) { + if (!$this->readDataOnly && isset($this->mapCellXfIndex[$xfIndex])) { // add cell style $cell->setXfIndex($this->mapCellXfIndex[$xfIndex]); } @@ -4194,7 +4196,9 @@ class Xls extends BaseReader // Read cell? if (($this->getReadFilter() !== null) && $this->getReadFilter()->readCell($columnString, $row + 1, $this->phpSheet->getTitle())) { $xfIndex = self::getUInt2d($recordData, 4 + 2 * $i); - $this->phpSheet->getCell($columnString . ($row + 1))->setXfIndex($this->mapCellXfIndex[$xfIndex]); + if (isset($this->mapCellXfIndex[$xfIndex])) { + $this->phpSheet->getCell($columnString . ($row + 1))->setXfIndex($this->mapCellXfIndex[$xfIndex]); + } } } } @@ -4245,7 +4249,7 @@ class Xls extends BaseReader $cell = $this->phpSheet->getCell($columnString . ($row + 1)); $cell->setValueExplicit($value, DataType::TYPE_STRING); - if (!$this->readDataOnly) { + if (!$this->readDataOnly && isset($this->mapCellXfIndex[$xfIndex])) { // add cell style $cell->setXfIndex($this->mapCellXfIndex[$xfIndex]); } @@ -4277,7 +4281,7 @@ class Xls extends BaseReader $xfIndex = self::getUInt2d($recordData, 4); // add style information - if (!$this->readDataOnly && $this->readEmptyCells) { + if (!$this->readDataOnly && $this->readEmptyCells && isset($this->mapCellXfIndex[$xfIndex])) { $this->phpSheet->getCell($columnString . ($row + 1))->setXfIndex($this->mapCellXfIndex[$xfIndex]); } } diff --git a/tests/PhpSpreadsheetTests/Reader/XlsTest.php b/tests/PhpSpreadsheetTests/Reader/XlsTest.php index 77ad91fa..da39f8b2 100644 --- a/tests/PhpSpreadsheetTests/Reader/XlsTest.php +++ b/tests/PhpSpreadsheetTests/Reader/XlsTest.php @@ -3,9 +3,9 @@ namespace PhpOffice\PhpSpreadsheetTests\Reader; use PhpOffice\PhpSpreadsheet\Reader\Xls; -use PHPUnit\Framework\TestCase; +use PhpOffice\PhpSpreadsheetTests\Functional\AbstractFunctional; -class XlsTest extends TestCase +class XlsTest extends AbstractFunctional { /** * Test load Xls file. @@ -17,4 +17,30 @@ class XlsTest extends TestCase $spreadsheet = $reader->load($filename); self::assertEquals('Title', $spreadsheet->getSheet(0)->getCell('A1')->getValue()); } + + /** + * Test load Xls file with invalid xfIndex. + */ + public function testLoadXlsBug1505(): void + { + $filename = 'tests/data/Reader/XLS/bug1505.xls'; + $reader = new Xls(); + $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); + self::assertEquals($sheet->getCell('A1')->getFormattedValue(), $newsheet->getCell('A1')->getFormattedValue()); + self::assertEquals($sheet->getCell("$col$row")->getFormattedValue(), $newsheet->getCell("$col$row")->getFormattedValue()); + } } diff --git a/tests/data/Reader/XLS/bug1505.xls b/tests/data/Reader/XLS/bug1505.xls new file mode 100644 index 0000000000000000000000000000000000000000..3440ceb911192e86029bfd95eb2a8d7093765865 GIT binary patch literal 20992 zcmeI)du$cieFyM!uiw}PY=bezkBcApfo+T*7%;{**ch)vX-Rw41qg3sx`^PG)ZbI9rt@2m@=r)^%UHbd|=AJ$C zz2|JTRZ_K$a<9hs&TnS!XXea#&D`Vr&flawf8+Nv|6WBr4yq{i$;$*4>x&2I{SH09 zQPJ|3FL_qK7SVeO{qphuCmt2jBebiz|rklr@X>k2({>w63K%A$Ze z9n6zuyH%$8ONzy-S2dX%N_i?yWt){MD)Dn@odF6Su-$TVsL!?xw5_;06{_RRb{*6jQutlRPQ~MF~jsQK`H6Pj7 zXwW@sC!Irs>g_*u^~OT`x7@D2s@AK(4dp${)to9 zicY2d%S2fl)e? z5!zysRve>RFwm;5(c5`ibWF9VWEJS8jrh|zIp%3vr1h8Sbxcv(wa^Dts#>PHRF7=Z zN}DyS4th4zzQn7*fb1I|H6KkiXNZ?-9W5!O%)x2BjwY{B+6awOVz7Xgc#l`YucK7g z&lV}QRHxLfHl=FY6=jKds=c%MOsYc4I{`ZP_vws&D`}gY*G&2Q2G`xR*8%w?t}pcC zd87C4d1D~l5cbg)3F?mQol;+3?=9zfHvIEJTJtF%06lnl6g~aRqbZdRY&FYcDCc*0 znO4@1rIV(YtDpyY#~Mc)#RsXWrbnE5xhijH=Q+m55Exwu5e zc%LF;@s1cDVB~t7NRhEbM~rhs5dp>UTfP7M~pK}q><@}ZF9t^vtw;beH$y5hSn0U(*urCwG>;{f+`D3M8&E%rQk-QWQv0A{BjQ^B1(l>1`yg{Y>gb zgA@%C*G!mt(I7>Ol=G~jPD`A&Mv4I`2Ba8}Vl0VkL35sBMEb4d+=)~QY=WZmJ|yT*S6{EE%l?1#aU7uNO2;iz2E-ZTH?H5 zBx)CZQaniUAjMl!JV@~({W$ggU(ph$l#voZN&qPVqy$S!0Eur6q^tMarxvv2HGDdk zdnH+)C$F^WlDiJ1b(XXaq;(=4$!PjGHhiAAB${KQ?gq_iDG`n-5u`**N(3oUq>}Ex zey$~Me~iQ}zLAnZ;?~}j+$2j%0x3zPn)1xJBfz2^(0BHkA8$jA%NgF`gAkyF$ z->TJ;*E7<`TLvRBS;%9X(LD*MXGye`t6u--s9eaIi?girWBA;KuWQs6p&Ix z%1rp^nU=T@Vx&}%yjwK7Jf(t^YDuXerHb@P^W!I4;+~C>(m+asV@d-l&63hUN)u^o z?Ta}rdDs5>Jb73A`aEp{X%k4BEQzlgO{=j8HQ>s~KA29;1=A zfaG1Z>SNjh(iThF0@4qkABKL`~tP;6*j_Gd%fPKScW*c`dlz5zBML z@*S}PN375h+u?{6Iby|*ScxN6>WJ-h#L678a!0Jf5!>a6RXSpHtJ_Lbx?OL@_-0jP z+SfQ@wT{>xM~rS-S)a#Wv_u;F95K4XZ>?AFh#hdm8XU2Mju_tv^Y05^GlwW6@>wq-%+5HX{{*R0L8HNJW-Z1X7VmPv4FGk3cx}xV|(}F-XNA6@yf4 zNyQ))i}Wz*wGXt!wXl&&Kq>*L1f&v6DgmiPq^Z{YZ)u6^b|aO7R0>ilNTrrk3R0;^ z8SQtkB!p9sTMQ%Z1ZgKoJ3-oMNjpK>DN;@9zkWeW+zuJ345TuU%0Mc!q%x4oMC$(D z$nR*0TQ?(>gH#SuIY{M}R1Q+PNOMo${R=H|n`xv9kSai`0I9-~DnP0b>B(g4KWd3v zVk7MWX%|SlK-y(VyFl6{(zA*`e5oaF-;GoWQYA>0AXQpYB}kPbz5QWv*1B-s<6eW2 zc7wDVq}?Fxwxr!4?H1_=fBO1fEpcDONL3(Jfm8)jl_gbyR3*|+*OL0Q#627%RfALw zQZ-1`mQ)Q=wMe7i+0~yIPCf278L0-O8jxy0s1mFZxJPcJdXVZtst2jwlIlUK7pd{x zlzJ_3Ki^0PKso@@0gw(@(gBbTh;;P3AHAw2zIrfH14s=ZHGtG$Nev)1h&2ACnXhY! zuStw_5Tt`3@nA)B@BN@99R%s1NcYeD-FLL)edVw3K{bNZ2vQ?Rjh561Qlm&;37z~W zmiRuvn{prEXSoY7Bj5Qq(ee7P&G_CxvmWCe4#Ev^+ z?T%Q7BX+_OJL!mZI$~XpShpkA;G*f~dR&=EWD zh+S~Rh9Y8oAE1e1rffDriD`lo(*z}^$tp2TP-2>-#QgC;|5IN!)e@H#BOLiP;Fj1(ogj6B)Cp3j zC3S+-DUyFQ1-I`;>H?_?q%M%UEU62mE|L7BDY(~Qq;8P9LFxvn+mgCL>K4gAnu7Zx zM(P2n2c#a5dMv32q#lv{qbazDW293codW3;NT)366iBB;@{gwAev^@ULFxsm7o=WG z>IJD+B>!j%?u{9#4NdD0j z+;cS207wHM4S+OYNdq7ah~ytl!TnJqod)SNNT)$MZAqs=IxUiaGzIr^jdTX2Ga#J- z>5L_v0qKlL{?QcNcQ(>lkj{d17NoP5bQYwuBKb#CaF5(b=Ri6K(m9aMS<*R>&WYq7 zO~L(qBMpKy2+|-(gO)T1(x6EG(G-03V5IXPod@YWNaro-JV@t7@{gwAYZ4<}0O!0fuRRBku!@IARwau~A2C z%n=)R#3mfENk?qT5u0|zW*o6uN9>X#Hs^@VJ7Not*kwoTiX*n@h%Gr{%Z}JpN9>v- zcHI$Mal~#oVmBSJTaMUmN9>LxcGnTR7ZKz80K*hBC1w~(%rKOgVJI=fR*4yg5;H6% z#y={EbG4C1KpFvQ1f&s58UblUB>$)&E-Oa52+~E6E`oH?k}iUDQ6xPoh*OVCppiyF z8U<+-q)|&61!+_y|7Z#>*GBUG?@3>skAXA>(wHTUfixzPe>4Tx7DgI}V;To(9Hene z8V8C0zb#$)M^kVeWTXj@CP111X~L2wK$;NAKbnGTHX}`fGzroqNRyT{3DTrU{?Qa% zUm9r&q$!Z5K$^0oDUhZ_@{gwATG&X_AWefb4brqFO@lNol7BP>*X>4{0ci%L8IWcy zX$GVjk^G}6xWzEiEJ(8;&4M&*NwXl$isTC(G=Wf8fhM+d64Eonzy8RkoaFw)0KZT1-HaTS^#MQqy>-` zENKCx1(E!tDY$(%(q)h?gLE0B%a(K*q{|}tM^kXG!AMs?x&qP_kgizL6_Boo_sETO3#3~h-2&;BCEWt)mPr25 z6x`1@(ru7#gLE6D+m>`2q}w9-M^o_CgOTolbO)q6AlKcuStw_7o@u& z-395cCEW$-u1NmT6nv#)qD~wF zz9roU>Apz*5h9#!MtT6!1CSno^uUrHfb>8lJwk+2kI#mY)<9YVX$_<`OIibIO(g#} z>wJ!l^bn+nAUy=>p(Q;8>7hveZ`L{I80is6k3f0^(j!ZH1kxjs{NJo|el*f!kRF5d z7^KIR^cbYaBI$3|-=kj!S&UNjQ!xIKJO_iR>JMVsZ{k1n>mvMkN}oJN&vi2Qf9_8Y z{bI}l{e(-1evO8I=Vg(8y6M03;D1@vKX(@RoEz%ji{sS?NMBU5>Z0t2cLDb~KSoyL ze}vUOzkc3r=e+NXKkw?lu-{u4