Repository navigation
GH-1421: reject negative FieldNode length during deserialization #1422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
ad2f9b9
e593bb3
23609d6
6f2c26e
d5bc951
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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 static org.apache.arrow.memory.util.LargeMemoryUtil.checkedCastToInt; | ||
|
|
||
| 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. | ||
| * @param nullCount The number of null values, or -1 if unknown. | ||
| */ | ||
| public ArrowFieldNode(long length, long nullCount) { | ||
| super(); | ||
| this.length = checkedCastToInt(length); | ||
| this.nullCount = checkedCastToInt(nullCount); | ||
| Preconditions.checkArgument(length >= 0, "Expected length >= 0, got %s", length); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Applied in 23609d6: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The PR description says this also fixes the C Data path. C Data imports reach this check through ArrayImporter.java:131, which calls new ArrowFieldNode(snapshot.length, snapshot.null_count). But no test covers that path. Could you add a test in the c module that imports an array with a length of -1 and another with a length larger than Integer.MAX_VALUE?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in |
||
| 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 | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -411,50 +411,58 @@ 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 (node.length() < 0) { | ||||||||||||||||||||||||
| throw new IOException("Record batch node " + i + " has negative length " + node.length()); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| if (node.nullCount() < -1) { | ||||||||||||||||||||||||
| throw new IOException( | ||||||||||||||||||||||||
| "Record batch node " + i + " has invalid null count " + node.nullCount()); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| if ((int) node.length() != node.length() || (int) node.nullCount() != node.nullCount()) { | ||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This catches lengths that are too large but not negative lengths.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in 6f2c26e. The |
||||||||||||||||||||||||
| throw new IOException( | ||||||||||||||||||||||||
| "Cannot currently deserialize record batches with " | ||||||||||||||||||||||||
| + "node length larger than INT_MAX records."); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| nodes.add(new ArrowFieldNode(node.length(), node.nullCount())); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| List<ArrowBuf> buffers = new ArrayList<>(); | ||||||||||||||||||||||||
| for (int i = 0; i < recordBatchFB.buffersLength(); ++i) { | ||||||||||||||||||||||||
| Buffer bufferFB = recordBatchFB.buffers(i); | ||||||||||||||||||||||||
| ArrowBuf vectorBuffer = body.slice(bufferFB.offset(), bufferFB.length()); | ||||||||||||||||||||||||
| buffers.add(vectorBuffer); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| nodes.add(new ArrowFieldNode(node.length(), node.nullCount())); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| List<ArrowBuf> buffers = new ArrayList<>(); | ||||||||||||||||||||||||
| for (int i = 0; i < recordBatchFB.buffersLength(); ++i) { | ||||||||||||||||||||||||
| Buffer bufferFB = recordBatchFB.buffers(i); | ||||||||||||||||||||||||
| ArrowBuf vectorBuffer = body.slice(bufferFB.offset(), bufferFB.length()); | ||||||||||||||||||||||||
| buffers.add(vectorBuffer); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| ArrowBodyCompression bodyCompression = | ||||||||||||||||||||||||
| recordBatchFB.compression() == null | ||||||||||||||||||||||||
| ? NoCompressionCodec.DEFAULT_BODY_COMPRESSION | ||||||||||||||||||||||||
| : new ArrowBodyCompression( | ||||||||||||||||||||||||
| recordBatchFB.compression().codec(), recordBatchFB.compression().method()); | ||||||||||||||||||||||||
| ArrowBodyCompression bodyCompression = | ||||||||||||||||||||||||
| recordBatchFB.compression() == null | ||||||||||||||||||||||||
| ? NoCompressionCodec.DEFAULT_BODY_COMPRESSION | ||||||||||||||||||||||||
| : new ArrowBodyCompression( | ||||||||||||||||||||||||
| recordBatchFB.compression().codec(), recordBatchFB.compression().method()); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| List<Long> variadicBufferCounts = new ArrayList<>(); | ||||||||||||||||||||||||
| for (int i = 0; i < recordBatchFB.variadicBufferCountsLength(); i++) { | ||||||||||||||||||||||||
| variadicBufferCounts.add(recordBatchFB.variadicBufferCounts(i)); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| List<Long> variadicBufferCounts = new ArrayList<>(); | ||||||||||||||||||||||||
| for (int i = 0; i < recordBatchFB.variadicBufferCountsLength(); i++) { | ||||||||||||||||||||||||
| variadicBufferCounts.add(recordBatchFB.variadicBufferCounts(i)); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if ((int) recordBatchFB.length() != recordBatchFB.length()) { | ||||||||||||||||||||||||
| throw new IOException( | ||||||||||||||||||||||||
| "Cannot currently deserialize record batches with more than INT_MAX records."); | ||||||||||||||||||||||||
| if ((int) recordBatchFB.length() != recordBatchFB.length()) { | ||||||||||||||||||||||||
| throw new IOException( | ||||||||||||||||||||||||
| "Cannot currently deserialize record batches with more than INT_MAX records."); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+452
to
+455
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The node checks above reject negative lengths, but a negative Could we reject it here too?
Suggested change
|
||||||||||||||||||||||||
| return new ArrowRecordBatch( | ||||||||||||||||||||||||
| checkedCastToInt(recordBatchFB.length()), | ||||||||||||||||||||||||
| nodes, | ||||||||||||||||||||||||
| buffers, | ||||||||||||||||||||||||
| bodyCompression, | ||||||||||||||||||||||||
| variadicBufferCounts, | ||||||||||||||||||||||||
| /*alignBuffers*/ true); | ||||||||||||||||||||||||
| } finally { | ||||||||||||||||||||||||
| body.getReferenceManager().release(); | ||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| ArrowRecordBatch arrowRecordBatch = | ||||||||||||||||||||||||
| new ArrowRecordBatch( | ||||||||||||||||||||||||
| checkedCastToInt(recordBatchFB.length()), | ||||||||||||||||||||||||
| nodes, | ||||||||||||||||||||||||
| buffers, | ||||||||||||||||||||||||
| bodyCompression, | ||||||||||||||||||||||||
| variadicBufferCounts, | ||||||||||||||||||||||||
| /*alignBuffers*/ true); | ||||||||||||||||||||||||
| body.getReferenceManager().release(); | ||||||||||||||||||||||||
| return arrowRecordBatch; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
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.