andygrove commented on code in PR #1422:
URL: https://github.com/apache/arrow-java/pull/1422#discussion_r4460018896
##########
vector/src/main/java/org/apache/arrow/vector/ipc/message/MessageSerializer.java:
##########
@@ -411,50 +411,51 @@ public static ArrowRecordBatch deserializeRecordBatch(
*/
public static ArrowRecordBatch deserializeRecordBatch(RecordBatch
recordBatchFB, ArrowBuf body)
throws IOException {
- // Now read the body
- int nodesLength = recordBatchFB.nodesLength();
- List<ArrowFieldNode> nodes = new ArrayList<>();
- for (int i = 0; i < nodesLength; ++i) {
- FieldNode node = recordBatchFB.nodes(i);
- if ((int) node.length() != node.length() || (int) node.nullCount() !=
node.nullCount()) {
- throw new IOException(
- "Cannot currently deserialize record batches with "
- + "node length larger than INT_MAX records.");
+ try {
+ // Now read the body
+ int nodesLength = recordBatchFB.nodesLength();
+ List<ArrowFieldNode> nodes = new ArrayList<>();
+ for (int i = 0; i < nodesLength; ++i) {
+ FieldNode node = recordBatchFB.nodes(i);
+ if ((int) node.length() != node.length() || (int) node.nullCount() !=
node.nullCount()) {
Review Comment:
Added in 6f2c26eda. The `ArrowFieldNode` constructor called right after this
check already rejected these, but with an `IllegalArgumentException`, so the
IPC readers failed on a malformed node with an unchecked exception instead of
the `IOException` they declare. The loop now rejects a negative length and a
null count below -1 with an `IOException`. The new checks run before the
int-range check, so a length of `Long.MIN_VALUE` is reported as negative, not
as larger than INT_MAX.
##########
vector/src/test/java/org/apache/arrow/vector/ipc/MessageSerializerTest.java:
##########
@@ -220,6 +223,44 @@ public void testSerializeRecordBatchV5() throws Exception {
alloc.close();
}
+ @Test
+ public void testNegativeNodeLengthDoesNotLeakBody() {
Review Comment:
Added in 6f2c26eda. I replaced the two tests here with two parameterized
tests that go through `deserializeRecordBatch`:
- `testInvalidFieldNodeRejectedWithoutLeakingBody`: lengths of -100,
`Long.MIN_VALUE`, `Integer.MAX_VALUE + 1` and `2^32`, and a null count of -2.
Each must throw `IOException` and release the body.
- `testValidFieldNodeDeserialized`: a length of 0, a null count of -1, and a
length of `Integer.MAX_VALUE`. Each must come back with the same length and
null count, and leave nothing allocated.
The -100 and -2 cases failed before the change above, with
`IllegalArgumentException`.
--
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]
