]> source.dussan.org Git - poi.git/commitdiff
Bug 66425: Avoid exceptions found via poi-fuzz
authorDominik Stadler <centic@apache.org>
Tue, 16 Jul 2024 05:26:42 +0000 (05:26 +0000)
committerDominik Stadler <centic@apache.org>
Tue, 16 Jul 2024 05:26:42 +0000 (05:26 +0000)
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

poi-scratchpad/src/test/java/org/apache/poi/hslf/dev/BaseTestPPTIterating.java
poi-scratchpad/src/test/java/org/apache/poi/hslf/dev/TestPPTXMLDump.java
poi-scratchpad/src/test/java/org/apache/poi/hslf/dev/TestSlideShowDumper.java
poi/src/main/java/org/apache/poi/ddf/EscherContainerRecord.java
poi/src/main/java/org/apache/poi/ddf/EscherRecord.java
poi/src/main/java/org/apache/poi/ddf/UnknownEscherRecord.java
poi/src/test/java/org/apache/poi/ddf/TestEscherContainerRecord.java
poi/src/test/java/org/apache/poi/ddf/TestUnknownEscherRecord.java
test-data/slideshow/clusterfuzz-testcase-minimized-POIHSLFFuzzer-6614960949821440.ppt [new file with mode: 0644]
test-data/spreadsheet/stress.xls

index 5372a23bbf4a793c49dc33418f2012cf3a14d3c1..e1458db0cbc8dfab4ccbb6d1d46a8a4282149949 100644 (file)
@@ -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<Arguments> files() {
index ca4d09be24dc89c4ebf04ae573e5299686e2ad63..213b05387cd1fdf5d064ad552ea5c351b46372f8 100644 (file)
@@ -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
index 06a742c1914e96624fb573b2424554e63f1ea22e..290616ce10b6f2d68d2447a2e0a7986563ccee73 100644 (file)
@@ -41,6 +41,11 @@ public class TestSlideShowDumper extends BaseTestPPTIterating {
         FAILING.add("clusterfuzz-testcase-minimized-POIHSLFFuzzer-6360479850954752.ppt");
     }
 
+    static final Set<String> 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
index 747dbde1a59cd9f4e58b9b9e6ed4812d1b33a482..8a0e555736319a9f926327655d8e1a4137e1453f 100644 (file)
@@ -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);
     }
 
index 3d9aeacca8554ae496c79d13c5cf8bcc60c344ad..e633a9207911427ae81afd0b307b789c47541a8a 100644 (file)
@@ -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
index a81286c5c72af4f053aca359678fcf22a837a38f..67b6bc63178aacdb4e516f38f662645ab4f98716 100644 (file)
@@ -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 );
     }
 
index 4b1cfd6291c35a15affa80b6bb4a38a4b05bfa58..d13188a53d8d6be3071f982c46bb537883ad5798 100644 (file)
@@ -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);
+        }
+    }
 }
index fa00b7c73924648d514c233ae1ccc4507770872b..697fd854cbde109de20149ae26a4d481ffd3a23d 100644 (file)
@@ -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 (file)
index 0000000..43ed103
Binary files /dev/null and b/test-data/slideshow/clusterfuzz-testcase-minimized-POIHSLFFuzzer-6614960949821440.ppt differ
index 78bed83bc03e28c6a52effe35ceddc2f839a0333..124e07a16f3ad80831342740eb17f0c21e1a5839 100644 (file)
Binary files a/test-data/spreadsheet/stress.xls and b/test-data/spreadsheet/stress.xls differ