Fix for 45133 - OBJ Record (5Dh) needs to pad the sub-record data to a 4-byte boundary

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@663855 13f79535-47bb-0310-9956-ffa450edef68

src/documentation/content/xdocs/changes.xml

@@ -37,6 +37,7 @@
 
 		<!-- Don't forget to update status.xml too! -->
         <release version="3.1-final" date="2008-06-??">
+           <action dev="POI-DEVELOPERS" type="fix">45133 - Fixed OBJ Record (5Dh) to pad the sub-record data to a 4-byte boundary</action>
            <action dev="POI-DEVELOPERS" type="fix">45145 - Fixed Sheet to always enforce RowRecordsAggregate before ValueRecordsAggregate</action>
            <action dev="POI-DEVELOPERS" type="fix">45123 - Fixed SharedFormulaRecord.convertSharedFormulas() to propagate token operand classes</action>
            <action dev="POI-DEVELOPERS" type="fix">45087 - Correctly detect date formats like [Black]YYYY as being date based</action>

src/documentation/content/xdocs/status.xml

@@ -34,6 +34,7 @@
 	<!-- Don't forget to update changes.xml too! -->
     <changes>
         <release version="3.1-final" date="2008-06-??">
+           <action dev="POI-DEVELOPERS" type="fix">45133 - Fixed OBJ Record (5Dh) to pad the sub-record data to a 4-byte boundary</action>
            <action dev="POI-DEVELOPERS" type="fix">45145 - Fixed Sheet to always enforce RowRecordsAggregate before ValueRecordsAggregate</action>
            <action dev="POI-DEVELOPERS" type="fix">45123 - Fixed SharedFormulaRecord.convertSharedFormulas() to propagate token operand classes</action>
            <action dev="POI-DEVELOPERS" type="fix">45087 - Correctly detect date formats like [Black]YYYY as being date based</action>

src/java/org/apache/poi/hssf/record/ObjRecord.java

@@ -1,4 +1,3 @@
-
 /* ====================================================================
    Licensed to the Apache Software Foundation (ASF) under one or more
    contributor license agreements.  See the NOTICE file distributed with
@@ -16,27 +15,21 @@
    limitations under the License.
 ==================================================================== */
         
-
-
 package org.apache.poi.hssf.record;
 
-
-
-import org.apache.poi.util.*;
-
 import java.io.ByteArrayInputStream;
-import java.util.List;
-import java.util.Iterator;
 import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+
+import org.apache.poi.util.LittleEndian;
 
 /**
  * The obj record is used to hold various graphic objects and controls.
  *
  * @author Glen Stampoultzis (glens at apache.org)
  */
-public class ObjRecord
+public final class ObjRecord extends Record {
-    extends Record
-{
     public final static short      sid                             = 0x5D;
     private List subrecords;
 
@@ -47,6 +40,7 @@ public class ObjRecord
     public ObjRecord()
     {
         subrecords = new ArrayList(2);
+        // TODO - ensure 2 sub-records (ftCmo  15h, and ftEnd  00h) are always created
     }
 
     /**
@@ -80,6 +74,7 @@ public class ObjRecord
         //following wont work properly
         int subSize = 0;
         byte[] subRecordData = in.readRemainder();
+
         RecordInputStream subRecStream = new RecordInputStream(new ByteArrayInputStream(subRecordData));
         while(subRecStream.hasNextRecord()) {
           subRecStream.nextRecord();
@@ -89,28 +84,19 @@ public class ObjRecord
         }
 
         /**
-         * Check if the RecordInputStream skipped EndSubRecord,
+         * Add the EndSubRecord explicitly.
-         * if it did then append it explicitly.
+         * 
-         * See Bug 41242 for details.
+         * TODO - the reason the EndSubRecord is always skipped is because its 'sid' is zero and
+         * that causes subRecStream.hasNextRecord() to return false.
+         * There may be more than the size of EndSubRecord left-over, if there is any padding 
+         * after that record.  The content of the EndSubRecord and the padding is all zeros.
+         * So there's not much to look at past the last substantial record.
+         * 
+         * See Bugs 41242/45133 for details.
          */
-        if (subRecordData.length - subSize == 4){
+        if (subRecordData.length - subSize >= 4) {
             subrecords.add(new EndSubRecord());
         }
-
-        /* JMH the size present/not present in the code below
-           needs to be considered in the RecordInputStream??
-        int pos = offset;
-        while (pos - offset <= size-2) // atleast one "short" must be present
-        {
-            short subRecordSid = LittleEndian.getShort(data, pos);
-            short subRecordSize = -1; // set default to "< 0"
-            if (pos-offset <= size-4) { // see if size info is present, else default to -1
-                subRecordSize = LittleEndian.getShort(data, pos + 2);
-            }
-            Record subRecord = SubRecord.createSubRecord(subRecordSid, subRecordSize, data, pos + 4);
-            subrecords.add(subRecord);
-            pos += subRecord.getRecordSize();
-        }*/
     }
 
     public String toString()
@@ -140,6 +126,8 @@ public class ObjRecord
             Record record = (Record) iterator.next();
             pos += record.serialize(pos, data);
         }
+        // assume padding (if present) does not need to be written.
+        // it is probably zero already, and it probably doesn't matter anyway
 
         return getRecordSize();
     }
