Repository navigation
GH-1421: reject negative FieldNode length during deserialization #1422
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,10 +16,9 @@ | |
| */ | ||
| package org.apache.arrow.vector.ipc.message; | ||
|
|
||
| import com.google.flatbuffers.FlatBufferBuilder; | ||
| import org.apache.arrow.flatbuf.FieldNode; | ||
| import org.apache.arrow.util.Preconditions; | ||
|
|
||
| /** Metadata about Vectors/Arrays that is written to a channel. */ | ||
| public class ArrowFieldNode implements FBSerializable { | ||
|
|
@@ -31,12 +30,14 @@ public class ArrowFieldNode implements FBSerializable { | |
| * Constructs a new instance. | ||
| * | ||
| * @param length The number of values written. | ||
| * @param nullCount The number of null values, or -1 if unknown. | ||
| */ | ||
| public ArrowFieldNode(long length, long nullCount) { | ||
| super(); | ||
| Preconditions.checkArgument(length >= 0, "Expected length >= 0, got %s", length); | ||
|
Member
There was a problem hiding this comment. This check now throws inside I reproduced it with a stream whose Could we release public static ArrowRecordBatch deserializeRecordBatch(RecordBatch recordBatchFB, ArrowBuf body)
throws IOException {
try {
// ... existing node/buffer/compression/variadic parsing, unchanged ...
return new ArrowRecordBatch(
checkedCastToInt(recordBatchFB.length()),
nodes,
buffers,
bodyCompression,
variadicBufferCounts,
/*alignBuffers*/ true);
} finally {
body.getReferenceManager().release();
}
}This also fixes the same leak on the existing
Member
Author
There was a problem hiding this comment. Applied in 23609d6: |
||
| Preconditions.checkArgument(nullCount >= -1, "Expected nullCount >= -1, got %s", nullCount); | ||
| this.length = Math.toIntExact(length); | ||
| this.nullCount = Math.toIntExact(nullCount); | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| /* | ||
| * 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.arrow.vector.ipc.message; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class TestArrowFieldNode { | ||
|
|
||
| @Test | ||
| public void testNegativeLengthRejected() { | ||
|
Member
There was a problem hiding this comment. These tests cover the constructor, but not the deserialization path the PR is about. Could we add a test that deserializes a record batch with a negative node length and checks that nothing leaks? For example, in @Test
public void testNegativeNodeLengthDoesNotLeakBody() {
FlatBufferBuilder builder = new FlatBufferBuilder();
RecordBatch.startNodesVector(builder, 1);
FieldNode.createFieldNode(builder, -100, 0);
int nodes = builder.endVector();
RecordBatch.startBuffersVector(builder, 0);
int buffers = builder.endVector();
RecordBatch.startRecordBatch(builder);
RecordBatch.addLength(builder, 0);
RecordBatch.addNodes(builder, nodes);
RecordBatch.addBuffers(builder, buffers);
builder.finish(RecordBatch.endRecordBatch(builder));
RecordBatch recordBatchFB = RecordBatch.getRootAsRecordBatch(builder.dataBuffer());
try (BufferAllocator allocator = new RootAllocator()) {
ArrowBuf body = allocator.buffer(8);
assertThrows(
IllegalArgumentException.class,
() -> MessageSerializer.deserializeRecordBatch(recordBatchFB, body));
assertEquals(0, allocator.getAllocatedMemory());
}
}It needs three imports:
Member
Author
There was a problem hiding this comment. Added |
||
| IllegalArgumentException e = | ||
| assertThrows(IllegalArgumentException.class, () -> new ArrowFieldNode(-100, 0)); | ||
| assertEquals("Expected length >= 0, got -100", e.getMessage()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testLargeNegativeLengthRejected() { | ||
| assertThrows(IllegalArgumentException.class, () -> new ArrowFieldNode(-5_000_000_000L, 0)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testLengthAboveIntRangeRejected() { | ||
| assertThrows(ArithmeticException.class, () -> new ArrowFieldNode((1L << 32) - 100, 0)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testZeroAndPositiveLengthAccepted() { | ||
| assertEquals(0, new ArrowFieldNode(0, 0).getLength()); | ||
| assertEquals(42, new ArrowFieldNode(42, 0).getLength()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testNegativeNullCountSentinelPreserved() { | ||
| ArrowFieldNode node = new ArrowFieldNode(10, -1); | ||
| assertEquals(10, node.getLength()); | ||
| assertEquals(-1, node.getNullCount()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testNullCountBelowSentinelRejected() { | ||
| assertThrows(IllegalArgumentException.class, () -> new ArrowFieldNode(10, -2)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testNullCountAboveIntRangeRejected() { | ||
| assertThrows(ArithmeticException.class, () -> new ArrowFieldNode(10, (1L << 32) - 2)); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check runs on the
longbefore it's narrowed, but thecheckedCastToIntonly usesMath.toIntExactwhen bounds checking is on.With
arrow.enable_unsafe_memory_access=trueit's a plain(int)cast. A positivelongwhose low 32 bits are negative passes the check and is stored as a negativeint, in the same mode this PR is supposed to protect.IPC is safe because
MessageSerializer.deserializeRecordBatchrejects values outsideintrange before it calls this. C Data is not:ArrayImporterpassessnapshot.length, anint64from the producer, straight in.For example
length = 4294967196(2^32 - 100) passes the check and becomes-100.Narrowing unconditionally should fixes it. Doing the same for
nullCountstill allows the-1sentinel:The static import of
checkedCastToIntbecomes unused after this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the thread above (#1422 (comment)); addressed there.