[Support-itecz]

What's Changed

BaseFixedWidthVector.copyFrom copies the value with a raw MemoryUtil.copyMemory from from.getDataBuffer().memoryAddress() + fromIndex * typeWidth, so neither the source nor the target buffer is bounds checked, even with bounds checking enabled. A fixed-width vector loaded from an IPC record batch whose field node length is larger than its data buffer (for example 4 int values over a 4-byte buffer) passes isNull. copyFrom / copyFromSafe / TransferPair.copyValueSafe on an index past the buffer then read native memory beyond it, while get() on the same index throws IndexOutOfBoundsException. In the new test the copied value is an int sitting 12 bytes past the start of the data slice.

The copy now goes through ArrowBuf.getBytes(index, dst, dstIndex, length), which checks both buffers, the same way BaseVariableWidthVector.copyFrom already does. Copies of valid data are unchanged.

VectorAppender and BitVectorHelper.concatBits had the same pattern on the delta vector's buffers, and the delta can come straight from loadFieldBuffers. VectorAppender now checks the delta's validity and data buffers (fixed-width, BitVector, view) and type buffers (union, dense union) with checkCapacity before the target is reallocated or written, the same way validateDelta from #1364 does for the variable-width paths, so a rejected delta leaves the target unchanged. concatBits also checkBytes both inputs. The offset buffer copies were already covered by the getInt/getLong of the last offset just above them. DecimalVector / Decimal256Vector setSafe and setBigEndianSafe already checkBytes both buffers before their raw copies, so they're left as is.

The copyFrom and concatBits checks only apply with bounds checking on. With arrow.enable_unsafe_memory_access=true they're skipped, so a malformed batch reads out of bounds again, which is what that flag opts into. The VectorAppender capacity checks run either way, like the existing validateDelta.

Float8Benchmarks.copyFromBenchmark (1024 copyFrom calls per op, JDK 26, 2 forks x 8 iterations, laptop so the error bars are wide):

	bounds checking on	enable_unsafe_memory_access=true
main	13.98 ± 6.03 us/op	8.51 ± 4.87 us/op
this PR	19.89 ± 5.18 us/op	8.79 ± 4.83 us/op

So roughly 6 ns per copied value with checks on and no measurable change with them off. The appender and concatBits checks are one range check per buffer per append, not per value.

TestCopyFrom.testCopyFromFixedWidthSourceBufferTooShort and the new TestVectorAppender.testAppendFixedWidthVectorSourceBufferTooShort / testAppendValidityBufferTooShort fail without the change and pass with it; the appender tests also assert the target's value count, capacity and values are unchanged after the rejected append. The arrow-vector and arrow-algorithm test suites pass locally along with spotless and checkstyle.

[jbonofre]

