diff --git a/src/documentation/content/xdocs/changes.xml b/src/documentation/content/xdocs/changes.xml index be46902da..ec32c2c02 100644 --- a/src/documentation/content/xdocs/changes.xml +++ b/src/documentation/content/xdocs/changes.xml @@ -37,6 +37,7 @@ + 45133 - Fixed OBJ Record (5Dh) to pad the sub-record data to a 4-byte boundary 45145 - Fixed Sheet to always enforce RowRecordsAggregate before ValueRecordsAggregate 45123 - Fixed SharedFormulaRecord.convertSharedFormulas() to propagate token operand classes 45087 - Correctly detect date formats like [Black]YYYY as being date based diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml index ae5136b7e..d296c8937 100644 --- a/src/documentation/content/xdocs/status.xml +++ b/src/documentation/content/xdocs/status.xml @@ -34,6 +34,7 @@ + 45133 - Fixed OBJ Record (5Dh) to pad the sub-record data to a 4-byte boundary 45145 - Fixed Sheet to always enforce RowRecordsAggregate before ValueRecordsAggregate 45123 - Fixed SharedFormulaRecord.convertSharedFormulas() to propagate token operand classes 45087 - Correctly detect date formats like [Black]YYYY as being date based diff --git a/src/java/org/apache/poi/hssf/record/ObjRecord.java b/src/java/org/apache/poi/hssf/record/ObjRecord.java index 6583bd677..7d29654cd 100644 --- a/src/java/org/apache/poi/hssf/record/ObjRecord.java +++ b/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 - extends Record -{ +public final class ObjRecord 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, - * if it did then append it explicitly. - * See Bug 41242 for details. + * Add the EndSubRecord explicitly. + * + * 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 - - - - +} diff --git a/src/testcases/org/apache/poi/hssf/record/TestObjRecord.java b/src/testcases/org/apache/poi/hssf/record/TestObjRecord.java index 5a792ba7d..e8a6596e5 100644 --- a/src/testcases/org/apache/poi/hssf/record/TestObjRecord.java +++ b/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() ); - assertTrue( subrecords.get(0) instanceof CommonObjectDataSubRecord); - assertTrue( subrecords.get(1) instanceof EndSubRecord ); + assertEquals(2, subrecords.size() ); + assertTrue(subrecords.get(0) instanceof CommonObjectDataSubRecord); + 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); - byte[] subData = new byte[recordBytes.length - 4]; + assertEquals(28, 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()); + } }