[andygrove]

What's Changed

ArrowFieldNode previously accepted a negative length. Because the IPC
(MessageSerializer.deserializeRecordBatch), JSON (JsonFileReader), and C Data
(ArrayImporter) deserialization paths construct ArrowFieldNode directly from
externally supplied metadata, a malformed or hostile input could set a negative node
length that then flowed into buffer-size math and buffer writes:

• BitVectorHelper.getValidityBufferSizeFromCount uses an unsigned shift, so a small
  negative length becomes a ~512 MB allocation request.
• BitVectorHelper.loadValidityBuffer then calls ArrowBuf.setOne(0, length / 8) with a
  negative byte count. With bounds checking disabled
  (arrow.enable_unsafe_memory_access=true) this reaches Unsafe.setMemory with the
  negative value reinterpreted as a huge unsigned length (memory corruption); with bounds
  checking enabled it throws only after a large buffer has been allocated and the vector's
  previous validity buffer already released.

This PR:

• Rejects a negative length, and a nullCount below the -1 "unknown" sentinel, in the
  ArrowFieldNode constructor.
• Narrows both values with Math.toIntExact even when bounds checking is disabled. In that
  mode checkedCastToInt is a plain cast, so a C Data length such as 2^32 - 100 passed the
  check and was stored as -100.
• Rejects a negative count in JsonFileReader as soon as it is read, for column and batch
  counts. A column's count sizes its buffers before the ArrowFieldNode is built, so the
  constructor check alone does not cover the JSON path.

Closes #1421.

[lidavidm]

I don't know if this comment really helps; at the very least it should be significantly trimmed. Also we could validate nullCount >= -1 at least?

[andygrove]

Removed the comment. The -1 sentinel is now documented on the nullCount param, and the constructor rejects nullCount < -1 (e593bb3).

[lidavidm]

Same here.

[andygrove]

Removed the test comments too.

[jbonofre]

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:

Suggested change

	Preconditions.checkArgument(length >= 0, "Expected length >= 0, got %s", length);
	this.length = checkedCastToInt(length);
	this.nullCount = checkedCastToInt(nullCount);
	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.

[andygrove]

Applied in e593bb3: both values are narrowed with Math.toIntExact, and the checkedCastToInt import is gone. Added testLengthAboveIntRangeRejected and testNullCountAboveIntRangeRejected. As you said, they pass on the old code in the default run; with ARROW_ENABLE_UNSAFE_MEMORY_ACCESS=true they fail on the old code and pass now.

[jbonofre]

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?

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.

[andygrove]

Added a readCount() helper in e593bb3 that rejects a negative count as soon as it's read, for the column count and both batch counts (read() and read(VectorSchemaRoot), which dictionary batches also use). The comment claiming JSON coverage is gone. TestJSONFile has a test per site; the column-count one reads through a 1 MB allocator and failed on the old code with OutOfMemoryException on the 536,870,900-byte validity buffer.

[jbonofre]

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:

Suggested change

	Preconditions.checkArgument(length >= 0, "Expected length >= 0, got %s", length);
	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.

[andygrove]

Same as the thread above (#1422 (comment)); addressed there.

[jbonofre]

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?

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.

[andygrove]

Same as the thread above (#1422 (comment)); addressed there.

[jbonofre]

Thanks @andygrove, let me do a new pass 😄

[jbonofre]

This check now throws inside MessageSerializer.deserializeRecordBatch(RecordBath, ArrowBuf), before that method releases body at the end. A negative node length now leaks the body buffer. Before this change the same input loaded without error and didn't leak.

I reproduced it with a stream whose FieldNode.length is patched to -100. ArrowStreamReader and ArrowFileReader both throw and leave 8192 bytes allocated, and allocator.close() fails with "Memory was leaked". Flight goes through the same call: ArrowMessage.asRecordBatch() retains underlying and relies on deserializeRecordBatch to release it.

Could we release body in a finally? ArrowRecordBatch retains the slices it keeps, so the release is correct on both paths:

  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 IOException paths in that method (for example node length > INT_MAX), and covers the dictionary batch and Flight callers. ArrowMessage doesn't need a change. Releasing there too would release the buffer twice.

[jbonofre]

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 MessageSerializerTest:

  @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: com.google.flatbuffers.FlatBufferBuilder, org.apache.arrow.flatbuf.FieldNode and org.apache.arrow.flatbuf.RecordBatch.