jbonofre commented on code in PR #1422:
URL: https://github.com/apache/arrow-java/pull/1422#discussion_r4434464977
##########
vector/src/main/java/org/apache/arrow/vector/ipc/message/ArrowFieldNode.java:
##########
@@ -35,6 +36,13 @@ public class ArrowFieldNode implements FBSerializable {
*/
public ArrowFieldNode(long length, long nullCount) {
super();
+ // A negative length is never valid and, if accepted, flows unchecked into
buffer size
+ // computations (BitVectorHelper.getValidityBufferSizeFromCount performs
an unsigned shift)
+ // and into ArrowBuf.setOne/setZero, which can corrupt memory when bounds
checking is
+ // disabled. Reject it at this single chokepoint shared by every
deserialization path
+ // (IPC, JSON, C Data). nullCount is intentionally not checked here:
VectorUnloader uses -1
+ // as a sentinel meaning "null count not included".
+ Preconditions.checkArgument(length >= 0, "Expected length >= 0, got %s",
length);
this.length = checkedCastToInt(length);
this.nullCount = checkedCastToInt(nullCount);
Review Comment:
This check runs on the `long` before it's narrowed, but `checkedCastToInt`
only uses `Math.toIntExact` when bounds checking is on. With
`arrow.enable_unsafe_memory_access=true` it's a plain `(int)` cast. A positive
`long` whose low 32 bits are negative passes the check and is stored as a
negative `int`, in the same mode this PR is meant to protect.
IPC is safe because `MessageSerializer.deserializeRecordBatch` rejects
values outside `int` range before it calls this. C Data isn't: `ArrayImporter`
passes `snapshot.length`, an `int64` from the producer, straight in. For
example, `length = 4294967196` (2^32 - 100) passes the check and becomes `-100`.
Narrowing unconditionally fixes it. Doing the same for `nullCount` still
allows the `-1` sentinel:
```suggestion
Preconditions.checkArgument(length >= 0, "Expected length >= 0, got %s",
length);
this.length = Math.toIntExact(length);
this.nullCount = Math.toIntExact(nullCount);
```
The static import of `checkedCastToInt` (L19) becomes unused after this. A
test with `new ArrowFieldNode((1L << 32) - 100, 0)` would help too. It won't
fail without the fix in the default test run, which has bounds checking on, but
it pins the expected behavior.
##########
vector/src/main/java/org/apache/arrow/vector/ipc/message/ArrowFieldNode.java:
##########
@@ -35,6 +36,13 @@ public class ArrowFieldNode implements FBSerializable {
*/
public ArrowFieldNode(long length, long nullCount) {
super();
+ // A negative length is never valid and, if accepted, flows unchecked into
buffer size
+ // computations (BitVectorHelper.getValidityBufferSizeFromCount performs
an unsigned shift)
+ // and into ArrowBuf.setOne/setZero, which can corrupt memory when bounds
checking is
+ // disabled. Reject it at this single chokepoint shared by every
deserialization path
+ // (IPC, JSON, C Data). nullCount is intentionally not checked here:
VectorUnloader uses -1
+ // as a sentinel meaning "null count not included".
+ Preconditions.checkArgument(length >= 0, "Expected length >= 0, got %s",
length);
Review Comment:
This check runs on the `long` before it's narrowed, but the
`checkedCastToInt` only uses `Math.toIntExact` when bounds checking is on.
With `arrow.enable_unsafe_memory_access=true` it's a plain `(int)` cast. A
positive `long` whose low 32 bits are negative passes the check and is stored
as a negative `int`, in the same mode this PR is supposed to protect.
IPC is safe because `MessageSerializer.deserializeRecordBatch` rejects
values outside `int` range before it calls this. C Data is not: `ArrayImporter`
passes `snapshot.length`, an `int64` from 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 `nullCount`
still allows the `-1` sentinel:
```suggestion
Preconditions.checkArgument(length >= 0, "Expected length >= 0, got %s",
length);
this.length = Math.toIntExact(length);
this.nullCount = Math.toIntExact(nullCount);
```
The static import of `checkedCastToInt` becomes unused after this.
##########
vector/src/main/java/org/apache/arrow/vector/ipc/message/ArrowFieldNode.java:
##########
@@ -35,6 +36,13 @@ public class ArrowFieldNode implements FBSerializable {
*/
public ArrowFieldNode(long length, long nullCount) {
super();
+ // A negative length is never valid and, if accepted, flows unchecked into
buffer size
+ // computations (BitVectorHelper.getValidityBufferSizeFromCount performs
an unsigned shift)
+ // and into ArrowBuf.setOne/setZero, which can corrupt memory when bounds
checking is
+ // disabled. Reject it at this single chokepoint shared by every
deserialization path
+ // (IPC, JSON, C Data). nullCount is intentionally not checked here:
VectorUnloader uses -1
Review Comment:
The JSON path is not covered here, because
`JsonFileReader.readFromJsonIntVector` uses `count` before it builds the
`ArrowFieldNote`:
- `vector.setInitialCapacity(valueCount)`
- `readIntBuffer(...)` -> `BIT` reader ->
`getValidityBufferSizeFromCount(-100)`. That is `(-93) >>> 3`, about 512 MB,
which is then allocated and fully zeroed with `setZero`
- `BitVectorHelper.getNullCount` then scans that buffer
Only after all that does `new ArrowFieldNode(valueCount, nullCount)` throw.
The release loop at the end of the method is skipped, so `vectorBuffers` also
leaks. A JSON file with `"count": -100` still triggers the large allocation,
and leaks it.
Could we check it right after it's read?
```java
int valueCount = readNextField("count", Integer.class);
if (valueCount < 0) {
throw new IllegalArgumentException("Expected count >= 0, got " +
valueCount);
}
```
The batch level `count` at lines 199 and 227 goes straight into
`setRowCount` with the same problem. It would also be good to reword this
comment so it doesn't say JSON is covered.
##########
vector/src/main/java/org/apache/arrow/vector/ipc/message/ArrowFieldNode.java:
##########
@@ -35,6 +36,13 @@ public class ArrowFieldNode implements FBSerializable {
*/
public ArrowFieldNode(long length, long nullCount) {
super();
+ // A negative length is never valid and, if accepted, flows unchecked into
buffer size
+ // computations (BitVectorHelper.getValidityBufferSizeFromCount performs
an unsigned shift)
+ // and into ArrowBuf.setOne/setZero, which can corrupt memory when bounds
checking is
+ // disabled. Reject it at this single chokepoint shared by every
deserialization path
+ // (IPC, JSON, C Data). nullCount is intentionally not checked here:
VectorUnloader uses -1
Review Comment:
The JSON path isn't covered here, because
`JsonFileReader.readFromJsonIntoVector` uses `count` before it builds the
`ArrowFieldNode`:
- `vector.setInitialCapacity(valueCount)` (L890)
- `readIntoBuffer(...)` → `BIT` reader →
`getValidityBufferSizeFromCount(-100)`. That is `(-93) >>> 3`, about 512 MB,
which is then allocated and fully zeroed with `setZero`
- `BitVectorHelper.getNullCount` then scans that buffer
Only after all that does `new ArrowFieldNode(valueCount, nullCount)` (L921)
throw. The release loop at the end of the method is skipped, so `vectorBuffers`
also leaks. A JSON file with `"count": -100` still triggers the large
allocation, and leaks it.
Could you check it right after it's read?
```java
int valueCount = readNextField("count", Integer.class);
if (valueCount < 0) {
throw new IllegalArgumentException("Expected count >= 0, got " +
valueCount);
}
```
The batch-level `count` at L199 and L227 goes straight into `setRowCount`
with the same problem. It would also be good to reword this comment so it
doesn't say JSON is covered.
--
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]