@@ -155,7 +143,9 @@ public class ObjRecord
             Record record = (Record) iterator.next();
             size += record.getRecordSize();
         }
-        return 4  + size;
+        int oddBytes = size & 0x03;
+        int padding = oddBytes == 0 ? 0 : 4 - oddBytes;
+        return 4  + size + padding;
     }
 
     public short getSid()
@@ -192,9 +182,4 @@ public class ObjRecord
 
         return rec;
     }
-
+}
-}  // END OF CLASS
-
-
-
-

src/testcases/org/apache/poi/hssf/record/TestObjRecord.java

@@ -1,4 +1,3 @@
-
 /* ====================================================================
    Licensed to the Apache Software Foundation (ASF) under one or more
    contributor license agreements.  See the NOTICE file distributed with
@@ -18,18 +17,19 @@
 
 package org.apache.poi.hssf.record;
 
-import junit.framework.*;
-
 import java.util.Arrays;
 import java.util.List;
 
+import junit.framework.AssertionFailedError;
+import junit.framework.TestCase;
+
 /**
  * Tests the serialization and deserialization of the ObjRecord class works correctly.
  * Test data taken directly from a real Excel file.
  *
  * @author Yegor Kozlov
  */
-public class TestObjRecord extends TestCase {
+public final class TestObjRecord extends TestCase {
     /**
      * OBJ record data containing two sub-records.
      * The data taken directly from a real Excel file.
@@ -38,22 +38,27 @@ public class TestObjRecord extends TestCase {
      *     [ftCmo]
      *     [ftEnd]
      */
-    public static byte[] recdata = {
+    private static final byte[] recdata = {
         0x15, 0x00, 0x12, 0x00, 0x06, 0x00, 0x01, 0x00, 0x11, 0x60,
         (byte)0xF4, 0x02, 0x41, 0x01, 0x14, 0x10, 0x1F, 0x02, 0x00, 0x00,
         0x00, 0x00, 0x00, 0x00, 0x00, 0x00
+        // TODO - this data seems to require two extra bytes padding. not sure where original file is.
+        // it's not bug 38607 attachment 17639
     };
 
+    private static final byte[] recdataNeedingPadding = {
+    	21, 0, 18, 0, 0, 0, 1, 0, 17, 96, 0, 0, 0, 0, 56, 111, -52, 3, 0, 0, 0, 0, 6, 0, 2, 0, 0, 0, 0, 0, 0, 0
+    };
 
-    public void testLoad() throws Exception {
+    public void testLoad() {
         ObjRecord record = new ObjRecord(new TestcaseRecordInputStream(ObjRecord.sid, (short)recdata.length, recdata));
 
-        assertEquals( recdata.length, record.getRecordSize() - 4);
+        assertEquals(28, record.getRecordSize() - 4);
 
         List subrecords = record.getSubRecords();
-        assertEquals( 2, subrecords.size() );
+        assertEquals(2, subrecords.size() );
-        assertTrue( subrecords.get(0) instanceof CommonObjectDataSubRecord);
+        assertTrue(subrecords.get(0) instanceof CommonObjectDataSubRecord);
-        assertTrue( subrecords.get(1) instanceof EndSubRecord );
+        assertTrue(subrecords.get(1) instanceof EndSubRecord );
 
     }
 
@@ -61,8 +66,8 @@ public class TestObjRecord extends TestCase {
         ObjRecord record = new ObjRecord(new TestcaseRecordInputStream(ObjRecord.sid, (short)recdata.length, recdata));
 
         byte [] recordBytes = record.serialize();
-        assertEquals(recdata.length, recordBytes.length - 4);
+        assertEquals(28, recordBytes.length - 4);
-        byte[] subData = new byte[recordBytes.length - 4];
+        byte[] subData = new byte[recdata.length];
         System.arraycopy(recordBytes, 4, subData, 0, subData.length);
         assertTrue(Arrays.equals(recdata, subData));
     }
@@ -92,4 +97,20 @@ public class TestObjRecord extends TestCase {
         assertTrue( subrecords.get(0) instanceof CommonObjectDataSubRecord);
         assertTrue( subrecords.get(1) instanceof EndSubRecord );
     }
+    
+    public void testReadWriteWithPadding_bug45133() {
+        ObjRecord record = new ObjRecord(new TestcaseRecordInputStream(ObjRecord.sid, (short)recdataNeedingPadding.length, recdataNeedingPadding));
+        
+        if (record.getRecordSize() == 34) {
+        	throw new AssertionFailedError("Identified bug 45133");
+        }
+
+        assertEquals(36, record.getRecordSize());
+
+        List subrecords = record.getSubRecords();
+        assertEquals(3, subrecords.size() );
+        assertEquals(CommonObjectDataSubRecord.class, subrecords.get(0).getClass());
+        assertEquals(GroupMarkerSubRecord.class, subrecords.get(1).getClass());
+        assertEquals(EndSubRecord.class, subrecords.get(2).getClass());
+    }
 }