MINOR: Bounds check delta dictionary data copy in VectorAppender - #1364

Nice improvement, this turns a raw out-of-bounds native copy into a checked one. One concern is ordering. By the time this setBytes runs, the target's data buffer has been reallocated for deltaDataSize and concatBits has already written the delta's validity bits into the target.

If the check throws, the target is left with extra validity bits while valueCount, offsets and lastSet are unchanged.

Could we validate the delta (last offset against its data buffer capacity) before any realloc or write, so a rejected delta leaves the target untouched?

Agreed. Added a validateDelta pass at the top of both visits that runs before any realloc or concatBits, so a rejected delta now leaves the target as it was. The test checks that too (see below).

Also, only the data copy is checked now. The validity copy via concatBits, the offset buffer MemoryUtil.copyMemory, and the offset-rebasing loop still read the delta without bounds checks. A delta whose offset buffer is shorter than (valueCount + 1) * 4 bytes, or whose validity buffer is shorter than valueCount bits, would still read out-of-bounds native memory.

Is it worth validating those buffer sizes up front too?

Yes, worth it. validateDelta now checks the validity buffer against getValidityBufferSizeFromCount(valueCount) and the offset buffer against (valueCount + 1) * OFFSET_WIDTH before anything reads them, so concatBits, the offset copyMemory and the rebasing loop only run on buffers that cover the value count.

deltaDataSize is derived from the delta's offsets without validation. A last offset smaller than the first gives a negative size, and a very large one can overflow the int when added to targetDataSize.
Intermediate offsets are not checked against the data size either, so later reads of the appender vector could go out of bounds even when the final offset passes.

It would be good to guard 0 <= start <= end <= dataBuffer.capacity() and use Math.addExact or similar.

Done. It walks the offsets once and requires 0 <= first, non-decreasing offsets throughout, and last <= data buffer capacity, so intermediate offsets are covered too. targetDataSize + deltaDataSize uses Math.addExact now.

The same comments applies here 😄

Same changes on the large path, with long offsets and Math.addExact(long, long).

setBytes bounds checking is gated by BoundsChecking.BOUNDS_CHECKING_ENABLED. With arrow.enable_unsafe_memory_access=true the check is skipped, no exception is thrown, and this test fails (and the fix does nothing in that mode). Consider skipping the test with an assumeTrue when bounds checking is off. Also, asserting only the exception type lets the partial-mutation issue go unnoticed. After the failure, please assert that the dictionary vector still has its original two values.

The new checks are explicit capacity/offset comparisons rather than relying on ArrowBuf bounds checking, so they apply with arrow.enable_unsafe_memory_access=true as well, and I left the test unconditional instead of adding an assumeTrue. After the failure it now asserts the dictionary still has 2 values, foo and bar, and that validity bits 2 and 3 are unset. With only the earlier setBytes change, that validity assertion fails.

This only covers the VarChar path.

Could we add a LargeVarChar case for the BaseLargeVariableWidthVector visit? A positive case with a valid delta whose data start offset is non-zero would also help, since that exercises the deltaDataStart argument to setBytes.

Added. The malformed case is parameterized over VarChar and LargeVarChar and over four offset shapes: past the data buffer, decreasing intermediate offset, last before first, and negative start. There is also a positive test for both types with offsets {3, 6, 9} over xxxbazqux, which expects foo, bar, baz, qux.

If assertThrows fails, closeableList is never closed and the allocator leaks. Consider wrapping it in a finally so the original assertion failure isn't masked by a leak error.

Fixed. The stream setup moved into a helper that closes everything in a finally, and each test only holds the reader in try-with-resources.