pjfanning commented on code in PR #1276:
URL: https://github.com/apache/poi/pull/1276#discussion_r4045389968
##########
poi/src/main/java/org/apache/poi/ddf/EscherBSERecord.java:
##########
@@ -107,7 +107,7 @@ public int fillFields(byte[] data, int offset,
EscherRecordFactory recordFactory
field_9_name = data[pos + 33];
field_10_unused2 = data[pos + 34];
field_11_unused3 = data[pos + 35];
- bytesRemaining -= 36;
+ bytesRemaining = Math.max(0, bytesRemaining - 36);
Review Comment:
A BSE record with fewer than 36 bytes of payload is malformed (the fixed
fields have already been read past its declared end at this point), so fail
with a clear message rather than clamping and reporting more bytes consumed
than the record declared:
```suggestion
if (bytesRemaining < 36) {
throw new RecordFormatException("EscherBSERecord declares " +
bytesRemaining
+ " bytes but needs at least 36 for its fixed fields");
}
bytesRemaining -= 36;
```
##########
poi/src/test/java/org/apache/poi/ddf/TestEscherBSERecordMalformedInput.java:
##########
@@ -0,0 +1,42 @@
+/* ====================================================================
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+==================================================================== */
+
+package org.apache.poi.ddf;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import org.apache.poi.util.LittleEndian;
+import org.junit.jupiter.api.Test;
+
+class TestEscherBSERecordMalformedInput {
+
+ @Test
+ void truncatedBSEPayloadDoesNotProduceNegativeRemainingLength() {
+ byte[] data = new byte[44];
+
+ LittleEndian.putShort(data, 2, EscherBSERecord.RECORD_ID);
+ // A BSE record has 36 bytes of fixed fields after its 8-byte header.
+ // This malformed record declares only 35 bytes of payload.
+ LittleEndian.putInt(data, 4, 35);
+
+ EscherBSERecord record = new EscherBSERecord();
+ int bytesRead = record.fillFields(data, 0, new
DefaultEscherRecordFactory());
+
+ assertEquals(44, bytesRead);
Review Comment:
With the explicit check the test would assert the clear failure instead of
the over-reported size (note `44` here is one more than the 8 + 35 bytes the
record declares):
```suggestion
EscherBSERecord record = new EscherBSERecord();
DefaultEscherRecordFactory factory = new
DefaultEscherRecordFactory();
RecordFormatException e = assertThrows(RecordFormatException.class,
() -> record.fillFields(data, 0, factory));
assertTrue(e.getMessage().contains("needs at least 36"),
e.getMessage());
```
(imports: `assertThrows`, `assertTrue`,
`org.apache.poi.util.RecordFormatException`.)
##########
poi/src/main/java/org/apache/poi/ddf/EscherBSERecord.java:
##########
@@ -121,7 +121,7 @@ public int fillFields(byte[] data, int offset,
EscherRecordFactory recordFactory
bytesRead = field_12_blipRecord.fillFields( data, pos + 36,
recordFactory );
}
pos += 36 + bytesRead;
- bytesRemaining -= bytesRead;
+ bytesRemaining = Math.max(0, bytesRemaining - bytesRead);
Review Comment:
Same here: if the embedded blip consumed more than the BSE declared, the
file is inconsistent. Before this PR that surfaced as the misleading
`safelyClone` message; clamping hides it and the returned size again overshoots
the declared record. Better to say what went wrong:
```suggestion
if (bytesRead > bytesRemaining) {
throw new RecordFormatException("Embedded blip of
EscherBSERecord consumed " + bytesRead
+ " bytes but only " + bytesRemaining + " were
declared");
}
bytesRemaining -= bytesRead;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
