From 163ff25594bb2751ea2ea5e3df4f82fdf9219304 Mon Sep 17 00:00:00 2001 From: Dominik Stadler Date: Mon, 7 Aug 2023 20:01:19 +0000 Subject: [PATCH] Bug 66425: Avoid a ClassCastException found via oss-fuzz We try to avoid throwing NullPointerException, but it was possible to trigger one here with a specially crafted input-file Should fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=61266 git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1911523 13f79535-47bb-0310-9956-ffa450edef68 --- .../apache/poi/integration/TestXLSX2CSV.java | 21 +++++ .../org/apache/poi/stress/TestAllFiles.java | 2 +- .../ReadOnlySharedStringsTable.java | 12 ++- .../TestReadOnlySharedStringsTable.java | 79 +++++++++++------- ...mized-XLSX2CSVFuzzer-5025401116950528.xlsx | Bin 0 -> 5615 bytes test-data/spreadsheet/stress.xls | Bin 60928 -> 61440 bytes 6 files changed, 80 insertions(+), 34 deletions(-) create mode 100644 test-data/spreadsheet/clusterfuzz-testcase-minimized-XLSX2CSVFuzzer-5025401116950528.xlsx diff --git a/poi-examples/src/test/java/org/apache/poi/integration/TestXLSX2CSV.java b/poi-examples/src/test/java/org/apache/poi/integration/TestXLSX2CSV.java index b172b38295..0867f12b81 100644 --- a/poi-examples/src/test/java/org/apache/poi/integration/TestXLSX2CSV.java +++ b/poi-examples/src/test/java/org/apache/poi/integration/TestXLSX2CSV.java @@ -18,6 +18,7 @@ package org.apache.poi.integration; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.PrintStream; @@ -26,6 +27,7 @@ import java.nio.charset.StandardCharsets; import org.apache.commons.io.output.UnsynchronizedByteArrayOutputStream; import org.apache.poi.examples.xssf.eventusermodel.XLSX2CSV; +import org.apache.poi.ooxml.POIXMLException; import org.apache.poi.openxml4j.opc.OPCPackage; import org.apache.poi.openxml4j.opc.PackageAccess; import org.apache.poi.xssf.XSSFTestDataSamples; @@ -94,6 +96,25 @@ public class TestXLSX2CSV { assertTrue(output.contains(",\"hello, xssf\",,\"hello, xssf\""), "Had: " + output); } + @Test + public void testInvalidSampleFile() throws Exception { + final UnsynchronizedByteArrayOutputStream outputBytes = UnsynchronizedByteArrayOutputStream.builder().get(); + PrintStream out = new PrintStream(outputBytes, true, StandardCharsets.UTF_8.name()); + + // The package open is instantaneous, as it should be. + try (OPCPackage p = OPCPackage.open(XSSFTestDataSamples.getSampleFile("clusterfuzz-testcase-minimized-XLSX2CSVFuzzer-5025401116950528.xlsx").getAbsolutePath(), PackageAccess.READ)) { + XLSX2CSV xlsx2csv = new XLSX2CSV(p, out, -1); + assertThrows(POIXMLException.class, + xlsx2csv::process); + } + + String errorOutput = errorBytes.toString(StandardCharsets.UTF_8); + assertEquals("", errorOutput); + + String output = outputBytes.toString(StandardCharsets.UTF_8); + assertEquals("", output, "Had: " + output); + } + @Test public void testMinColumns() throws Exception { final UnsynchronizedByteArrayOutputStream outputBytes = UnsynchronizedByteArrayOutputStream.builder().get(); diff --git a/poi-integration/src/test/java/org/apache/poi/stress/TestAllFiles.java b/poi-integration/src/test/java/org/apache/poi/stress/TestAllFiles.java index 446375affd..a9f3b37aee 100644 --- a/poi-integration/src/test/java/org/apache/poi/stress/TestAllFiles.java +++ b/poi-integration/src/test/java/org/apache/poi/stress/TestAllFiles.java @@ -235,7 +235,7 @@ public class TestAllFiles { @SuppressWarnings("unchecked") private static void verify(String file, Executable exec, Class exClass, String exMessage, String password, FileHandler fileHandler) { - final String errPrefix = file + " - failed for handler " + fileHandler.getClass().getSimpleName() + ": "; + final String errPrefix = file.replace("\\", "/") + " - failed for handler " + fileHandler.getClass().getSimpleName() + ": "; // this also removes the password for non encrypted files Biff8EncryptionKey.setCurrentUserPassword(password); if (exClass != null && AssertionFailedError.class.isAssignableFrom(exClass)) { diff --git a/poi-ooxml/src/main/java/org/apache/poi/xssf/eventusermodel/ReadOnlySharedStringsTable.java b/poi-ooxml/src/main/java/org/apache/poi/xssf/eventusermodel/ReadOnlySharedStringsTable.java index 6336836821..07d00bd99a 100644 --- a/poi-ooxml/src/main/java/org/apache/poi/xssf/eventusermodel/ReadOnlySharedStringsTable.java +++ b/poi-ooxml/src/main/java/org/apache/poi/xssf/eventusermodel/ReadOnlySharedStringsTable.java @@ -251,7 +251,9 @@ public class ReadOnlySharedStringsTable extends DefaultHandler implements Shared this.strings = new ArrayList<>(this.uniqueCount); characters = new StringBuilder(64); } else if ("si".equals(localName)) { - characters.setLength(0); + if (characters != null) { + characters.setLength(0); + } } else if ("t".equals(localName)) { tIsOpen = true; } else if ("rPh".equals(localName)) { @@ -269,7 +271,9 @@ public class ReadOnlySharedStringsTable extends DefaultHandler implements Shared } if ("si".equals(localName)) { - strings.add(characters.toString()); + if (strings != null && characters != null) { + strings.add(characters.toString()); + } } else if ("t".equals(localName)) { tIsOpen = false; } else if ("rPh".equals(localName)) { @@ -285,7 +289,9 @@ public class ReadOnlySharedStringsTable extends DefaultHandler implements Shared if (inRPh && includePhoneticRuns) { characters.append(ch, start, length); } else if (! inRPh){ - characters.append(ch, start, length); + if (characters != null) { + characters.append(ch, start, length); + } } } } diff --git a/poi-ooxml/src/test/java/org/apache/poi/xssf/eventusermodel/TestReadOnlySharedStringsTable.java b/poi-ooxml/src/test/java/org/apache/poi/xssf/eventusermodel/TestReadOnlySharedStringsTable.java index 7df35d713c..be486270ed 100644 --- a/poi-ooxml/src/test/java/org/apache/poi/xssf/eventusermodel/TestReadOnlySharedStringsTable.java +++ b/poi-ooxml/src/test/java/org/apache/poi/xssf/eventusermodel/TestReadOnlySharedStringsTable.java @@ -20,6 +20,7 @@ package org.apache.poi.xssf.eventusermodel; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import java.io.IOException; import java.io.InputStream; @@ -39,7 +40,7 @@ import org.xml.sax.SAXException; * Tests for {@link org.apache.poi.xssf.eventusermodel.XSSFReader} */ public final class TestReadOnlySharedStringsTable { - private static POIDataSamples _ssTests = POIDataSamples.getSpreadSheetInstance(); + private static final POIDataSamples _ssTests = POIDataSamples.getSpreadSheetInstance(); @Test void testParse() throws Exception { @@ -47,24 +48,25 @@ public final class TestReadOnlySharedStringsTable { List parts = pkg.getPartsByName(Pattern.compile("/xl/sharedStrings.xml")); assertEquals(1, parts.size()); - SharedStringsTable stbl = new SharedStringsTable(parts.get(0)); - ReadOnlySharedStringsTable rtbl = new ReadOnlySharedStringsTable(parts.get(0)); - ReadOnlySharedStringsTable rtbl2; - try (InputStream stream = parts.get(0).getInputStream()){ - rtbl2 = new ReadOnlySharedStringsTable(stream); - } + try (SharedStringsTable stbl = new SharedStringsTable(parts.get(0))) { + ReadOnlySharedStringsTable rtbl = new ReadOnlySharedStringsTable(parts.get(0)); + ReadOnlySharedStringsTable rtbl2; + try (InputStream stream = parts.get(0).getInputStream()) { + rtbl2 = new ReadOnlySharedStringsTable(stream); + } - assertEquals(stbl.getCount(), rtbl.getCount()); - assertEquals(stbl.getUniqueCount(), rtbl.getUniqueCount()); - assertEquals(stbl.getUniqueCount(), rtbl2.getUniqueCount()); + assertEquals(stbl.getCount(), rtbl.getCount()); + assertEquals(stbl.getUniqueCount(), rtbl.getUniqueCount()); + assertEquals(stbl.getUniqueCount(), rtbl2.getUniqueCount()); - assertEquals(stbl.getCount(), stbl.getUniqueCount()); - assertEquals(rtbl.getCount(), rtbl.getUniqueCount()); - assertEquals(rtbl.getCount(), rtbl2.getUniqueCount()); - for (int i = 0; i < stbl.getUniqueCount(); i++) { - RichTextString i1 = stbl.getItemAt(i); - assertEquals(i1.getString(), rtbl.getItemAt(i).getString()); - assertEquals(i1.getString(), rtbl2.getItemAt(i).getString()); + assertEquals(stbl.getCount(), stbl.getUniqueCount()); + assertEquals(rtbl.getCount(), rtbl.getUniqueCount()); + assertEquals(rtbl.getCount(), rtbl2.getUniqueCount()); + for (int i = 0; i < stbl.getUniqueCount(); i++) { + RichTextString i1 = stbl.getItemAt(i); + assertEquals(i1.getString(), rtbl.getItemAt(i).getString()); + assertEquals(i1.getString(), rtbl2.getItemAt(i).getString()); + } } } } @@ -75,20 +77,21 @@ public final class TestReadOnlySharedStringsTable { List parts = pkg.getPartsByName(Pattern.compile("/xl/sharedStrings.xml")); assertEquals(1, parts.size()); - SharedStringsTable stbl = new SharedStringsTable(parts.get(0)); - ReadOnlySharedStringsTable rtbl = new ReadOnlySharedStringsTable(parts.get(0)); - ReadOnlySharedStringsTable rtbl2; - try (InputStream stream = parts.get(0).getInputStream()) { - rtbl2 = new ReadOnlySharedStringsTable(stream); - } + try (SharedStringsTable stbl = new SharedStringsTable(parts.get(0))) { + ReadOnlySharedStringsTable rtbl = new ReadOnlySharedStringsTable(parts.get(0)); + ReadOnlySharedStringsTable rtbl2; + try (InputStream stream = parts.get(0).getInputStream()) { + rtbl2 = new ReadOnlySharedStringsTable(stream); + } - assertEquals(stbl.getCount(), rtbl.getCount()); - assertEquals(stbl.getUniqueCount(), rtbl.getUniqueCount()); - assertEquals(stbl.getUniqueCount(), rtbl2.getUniqueCount()); - for (int i = 0; i < stbl.getUniqueCount(); i++) { - RichTextString i1 = stbl.getItemAt(i); - assertEquals(i1.getString(), rtbl.getItemAt(i).getString()); - assertEquals(i1.getString(), rtbl2.getItemAt(i).getString()); + assertEquals(stbl.getCount(), rtbl.getCount()); + assertEquals(stbl.getUniqueCount(), rtbl.getUniqueCount()); + assertEquals(stbl.getUniqueCount(), rtbl2.getUniqueCount()); + for (int i = 0; i < stbl.getUniqueCount(); i++) { + RichTextString i1 = stbl.getItemAt(i); + assertEquals(i1.getString(), rtbl.getItemAt(i).getString()); + assertEquals(i1.getString(), rtbl2.getItemAt(i).getString()); + } } } } @@ -130,6 +133,22 @@ public final class TestReadOnlySharedStringsTable { } } + @Test + void testNullPointerException() throws Exception { + try (OPCPackage pkg = OPCPackage.open(_ssTests.openResourceAsStream("clusterfuzz-testcase-minimized-XLSX2CSVFuzzer-5025401116950528.xlsx"))) { + assertEmptySST(pkg); + } + + try (OPCPackage pkg = OPCPackage.open(_ssTests.openResourceAsStream("clusterfuzz-testcase-minimized-XLSX2CSVFuzzer-5025401116950528.xlsx"))) { + List parts = pkg.getPartsByName(Pattern.compile("/xl/sharedStrings.xml")); + assertEquals(1, parts.size()); + + //noinspection resource + assertThrows(IOException.class, + () -> new SharedStringsTable(parts.get(0))); + } + } + private void assertEmptySST(OPCPackage pkg) throws IOException, SAXException { ReadOnlySharedStringsTable sst = new ReadOnlySharedStringsTable(pkg); assertEquals(0, sst.getCount()); diff --git a/test-data/spreadsheet/clusterfuzz-testcase-minimized-XLSX2CSVFuzzer-5025401116950528.xlsx b/test-data/spreadsheet/clusterfuzz-testcase-minimized-XLSX2CSVFuzzer-5025401116950528.xlsx new file mode 100644 index 0000000000000000000000000000000000000000..391aa7b2a8722f95f6ae8ca1e8fb33a97e56af75 GIT binary patch literal 5615 zcmeHLXIN9q*509aM4}PtML=qTp(#C7k#gvWw1h|}R6!#}no1KX!l5bVfJ&1hMJXX5 zH3}$56A)2EDTZR9%D2Ju9F^<$|33HL^vYI$E6f#Rk~Rgm`-i+#ZIA-vX$>{{OT8un3e`*qOAz zp<#+k>QbTdCGVpUv81RBy(5rt4R`jEe9!DWLJRS$7pD-NYiGzvr6JLd*#FGM{NX(H z{?SQhzg%`5ItO~pR?$A($_y#DGdWGMo0dIhm@2Mobs3F(rQ4ifjhE6kfKm#y?X?Qv z>>MiLwy_pRr#c>_{PCgs^rZ$aHe3N)UUPqh2MdBvwZ*LQM0cLT<|E6>at9aog>ZRG zL5qYQ4;?9su#lB_mwh)3i%B$F4v4@c#3eZ!D%0uZOsQjqT>B~zP=A#R*#54=orMz5 z;)^mheR4$30SljxyRtWGBb}_}yI)1?+tCka&-US%A7Rw?X$2OP^QUduUA=YkLTH0MPbJD=9E;l@?Gda1lMXd@mcBXk0 zZ*G_Nv^1QVy||)-(j5`xHvI|JW&62jGCBO(2D|zViA#TSn0%a_f3w6{Z`jeqd|X6H z^~(5@f#PdO4F%WttIPnfy-f|6{<0&hE#bnSL7JTcHykt2199}ndIiYJ>^uc-cmHoE z{>SXVgp^+6Rya#LMZW>{c4O8@pCP}X{*->bpaoyE$VdL_M1qLM*Dul3ktZ=PpM9C> zRkxe?_|Dyep9d{#-1KrqU`eNFEIIc={r>b(OWs zTBH#!MD|@MI>R_YBaib0`P?+>Ec3nImv^(Y$OcgdR&x3|x;BI`)On(#XG}hA*C_c3 z8D8zvpIlLS6&Ii}H+Ml4xg8@SupI`?|o^81?co6F>0-tEltLw-2pnZW3#Yi*);|`uTZBd;ZD@({NaXh+=A>$H7s`nmwsBUz$vR`C7Ip~Ny?)-| zMlS?NGuHPx6~q;L;rdGCht$d`rXDHsBPY7p&k_TcPi+eWdW-~7hjj?$PFmMveQk-C z|B%T*lkSt-9!+*5ld}xZYBcre6m+>XKvUEexM)j?!pf-vaYqo-BDLpU*#Bo2M82-v8rjhlP|FhgXj`%kl~;*e1_7Yb{h!}g{^JOP>4=7)?wI$z)}F*3&K8%N8Mb3^dpG5^e} zH%>i%hobZMt+)K8CoSXkZE;z+dj#cVG2B6Z8KtWeuTgw!F=9h)g7*k39ihER>{w=6 zU(Y8E8y#`n#VAC;_pHG6z?ge3U9md#Ny6x#Ep`{!FIj2owPM8G;om$O9QstxafVkap-fNV*E?U>4J@3PX zYdKRwjwJELRl##pRDy&|5V9mcNfx^1G<4o`2Q|B5IV&cdw76Hj%Zygca9K&2Dupmu zLucn!4O5+tnP=r`EH~{{rst$<&Uaz(o5b9uRj>-aX?gN#BMZFOp(&ELQpQ+!xXfgn zNm<`Pc?J~~XFET&>5sTpP+(dz4#0GyWo6H-&h}GVCJ-xHW0q7HX(egaV1D^?5Le`L z%9Tt?+p!q5&Gc%)oDuQ;5-9Aat%?FyM}#U z{$UMVn&0T#32!+=68Mi6xIV%^a#MgC78@Awx76*vApQ}$=xd}nup-y_O8pHLf=?Zc zVL_QI3e{Q8P@~sJ>0V|ee}!)0t9Y=@)6>4a(+b%eD)+e0vR#)NctjQR76WrDU&N0} z6E!Q8X2+n6(kjW${W48i)W*gquXZHc9AK`9`9oyuP;1!Zk%}b7d&z3=Mhvc=hj*>T zbN7`Ld3Y~7(m#+juBJ%64YiIsu4-ocZ9XZKU&V(&?9pHDjW$qDc2c{ouiUA#upEKb zoD2Sv=NZaIQ|SGKyz+(drwH)mMtZ(j7>$AD)V`N3xm}xbBT3i`?RN2Uq29S^ttwc! zQMN_-=g`(KAvfo~vFf&l7 z6HmCWW}P&rToccg)mYanEpukzdF9e6mPATj&I?J(Vi`CWVgY}_bcFw#vV7j!0Iv)r z5}!Ywf*$J-IkJoD41UYowpl(oXbGDSk#2@vBtN}+R(4)E<1d@^i;B@74YJ-G5UlY$ zoYmO(3Txb^`t)uDf8OQUvqE=UFqhjOp9?}&!9oRIw;Rd^b)YDT;x+ML*9iN9Yn0Eo zMnjLr^M2Ly%8*0iN!}fGVvb20!WCp#R%0e%GmC++DSD1$2i|;t>F^uH3gY$r5oV?K z`;51WgBG)ggP5N>_%ajQO@`*ChRSTZ5OMN7lqTPYJvcN>28oyCTYlLuxv^$6?;mqk zc2b<^9pM+Y;E~Qluu=%KBZBS7bRJ=wH=AFeN_7#2e_8$d_s*RN^c#jN8Bp%YS zDi6_Jv-B=}Ey*#zuU6=0zK^s>dF&qYpKI}`~oSECuY&Dk@DKkPnBos zPRGQ%)W022tBHG#;Cp!Ali)n}*^@bGRic;Zuf$QuRFu7Vu(SPfklr@;>GR_&uQBJ@ z7;WNkf;NN$1$Nyxb@i?_70z#fRrW=nIPG9y>o99BR%Ufmob9yj@Lx{0lJNqoI{Uuf zTF+ldzbdYyH^+ut%gkR(ZLdL(RB65UV;a2YmPUwvQJZ)>jh^o3KN3=$+U%lQO6>9Xm1sZgdUUOEm%5VH7y5ZxBKvVk0M+ekJdjBym* zn;|@-klxhI`#(JW_5*ib_;VU!N%iJGM_mY&G$>QQzkb@wqu*Rcnf}x#1ysDBPRFK( zzxw|5aPbok2&3CA6aSy1#$G>r3(Oy$TEIKBJ>}+J@LqNP0e%FA1O)$!R`2z&S3`by zuwwmZ_`elpr||u|x%(rxBP+kxnZ3h&C_GXZ3C6{od*>qXXR~439VXBX>y4G&eeSB& zUI%x2*_0+hofDt|0B~YI8S{bfUmAGc3%gs6?1jpoyTarLUO-M91b{<70RiA3Z0{%j zkjuSZIHt$n&Vp!P@G!6soX8JrGD9DU!+?&`WTD!P#g;v{(~SJtidPngJ-(!oqr#Dor&BMRfLxx2MlY+^(Z zB-Pg}REIX{Dv47;Bm^aaX&tRV%!?q=ty@v{`*w9gVqoUI_j}*_y-Um0mLz;|ub^yAH#dHk|8&{Ji6a)VpT=bi9+@h_m&lvcUlqa z=uCbUW2nWPWCx0|7_4B&AgPq_Mpe{FQ-!9T6%y+9;s-XG%b@qF$osDPvZ=f}pPW`> ze%t<%kE65`d1y(T(??=Mk#O`n3Q_l#r|Wy1lQPh4{i-k1q=45dJvCd*PI9N=>+Ego z>J1$2>S>LSjp?Jlpg#~iuaCND}`h4Yb zAxl}^w~o~9$(0ImkL)?96yE{OtN=nYt%>$oI g7u$+n%md3Gfc9k|wF<0$wbZ2_Wqc-?p8Dqa3+!md{r~^~ delta 517 zcmZXPT_}T59LAq>-gnsczS}SnEpw}Pa^tg25+yCkh1ivn3uVk_^07wbYQk~Fh2`3X z92b<5M3I#XwG?tG$&HjKbIxQXo%;9te?HHNJSdS{Wk;pvQ2<}bWO9t_^oX!4rF^AZ zT~+%{p-7|A!iLmE`hL$p)HB5#@fk+T`3+InY`ALITucdNZma8Gbh0I@IaQUEsZdZB z)0~zpEEAz7(+ADT9<901XrB@Su&plr*=E&epZ zS^@Lw(oRfRsQYVSLjM(BPt#$FE_8YCS@-;0Wo0%o$-OpzX+i6-(1Zm|6Ft45(Z0cf zkp(!AIA*cx4%55>wDZqumz(b9^1?0={J|}I;hI=EodY~8K*c6-w+$TZ1HNMxjJNU5 znJEWZskE