jbonofre commented on code in PR #1376:
URL: https://github.com/apache/arrow-java/pull/1376#discussion_r4323444999
##########
vector/src/main/java/org/apache/arrow/vector/BaseFixedWidthVector.java:
##########
@@ -833,10 +832,12 @@ public void copyFrom(int fromIndex, int thisIndex,
ValueVector from) {
BitVectorHelper.unsetBit(this.getValidityBuffer(), thisIndex);
} else {
BitVectorHelper.setBit(this.getValidityBuffer(), thisIndex);
- MemoryUtil.copyMemory(
- from.getDataBuffer().memoryAddress() + (long) fromIndex * typeWidth,
- this.getDataBuffer().memoryAddress() + (long) thisIndex * typeWidth,
- typeWidth);
+ from.getDataBuffer()
Review 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.
##########
vector/src/test/java/org/apache/arrow/vector/TestCopyFrom.java:
##########
@@ -1098,4 +1102,25 @@ public void testCopySafeArrow7837() {
assertEquals("1234567890",
Objects.requireNonNull(vc2.getObject(5)).toString());
}
}
+
+ @Test
+ public void testCopyFromFixedWidthSourceBufferTooShort() {
+ try (IntVector source = new IntVector("source", allocator);
+ IntVector target = new IntVector("target", allocator);
+ ArrowBuf body = allocator.buffer(64)) {
+ 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
Review 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.
##########
vector/src/main/java/org/apache/arrow/vector/BaseFixedWidthVector.java:
##########
@@ -833,10 +832,12 @@ public void copyFrom(int fromIndex, int thisIndex,
ValueVector from) {
BitVectorHelper.unsetBit(this.getValidityBuffer(), thisIndex);
} else {
BitVectorHelper.setBit(this.getValidityBuffer(), thisIndex);
- MemoryUtil.copyMemory(
- from.getDataBuffer().memoryAddress() + (long) fromIndex * typeWidth,
- this.getDataBuffer().memoryAddress() + (long) thisIndex * typeWidth,
- typeWidth);
+ from.getDataBuffer()
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
