From: Dominik Stadler Date: Tue, 16 Jul 2024 05:26:42 +0000 (+0000) Subject: Bug 66425: Avoid exceptions found via poi-fuzz X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=09fbfd5be45c7597cb1517a95e18b19429c47d58;p=poi.git Bug 66425: Avoid exceptions found via poi-fuzz Avoid a possible OutOfMemoryException with many child-records This avoids having too many children in EscherRecords, the limit of 100_000 is arbitrarily chosen and can be adjusted if needed Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=62924 and maybe others git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1919272 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/poi-scratchpad/src/test/java/org/apache/poi/hslf/dev/BaseTestPPTIterating.java b/poi-scratchpad/src/test/java/org/apache/poi/hslf/dev/BaseTestPPTIterating.java index 5372a23bbf..e1458db0cb 100644 --- a/poi-scratchpad/src/test/java/org/apache/poi/hslf/dev/BaseTestPPTIterating.java +++ b/poi-scratchpad/src/test/java/org/apache/poi/hslf/dev/BaseTestPPTIterating.java @@ -72,6 +72,7 @@ public abstract class BaseTestPPTIterating { EXCLUDED.put("clusterfuzz-testcase-minimized-POIHSLFFuzzer-4624961081573376.ppt", FileNotFoundException.class); EXCLUDED.put("clusterfuzz-testcase-minimized-POIHSLFFuzzer-5018229722382336.ppt", RuntimeException.class); EXCLUDED.put("clusterfuzz-testcase-minimized-POIHSLFFuzzer-6192650357112832.ppt", RuntimeException.class); + EXCLUDED.put("clusterfuzz-testcase-minimized-POIHSLFFuzzer-6614960949821440.ppt", RuntimeException.class); } public static Stream files() { diff --git a/poi-scratchpad/src/test/java/org/apache/poi/hslf/dev/TestPPTXMLDump.java b/poi-scratchpad/src/test/java/org/apache/poi/hslf/dev/TestPPTXMLDump.java index ca4d09be24..213b05387c 100644 --- a/poi-scratchpad/src/test/java/org/apache/poi/hslf/dev/TestPPTXMLDump.java +++ b/poi-scratchpad/src/test/java/org/apache/poi/hslf/dev/TestPPTXMLDump.java @@ -36,6 +36,7 @@ public class TestPPTXMLDump extends BaseTestPPTIterating { LOCAL_EXCLUDED.add("clusterfuzz-testcase-minimized-POIHSLFFuzzer-6032591399288832.ppt"); LOCAL_EXCLUDED.add("clusterfuzz-testcase-minimized-POIHSLFFuzzer-6360479850954752.ppt"); LOCAL_EXCLUDED.add("ppt_with_png_encrypted.ppt"); + LOCAL_EXCLUDED.add("clusterfuzz-testcase-minimized-POIHSLFFuzzer-6614960949821440.ppt"); } @Test diff --git a/poi-scratchpad/src/test/java/org/apache/poi/hslf/dev/TestSlideShowDumper.java b/poi-scratchpad/src/test/java/org/apache/poi/hslf/dev/TestSlideShowDumper.java index 06a742c191..290616ce10 100644 --- a/poi-scratchpad/src/test/java/org/apache/poi/hslf/dev/TestSlideShowDumper.java +++ b/poi-scratchpad/src/test/java/org/apache/poi/hslf/dev/TestSlideShowDumper.java @@ -41,6 +41,11 @@ public class TestSlideShowDumper extends BaseTestPPTIterating { FAILING.add("clusterfuzz-testcase-minimized-POIHSLFFuzzer-6360479850954752.ppt"); } + static final Set LOCAL_EXCLUDED = new HashSet<>(); + static { + LOCAL_EXCLUDED.add("clusterfuzz-testcase-minimized-POIHSLFFuzzer-6614960949821440.ppt"); + } + @Test void testMain() throws IOException { // SlideShowDumper calls IOUtils.toByteArray(is), which would fail if a different size is defined @@ -71,6 +76,11 @@ public class TestSlideShowDumper extends BaseTestPPTIterating { throw e; } } + + // these fail everywhere else, so also should fail here + if (LOCAL_EXCLUDED.contains(pFile.getName())) { + throw new RuntimeException(); + } } @Override diff --git a/poi/src/main/java/org/apache/poi/ddf/EscherContainerRecord.java b/poi/src/main/java/org/apache/poi/ddf/EscherContainerRecord.java index 747dbde1a5..8a0e555736 100644 --- a/poi/src/main/java/org/apache/poi/ddf/EscherContainerRecord.java +++ b/poi/src/main/java/org/apache/poi/ddf/EscherContainerRecord.java @@ -208,6 +208,12 @@ public final class EscherContainerRecord extends EscherRecord implements Iterabl if (childRecords == _childRecords) { throw new IllegalStateException("Child records private data member has escaped"); } + + if (childRecords.size() > MAX_NUMBER_OF_CHILDREN) { + throw new IllegalStateException("Cannot add more than " + MAX_NUMBER_OF_CHILDREN + + " child records, you can use 'EscherRecord.setMaxNumberOfChildren()' to increase the allow size"); + } + _childRecords.clear(); _childRecords.addAll(childRecords); } @@ -261,6 +267,11 @@ public final class EscherContainerRecord extends EscherRecord implements Iterabl * @param record the record to be added */ public void addChildRecord(EscherRecord record) { + if (_childRecords.size() >= MAX_NUMBER_OF_CHILDREN) { + throw new IllegalStateException("Cannot add more than " + MAX_NUMBER_OF_CHILDREN + + " child records, you can use 'EscherRecord.setMaxNumberOfChildren()' to increase the allow size"); + } + _childRecords.add(record); } diff --git a/poi/src/main/java/org/apache/poi/ddf/EscherRecord.java b/poi/src/main/java/org/apache/poi/ddf/EscherRecord.java index 3d9aeacca8..e633a92079 100644 --- a/poi/src/main/java/org/apache/poi/ddf/EscherRecord.java +++ b/poi/src/main/java/org/apache/poi/ddf/EscherRecord.java @@ -45,6 +45,10 @@ public abstract class EscherRecord implements Duplicatable, GenericRecord { private short _options; private short _recordId; + // arbitrarily selected; may need to increase + private static final int DEFAULT_MAX_NUMBER_OF_CHILDREN = 100_000; + protected static int MAX_NUMBER_OF_CHILDREN = DEFAULT_MAX_NUMBER_OF_CHILDREN; + /** * Create a new instance */ @@ -367,4 +371,18 @@ public abstract class EscherRecord implements Duplicatable, GenericRecord { @Override public abstract EscherRecord copy(); + + /** + * @param length the max record length allowed for EscherArrayProperty + */ + public static void setMaxNumberOfChildren(int length) { + MAX_NUMBER_OF_CHILDREN = length; + } + + /** + * @return the max record length allowed for EscherArrayProperty + */ + public static int getMaxNumberOfChildren() { + return MAX_NUMBER_OF_CHILDREN; + } } \ No newline at end of file diff --git a/poi/src/main/java/org/apache/poi/ddf/UnknownEscherRecord.java b/poi/src/main/java/org/apache/poi/ddf/UnknownEscherRecord.java index a81286c5c7..67b6bc6317 100644 --- a/poi/src/main/java/org/apache/poi/ddf/UnknownEscherRecord.java +++ b/poi/src/main/java/org/apache/poi/ddf/UnknownEscherRecord.java @@ -160,6 +160,12 @@ public final class UnknownEscherRecord extends EscherRecord { if (childRecords == _childRecords) { return; } + + if (childRecords.size() > MAX_NUMBER_OF_CHILDREN) { + throw new IllegalStateException("Cannot add more than " + MAX_NUMBER_OF_CHILDREN + + " child records, you can use 'EscherRecord.setMaxNumberOfChildren()' to increase the allow size"); + } + _childRecords.clear(); _childRecords.addAll(childRecords); } @@ -170,6 +176,11 @@ public final class UnknownEscherRecord extends EscherRecord { } public void addChildRecord(EscherRecord childRecord) { + if (_childRecords.size() >= MAX_NUMBER_OF_CHILDREN) { + throw new IllegalStateException("Cannot add more than " + MAX_NUMBER_OF_CHILDREN + + " child records, you can use 'EscherRecord.setMaxNumberOfChildren()' to increase the allow size"); + } + getChildRecords().add( childRecord ); } diff --git a/poi/src/test/java/org/apache/poi/ddf/TestEscherContainerRecord.java b/poi/src/test/java/org/apache/poi/ddf/TestEscherContainerRecord.java index 4b1cfd6291..d13188a53d 100644 --- a/poi/src/test/java/org/apache/poi/ddf/TestEscherContainerRecord.java +++ b/poi/src/test/java/org/apache/poi/ddf/TestEscherContainerRecord.java @@ -20,19 +20,23 @@ package org.apache.poi.ddf; import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotSame; -import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.assertThrows; +import java.util.Arrays; import java.util.List; import org.apache.poi.POIDataSamples; import org.apache.poi.util.HexDump; import org.apache.poi.util.HexRead; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.parallel.Isolated; /** * Tests for {@link EscherContainerRecord} */ +@Isolated // this test changes global static MAX_NUMBER_OF_CHILDREN final class TestEscherContainerRecord { private static final POIDataSamples _samples = POIDataSamples.getDDFInstance(); @@ -42,7 +46,7 @@ final class TestEscherContainerRecord { byte[] data = HexRead.readFromString("0F 02 11 F1 00 00 00 00"); EscherRecord r = f.createRecord(data, 0); r.fillFields(data, 0, f); - assertTrue(r instanceof EscherContainerRecord); + assertInstanceOf(EscherContainerRecord.class, r); assertEquals((short) 0x020F, r.getOptions()); assertEquals((short) 0xF111, r.getRecordId()); @@ -91,7 +95,7 @@ final class TestEscherContainerRecord { "\t, \"recordSize\": 8\n" + "\t, \"isContainer\": true\n" + "}"; - expected = expected.replace("\n", System.getProperty("line.separator")); + expected = expected.replace("\n", System.lineSeparator()); assertEquals(expected, r.toString()); EscherOptRecord r2 = new EscherOptRecord(); @@ -121,7 +125,7 @@ final class TestEscherContainerRecord { "\t\t}\n" + "\t]\n" + "}"; - expected = expected.replace("\n", System.getProperty("line.separator")); + expected = expected.replace("\n", System.lineSeparator()); assertEquals(expected, r.toString()); r.addChildRecord(r2); @@ -155,7 +159,7 @@ final class TestEscherContainerRecord { "\t\t}\n" + "\t]\n" + "}"; - expected = expected.replace("\n", System.getProperty("line.separator")); + expected = expected.replace("\n", System.lineSeparator()); assertEquals(expected, r.toString()); } @@ -170,7 +174,7 @@ final class TestEscherContainerRecord { @Override public String getRecordName() { return ""; } @Override - public Enum getGenericRecordType() { return EscherRecordTypes.UNKNOWN; } + public Enum getGenericRecordType() { return EscherRecordTypes.UNKNOWN; } @Override public DummyEscherRecord copy() { return null; } } @@ -231,4 +235,25 @@ final class TestEscherContainerRecord { assertEquals(chC, children2.get(0)); assertEquals(chA, children2.get(1)); } + + @Test + void testTooManyChildren() { + EscherContainerRecord ecr = new EscherContainerRecord(); + + int before = EscherRecord.getMaxNumberOfChildren(); + try { + EscherRecord.setMaxNumberOfChildren(2); + + ecr.addChildRecord(new EscherDgRecord()); + ecr.addChildRecord(new EscherDgRecord()); + assertThrows(IllegalStateException.class, + () -> ecr.addChildRecord(new EscherDgRecord())); + + ecr.setChildRecords(Arrays.asList(new EscherDgRecord(), new EscherDgRecord())); + assertThrows(IllegalStateException.class, + () -> ecr.setChildRecords(Arrays.asList(new EscherDgRecord(), new EscherDgRecord(), new EscherDgRecord()))); + } finally { + EscherRecord.setMaxNumberOfChildren(before); + } + } } diff --git a/poi/src/test/java/org/apache/poi/ddf/TestUnknownEscherRecord.java b/poi/src/test/java/org/apache/poi/ddf/TestUnknownEscherRecord.java index fa00b7c739..697fd854cb 100644 --- a/poi/src/test/java/org/apache/poi/ddf/TestUnknownEscherRecord.java +++ b/poi/src/test/java/org/apache/poi/ddf/TestUnknownEscherRecord.java @@ -19,12 +19,17 @@ package org.apache.poi.ddf; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import org.apache.poi.util.HexDump; import org.apache.poi.util.HexRead; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.parallel.Isolated; +import java.util.Arrays; + +@Isolated // this test changes global static MAX_NUMBER_OF_CHILDREN final class TestUnknownEscherRecord { @Test void testFillFields() { @@ -159,7 +164,28 @@ final class TestUnknownEscherRecord { "\t, \"recordSize\": 8\n" + "\t, \"data\": \"\"\n" + "}"; - expected = expected.replace("\n", System.getProperty("line.separator")); + expected = expected.replace("\n", System.lineSeparator()); assertEquals(expected, r.toString() ); } + + @Test + void testTooManyChildren() { + UnknownEscherRecord ecr = new UnknownEscherRecord(); + + int before = EscherRecord.getMaxNumberOfChildren(); + try { + EscherRecord.setMaxNumberOfChildren(2); + + ecr.addChildRecord(new EscherDgRecord()); + ecr.addChildRecord(new EscherDgRecord()); + assertThrows(IllegalStateException.class, + () -> ecr.addChildRecord(new EscherDgRecord())); + + ecr.setChildRecords(Arrays.asList(new EscherDgRecord(), new EscherDgRecord())); + assertThrows(IllegalStateException.class, + () -> ecr.setChildRecords(Arrays.asList(new EscherDgRecord(), new EscherDgRecord(), new EscherDgRecord()))); + } finally { + EscherRecord.setMaxNumberOfChildren(before); + } + } } diff --git a/test-data/slideshow/clusterfuzz-testcase-minimized-POIHSLFFuzzer-6614960949821440.ppt b/test-data/slideshow/clusterfuzz-testcase-minimized-POIHSLFFuzzer-6614960949821440.ppt new file mode 100644 index 0000000000..43ed103f45 Binary files /dev/null and b/test-data/slideshow/clusterfuzz-testcase-minimized-POIHSLFFuzzer-6614960949821440.ppt differ diff --git a/test-data/spreadsheet/stress.xls b/test-data/spreadsheet/stress.xls index 78bed83bc0..124e07a16f 100644 Binary files a/test-data/spreadsheet/stress.xls and b/test-data/spreadsheet/stress.xls differ