This hardens only BaseFixedWidthVector.copyFrom. The same unchecked pattern, MemoryUtil.copyMemory on raw memoryAddress() plus an offset, remains in other places in vector/src/main. These include DecimalVector, Decimal256Vector, BitVectorHelper, and VectorAppender`.

If the goal is to make malformed IPC input fail with IndexOutOfBoundsException instead of an out-of-bounds read, can we say which of these are in scope?

If they are reachable from loadFieldBuffers style input, I would fix them here.

[Support-itecz]

VectorAppender and BitVectorHelper.concatBits are reachable: the delta vector can be one loaded via loadFieldBuffers, and nothing checks its data, view, type or validity buffer against its value count before the raw copy. I added a source checkBytes before each of those copies and both concatBits inputs, plus two appender tests (short data buffer, short validity buffer) that fail without the change. The offset copies were already bounded by the getInt/getLong of the last offset right above them. DecimalVector/Decimal256Vector setSafe and setBigEndianSafe already checkBytes both buffers before copying, so I left those alone. The description now lists what is covered.

[jbonofre]

Oh, also, copyFrom is called once per element, and copyFromSafe loops and appenders call it in bulk. ArrowBuf.getBytes(long, ArrowBuf, long, int) adds bounds checks on both buffers plus argument validation, in place of a single raw copy.

Do you have a before/after number, for instance the existing JMH benchmarks for copyFrom? I would like to see the cost here.

Also, this only protects when bounds checking is on. With arrow.enable_unsafe_memory_access=true, getBytes skips validation and the read is unchecked again. That is probably acceptable, since the flag disables checks by design, but please say so in the description.

[Support-itecz]

Ran Float8Benchmarks.copyFromBenchmark (1024 copies per op, 2 forks x 8 iterations on a laptop, so the error bars are wide):

• checks on: main 13.98 ± 6.03 us/op, this PR 19.89 ± 5.18 us/op
• enable_unsafe_memory_access=true: main 8.51 ± 4.87, this PR 8.79 ± 4.83

So about 6 ns per value with checks on and nothing measurable with them off. I also tried checkBytes on both buffers plus the original raw copy instead of getBytes, and it came out the same (20.38 ± 11.60), so I kept getBytes. The appender changes are one range check per buffer per append, not per value.

Agreed on the unsafe flag. With it set the source check is skipped and the read is unchecked again. That is in the description now.

[jbonofre]

The source is loaded with a zero-length validity buffer (body.slice(0, 0)) and a null count of 0. The first copyFromSafe(0, 0, source) then needs isSet(0) to read that buffer without throwing.

Is that guaranteed, or does it work only because of how isSet handles this case today?
If a later change makes isSet check the buffer length, this test fails for a reason unrelated to the data-buffer bounds check it's meant to cover.

Giving the source a real validity buffer (all bits set) would make the test say what it means.

[Support-itecz]

Good catch. Today it works because loadValidityBuffer replaces an empty validity buffer with an all-ones one when the null count is 0, not because of isSet, but the test should not rely on that. The source now gets a real 8-byte validity buffer with all 4 bits set, and the new appender tests do the same.

[jbonofre]

This check runs after the target has already been reallocated to newValueCount and after concatBits has copied the delta validity bits into the target. So a malformed delta (e.g. an IPC delta dictionary whose field node claims ~2^30 values over a 4-byte data buffer) can force a multi GB reallocation of the target before it is rejected, and if that allocation succeeds the target is left with stray validity bits past its value count.

#1364 uses the opposite order for the variable width paths: validateDelta runs before the target is touched, so a rejected delta leaves the target unchanged.

Could we do the same here, right after the getValueCount() == 0 early return? Something like:

int deltaCount = deltaVector.getValueCount();
checkCapacity(
    deltaVector.getValidityBuffer(),
    BitVectorHelper.getValidityBufferSizeFromCount(deltaCount),
    "validity");
long dataSize =
    deltaVector instanceof BitVector
        ? BitVectorHelper.getValidityBufferSizeFromCount(deltaCount)
        : (long) deltaVector.getTypeWidth() * deltaCount;
checkCapacity(deltaVector.getDataBuffer(), dataSize, "data");

The union (line 682) and dense union (line 748) type buffer checks have the same problem: they run after reAlloc(). They should move above the capacity loop too, e.g. checkCapacity(deltaVector.getTypeBuffer(), (long) deltaVector.getValueCount() * UnionVector.TYPE_WIDTH, "type").

[Support-itecz]

Agreed, the target shouldn't be reallocated or have bits written for a delta we then reject. Moved the validity and data checks to right after the getValueCount() == 0 return using checkCapacity, with the BitVector case sized from the validity size as you suggested, and dropped the late checkBytes. Union and dense union now check the type buffer (valueCount * TYPE_WIDTH) before the capacity loop too. The two appender tests also assert the target's value count, capacity and values are unchanged after the throw. Pushed in 67b5780.

[jbonofre]

By the time this check runs, the delta variadic data buffers have already been retained and added to the target dataBuffers, and the validity bits have been appended. If it throws, the target keeps those buffers: they stay pinned until the target is closed, they are written out as extra variadic buffers on the next IPC write of the target, and every retry adds more.

Can we move this check (plus a validity check) above the capacity loop, before the retain()/addAll(), using checkCapacity so a rejected delta leaves the target untouched:

checkCapacity(
    deltaVector.getDataBuffer(),
    (long) BaseVariableWidthViewVector.ELEMENT_SIZE * deltaVector.getValueCount(),
    "view");

[Support-itecz]

Right, a throw there left retained buffers in the target's dataBuffers. The view and validity checks now run with checkCapacity before the capacity loop, so nothing is retained or added unless the delta passes. Same commit.