Avoid OOM with incorrect property sizes

Add allocation check to verify size >= 0 and < 1mio
Also reformat code to match general coding style

Fixes https://issues.oss-fuzz.com/issues/485091380

poi-ooxml/src/main/java/org/apache/poi/xslf/usermodel/XSLFDiagram.java

@@ -148,8 +148,12 @@ public class XSLFDiagram extends XSLFGraphicFrame {
         shapeCt.setSpPr(msShapeCt.getSpPr());
 
         CTShapeNonVisual nonVisualCt = shapeCt.addNewNvSpPr();
-        nonVisualCt.setCNvPr(msShapeCt.getNvSpPr().getCNvPr());
+        com.microsoft.schemas.office.drawing.x2008.diagram.CTShapeNonVisual nvSpPr = msShapeCt.getNvSpPr();
-        nonVisualCt.setCNvSpPr(msShapeCt.getNvSpPr().getCNvSpPr());
+        if (nvSpPr == null) {
+            nvSpPr = msShapeCt.addNewNvSpPr();
+        }
+        nonVisualCt.setCNvPr(nvSpPr.getCNvPr());
+        nonVisualCt.setCNvSpPr(nvSpPr.getCNvSpPr());
         nonVisualCt.setNvPr(CTApplicationNonVisualDrawingProps.Factory.newInstance());
         shapeCt.setNvSpPr(nonVisualCt);
 

poi-ooxml/src/main/java/org/apache/poi/xslf/usermodel/XSLFTable.java

@@ -427,6 +427,10 @@ public class XSLFTable extends XSLFGraphicFrame implements Iterable<XSLFTableRow
                 for (int col2=col+1; col2<col+tc.getGridSpan(); col2++) {
                     assert(col2 < cols);
                     XSLFTableCell tc2 = getCell(row, col2);
+                    if (tc2 == null) {
+                        LOG.warn("unavailable table span - rendering result is probably wrong");
+                        continue;
+                    }
                     if (tc2.getGridSpan() != 1 || tc2.getRowSpan() != 1) {
                         LOG.warn("invalid table span - rendering result is probably wrong");
                     }
@@ -435,6 +439,10 @@ public class XSLFTable extends XSLFGraphicFrame implements Iterable<XSLFTableRow
                 for (int row2=row+1; row2<row+tc.getRowSpan(); row2++) {
                     assert(row2 < rows);
                     XSLFTableCell tc2 = getCell(row2, col);
+                    if (tc2 == null) {
+                        LOG.warn("unavailable table span - rendering result is probably wrong");
+                        continue;
+                    }
                     if (tc2.getGridSpan() != 1 || tc2.getRowSpan() != 1) {
                         LOG.warn("invalid table span - rendering result is probably wrong");
                     }

poi-ooxml/src/main/java/org/apache/poi/xssf/usermodel/XSSFPicture.java

@@ -268,7 +268,7 @@ public final class XSSFPicture extends XSSFShape implements Picture {
      */
     @Override
     public XSSFPictureData getPictureData() {
-        if (ctPicture.getBlipFill().getBlip() == null) {
+        if (ctPicture.getBlipFill() == null || ctPicture.getBlipFill().getBlip() == null) {
             return null;
         }
 

poi-scratchpad/src/main/java/org/apache/poi/hemf/record/emf/HemfFill.java

@@ -35,6 +35,7 @@ import java.util.Map;
 import java.util.function.Supplier;
 
 import org.apache.commons.io.output.UnsynchronizedByteArrayOutputStream;
+import org.apache.logging.log4j.Logger;
 import org.apache.poi.hemf.draw.HemfDrawProperties;
 import org.apache.poi.hemf.draw.HemfGraphics;
 import org.apache.poi.hwmf.draw.HwmfGraphics;
@@ -45,6 +46,7 @@ import org.apache.poi.hwmf.record.HwmfFill;
 import org.apache.poi.hwmf.record.HwmfFill.ColorUsage;
 import org.apache.poi.hwmf.record.HwmfRegionMode;
 import org.apache.poi.hwmf.record.HwmfTernaryRasterOp;
+import org.apache.poi.logging.PoiLogManager;
 import org.apache.poi.util.GenericRecordJsonWriter;
 import org.apache.poi.util.GenericRecordUtil;
 import org.apache.poi.util.IOUtils;
@@ -52,6 +54,8 @@ import org.apache.poi.util.LittleEndianConsts;
 import org.apache.poi.util.LittleEndianInputStream;
 
 public final class HemfFill {
+    private static final Logger LOG = PoiLogManager.getLogger(HemfFill.class);
+
     private HemfFill() {}
 
     /**
@@ -194,11 +198,6 @@ public final class HemfFill {
             super.draw(ctx);
         }
 
-        @Override
-        public String toString() {
-            return GenericRecordJsonWriter.marshal(this);
-        }
-
         public Rectangle2D getBounds() {
             return bounds;
         }
@@ -321,14 +320,8 @@ public final class HemfFill {
         public HemfRecordType getEmfRecordType() {
             return HemfRecordType.bitBlt;
         }
-
-        @Override
-        protected boolean srcEqualsDstDimension() {
-            return false;
-        }
     }
 
-
     /** The EMR_FRAMERGN record draws a border around the specified region using the specified brush. */
     public static class EmfFrameRgn extends HwmfDraw.WmfFrameRegion implements HemfRecord {
         private final Rectangle2D bounds = new Rectangle2D.Double();
@@ -611,9 +604,13 @@ public final class HemfFill {
             size += readBounds2(leis, destRect);
 
             blendOperation = leis.readByte();
-            assert (blendOperation == 0);
+            if (blendOperation != 0) {
+                LOG.atWarn().log("Unexpected blend-operation in emf file: {}", blendOperation);
+            }
             blendFlags = leis.readByte();
-            assert (blendOperation == 0);
+            if (blendFlags != 0) {
+                LOG.atWarn().log("Unexpected blend-flags in emf file: {}", blendFlags);
+            }
             srcConstantAlpha = leis.readUByte();
             alphaFormat = leis.readByte();
 

poi-scratchpad/src/main/java/org/apache/poi/hemf/usermodel/HemfPicture.java

@@ -81,6 +81,8 @@ public class HemfPicture implements Iterable<HemfRecord>, GenericRecord {
         List<HemfRecord> r = getRecords();
         if (r.isEmpty()) {
             throw new RecordFormatException("No records could be parsed - your .emf file is invalid");
+        } else if (!(r.get(0) instanceof HemfHeader)) {
+            throw new RecordFormatException("Could not convert object of type " + r.get(0).getClass() + " + - your .emf file is invalid");
         } else {
             return (HemfHeader)r.get(0);
         }

poi-scratchpad/src/test/java/org/apache/poi/hslf/dev/TestSlideIdListing.java

@@ -33,6 +33,7 @@ public class TestSlideIdListing extends BaseTestPPTIterating {
         LOCAL_EXCLUDED.add("clusterfuzz-testcase-minimized-POIHSLFFuzzer-5306877435838464.ppt");
         LOCAL_EXCLUDED.add("clusterfuzz-testcase-minimized-POIHSLFFuzzer-6360479850954752.ppt");
         LOCAL_EXCLUDED.add("clusterfuzz-testcase-minimized-POIHSLFFuzzer-6028723156746240.ppt");
+        LOCAL_EXCLUDED.add("clusterfuzz-testcase-minimized-POIHSLFFuzzer-4983252485210112.ppt");
     }
 
     @Test

poi-scratchpad/src/test/java/org/apache/poi/hslf/dev/TestSlideShowRecordDumper.java

@@ -33,6 +33,7 @@ public class TestSlideShowRecordDumper extends BaseTestPPTIterating {
     static {
         LOCAL_EXCLUDED.add("clusterfuzz-testcase-minimized-POIHSLFFuzzer-6360479850954752.ppt");
         LOCAL_EXCLUDED.add("clusterfuzz-testcase-minimized-POIHSLFFuzzer-6028723156746240.ppt");
+        LOCAL_EXCLUDED.add("clusterfuzz-testcase-minimized-POIHSLFFuzzer-4983252485210112.ppt");
     }
 
     @Test

poi-scratchpad/src/test/java/org/apache/poi/hslf/dev/TestUserEditAndPersistListing.java

@@ -32,6 +32,7 @@ public class TestUserEditAndPersistListing extends BaseTestPPTIterating {
     static {
         LOCAL_EXCLUDED.add("clusterfuzz-testcase-minimized-POIHSLFFuzzer-6360479850954752.ppt");
         LOCAL_EXCLUDED.add("clusterfuzz-testcase-minimized-POIHSLFFuzzer-6028723156746240.ppt");
+        LOCAL_EXCLUDED.add("clusterfuzz-testcase-minimized-POIHSLFFuzzer-4983252485210112.ppt");
     }
 
     @Test

poi/src/main/java/org/apache/poi/ddf/AbstractEscherOptRecord.java

@@ -23,6 +23,7 @@ import java.util.Map;
 import java.util.function.Supplier;
 
 import org.apache.poi.util.GenericRecordUtil;
+import org.apache.poi.util.IOUtils;
 import org.apache.poi.util.LittleEndian;
 
 /**
@@ -30,6 +31,9 @@ import org.apache.poi.util.LittleEndian;
  * {@link EscherTertiaryOptRecord}
  */
 public abstract class AbstractEscherOptRecord extends EscherRecord {
+    // arbitrary limit, can be adjusted if it turns out to be too low
+    private static final int MAX_PROPERTY_SIZE = 1_000_000;
+
     private final List<EscherProperty> properties = new ArrayList<>();
 
     protected AbstractEscherOptRecord() {}
@@ -39,21 +43,17 @@ public abstract class AbstractEscherOptRecord extends EscherRecord {
         properties.addAll(other.properties);
     }
 
-
     /**
      * Add a property to this record.
      *
      * @param prop the escher property to add
      */
-    public void addEscherProperty( EscherProperty prop )
+    public void addEscherProperty( EscherProperty prop ) {
-    {
         properties.add( prop );
     }
 
     @Override
-    public int fillFields( byte[] data, int offset,
+    public int fillFields( byte[] data, int offset, EscherRecordFactory recordFactory ) {
-            EscherRecordFactory recordFactory )
-    {
         int bytesRemaining = readHeader( data, offset );
         if (bytesRemaining < 0) {
             throw new IllegalStateException("Invalid value for bytesRemaining: " + bytesRemaining);
@@ -72,8 +72,7 @@ public abstract class AbstractEscherOptRecord extends EscherRecord {
      *
      * @return the list of properties
      */
-    public List<EscherProperty> getEscherProperties()
+    public List<EscherProperty> getEscherProperties() {
-    {
         return properties;
     }
 
@@ -83,26 +82,23 @@ public abstract class AbstractEscherOptRecord extends EscherRecord {
      * @param index the ordinal index of the property
      * @return the escher property
      */
-    public EscherProperty getEscherProperty( int index )
+    public EscherProperty getEscherProperty( int index ) {
-    {
         return properties.get( index );
     }
 
-
+    private int getPropertiesSize() {
-    private int getPropertiesSize()
-    {
         int totalSize = 0;
-        for ( EscherProperty property : properties )
+        for ( EscherProperty property : properties ) {
-        {
+            int propertySize = property.getPropertySize();
-            totalSize += property.getPropertySize();
+            IOUtils.safelyAllocateCheck(propertySize, MAX_PROPERTY_SIZE);
+            totalSize += propertySize;
         }
 
         return totalSize;
     }
 
     @Override
-    public int getRecordSize()
+    public int getRecordSize() {
-    {
         return 8 + getPropertiesSize();
     }
 
@@ -116,9 +112,7 @@ public abstract class AbstractEscherOptRecord extends EscherRecord {
     }
 
     @Override
-    public int serialize( int offset, byte[] data,
+    public int serialize( int offset, byte[] data, EscherSerializationListener listener ) {
-            EscherSerializationListener listener )
-    {
         listener.beforeRecordSerialize( offset, getRecordId(), this );
 
         LittleEndian.putShort( data, offset, getOptions() );

poi/src/main/java/org/apache/poi/ddf/EscherRecord.java

@@ -70,8 +70,7 @@ public abstract class EscherRecord implements Duplicatable, GenericRecord {
      *
      * @see #fillFields(byte[], int, org.apache.poi.ddf.EscherRecordFactory)
      */
-    protected int fillFields( byte[] data, EscherRecordFactory f )
+    protected int fillFields( byte[] data, EscherRecordFactory f ) {
-    {
         return fillFields( data, 0, f );
     }
 
@@ -154,8 +153,7 @@ public abstract class EscherRecord implements Duplicatable, GenericRecord {
      * @return The options field for this record. All records have one.
      */
     @Internal
-    public short getOptions()
+    public short getOptions() {
-    {
         return _options;
     }
 
@@ -183,8 +181,7 @@ public abstract class EscherRecord implements Duplicatable, GenericRecord {
      * @return  the serialized record.
      * @see #serialize(int, byte[])
      */
-    public byte[] serialize()
+    public byte[] serialize() {
-    {
         byte[] retval = new byte[getRecordSize()];
 
         serialize( 0, retval );
@@ -201,8 +198,7 @@ public abstract class EscherRecord implements Duplicatable, GenericRecord {
      *
      * @see #serialize(int, byte[], org.apache.poi.ddf.EscherSerializationListener)
      */
-    public int serialize( int offset, byte[] data)
+    public int serialize( int offset, byte[] data) {
-    {
         return serialize( offset, data, new NullEscherSerializationListener() );
     }
 
@@ -252,7 +248,9 @@ public abstract class EscherRecord implements Duplicatable, GenericRecord {
      *
      * @see EscherContainerRecord
      */
-    public List<EscherRecord> getChildRecords() { return Collections.emptyList(); }
+    public List<EscherRecord> getChildRecords() {
+        return Collections.emptyList();
+    }
 
     /**
      * Sets the child records for this record.  By default this will throw
@@ -281,8 +279,7 @@ public abstract class EscherRecord implements Duplicatable, GenericRecord {
      * @param w         The print writer to output to.
      * @param indent    The current indent level.
      */
-    public void display(PrintWriter w, int indent)
+    public void display(PrintWriter w, int indent) {
-    {
         for (int i = 0; i < indent * 4; i++) {
             w.print(' ');
         }
@@ -301,8 +298,7 @@ public abstract class EscherRecord implements Duplicatable, GenericRecord {
      *
      * @return The instance part of the record
      */
-    public short getInstance()
+    public short getInstance() {
-    {
         return fInstance.getShortValue( _options );
     }
 
@@ -311,8 +307,7 @@ public abstract class EscherRecord implements Duplicatable, GenericRecord {
      *
      * @param value instance part value
      */
-    public void setInstance( short value )
+    public void setInstance( short value ) {
-    {
         _options = fInstance.setShortValue( _options, value );
     }
 
@@ -321,8 +316,7 @@ public abstract class EscherRecord implements Duplicatable, GenericRecord {
      *
      * @return The version part of the option record
      */
-    public short getVersion()
+    public short getVersion() {
-    {
         return fVersion.getShortValue( _options );
     }
 
@@ -331,12 +325,11 @@ public abstract class EscherRecord implements Duplicatable, GenericRecord {
      *
      * @param value version part value
      */
-    public void setVersion( short value )
+    public void setVersion( short value ) {
-    {
         _options = fVersion.setShortValue( _options, value );
     }
 
-    public String toXml(){
+    public String toXml() {
         return toXml("");
     }
 
@@ -344,7 +337,7 @@ public abstract class EscherRecord implements Duplicatable, GenericRecord {
      * @param tab - each children must be indented right relative to its parent
      * @return xml representation of this record
      */
-    public final String toXml(String tab){
+    public final String toXml(String tab) {
         return GenericRecordXmlWriter.marshal(this);
     }
 

poi/src/main/java/org/apache/poi/hssf/usermodel/HSSFShapeGroup.java

@@ -355,7 +355,11 @@ public class HSSFShapeGroup extends HSSFShape implements HSSFShapeContainer {
         EscherContainerRecord containerRecord = getEscherContainer().getChildById(EscherContainerRecord.SP_CONTAINER);
         EscherSpRecord spRecord = containerRecord.getChildById(EscherSpRecord.RECORD_ID);
         spRecord.setShapeId(shapeId);
-        CommonObjectDataSubRecord cod = (CommonObjectDataSubRecord) getObjRecord().getSubRecords().get(0);
+        ObjRecord objRecord = getObjRecord();
+        if (objRecord == null) {
+            throw new IllegalStateException("Did not have an ObjRecord for the HSSFShapeGroup");
+        }
+        CommonObjectDataSubRecord cod = (CommonObjectDataSubRecord) objRecord.getSubRecords().get(0);
         cod.setObjectId((short) (shapeId % 1024));
     }
 

poi/src/test/java/org/apache/poi/hssf/model/TestDrawingAggregate.java

@@ -139,7 +139,8 @@ class TestDrawingAggregate {
                         !file.getName().equals("clusterfuzz-testcase-minimized-POIHSSFFuzzer-5285517825277952.xls") &&
                         !file.getName().equals("clusterfuzz-testcase-minimized-POIHSSFFuzzer-4977868385681408.xls") &&
                         !file.getName().equals("crash-e329fca9087fe21bca4a80c8bc472a661c98d860.xls") &&
-                        !file.getName().equals("cf9f845e73447b092477d0472402a5baea4b8c9f.xls")).
+                        !file.getName().equals("cf9f845e73447b092477d0472402a5baea4b8c9f.xls") &&
+                        !file.getName().equals("LIBRE_OFFICE-94379-0.zip-57.xls")).
                 map(Arguments::of);
     }