Repository navigation
MINOR: Bounds check source buffer in BaseFixedWidthVector.copyFrom - #1376
Support-itecz wants to merge 1 commit into
Conversation
| from.getDataBuffer().memoryAddress() + (long) fromIndex * typeWidth, | ||
| this.getDataBuffer().memoryAddress() + (long) thisIndex * typeWidth, | ||
| typeWidth); | ||
| from.getDataBuffer() |
There was a problem hiding this comment.
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.
| from.getDataBuffer().memoryAddress() + (long) fromIndex * typeWidth, | ||
| this.getDataBuffer().memoryAddress() + (long) thisIndex * typeWidth, | ||
| typeWidth); | ||
| from.getDataBuffer() |
There was a problem hiding this comment.
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.
| body.setZero(0, body.capacity()); | ||
| body.setInt(0, 1); | ||
| body.setInt(12, 0xBAD); | ||
| // Field node claims 4 values, but the data buffer only holds one, as a malformed IPC |
There was a problem hiding this comment.
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.
What's Changed
BaseFixedWidthVector.copyFromcopies the value with a rawMemoryUtil.copyMemoryfromfrom.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) passesisNull.copyFrom/copyFromSafe/TransferPair.copyValueSafeon an index past the buffer then read native memory beyond it, whileget()on the same index throwsIndexOutOfBoundsException. In the new test the copied value is an int sitting 8 bytes past the end of the data slice.The copy now goes through
ArrowBuf.getBytes(index, dst, dstIndex, length), which checks both buffers, the same wayBaseVariableWidthVector.copyFromalready does. Copies of valid data are unchanged.TestCopyFrom.testCopyFromFixedWidthSourceBufferTooShortfails without the change and passes with it, and thearrow-vectorandarrow-algorithmtest suites pass locally along with spotless and checkstyle.