Skip to content

MINOR: Bounds check source buffer in BaseFixedWidthVector.copyFrom #1376

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Support-itecz wants to merge 1 commit into
base: main
Choose a base branch
from
Open

MINOR: Bounds check source buffer in BaseFixedWidthVector.copyFrom #1376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions vector/src/main/java/org/apache/arrow/vector/BaseFixedWidthVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import org.apache.arrow.memory.BufferAllocator;
import org.apache.arrow.memory.util.ArrowBufPointer;
import org.apache.arrow.memory.util.ByteFunctionHelpers;
import org.apache.arrow.memory.util.MemoryUtil;
import org.apache.arrow.memory.util.hash.ArrowBufHasher;
import org.apache.arrow.util.Preconditions;
import org.apache.arrow.vector.compare.VectorVisitor;
Expand Down Expand Up @@ -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()

@jbonofre jbonofre Oct 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jbonofre jbonofre Oct 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.getBytes(
(long) fromIndex * typeWidth,
this.getDataBuffer(),
(long) thisIndex * typeWidth,
typeWidth);
}
}

Expand Down
25 changes: 25 additions & 0 deletions vector/src/test/java/org/apache/arrow/vector/TestCopyFrom.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,19 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.math.BigDecimal;
import java.nio.charset.StandardCharsets;
import java.time.Duration;
import java.time.Period;
import java.util.Arrays;
import java.util.Objects;
import org.apache.arrow.memory.ArrowBuf;
import org.apache.arrow.memory.BufferAllocator;
import org.apache.arrow.memory.RootAllocator;
import org.apache.arrow.vector.ipc.message.ArrowFieldNode;
import org.apache.arrow.vector.types.Types.MinorType;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
Expand Down Expand Up @@ -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

@jbonofre jbonofre Oct 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// record batch can.
ArrowBuf validity = body.slice(0, 0);
ArrowBuf data = body.slice(0, IntVector.TYPE_WIDTH);
source.loadFieldBuffers(new ArrowFieldNode(4, 0), Arrays.asList(validity, data));

target.allocateNew(4);
target.copyFromSafe(0, 0, source);
assertEquals(1, target.get(0));
assertThrows(IndexOutOfBoundsException.class, () -> target.copyFromSafe(3, 1, source));
}
}
}
Loading