[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.

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?

[lidavidm]

Same here.

[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.

[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.

[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.

[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.