GH-1217: Bounds check before alloc in var-width view vectors

[lidavidm]

Also, document that when enable_unsafe_memory_access is enabled, all bets are off.

Reported by n0mi1k.

[jbonofre]

I'm a bit surprised as #1217 names this method as affected, but it isn't touched by this PR.

buffer.set(dataBuf, dataOffset, dataLength) -> ReusableByteArray.set() still does bytes = new byte[len] before the source getBytes() bounds check runs. The same allocate-before-check pattern just fixed two methods above in getData(int).

Should we include the same change here?

[lidavidm]

Hmm, adding manual checks everywhere is going to be fairly brittle...

[lidavidm]

I consolidated all of these methods to use a single accessor that always checks the buffer size.

[jbonofre]

Same here: it's mentioned in #1217 but not changed in this PR.

allocateOrGetLastDataBuffer(viewLength) allocates a buffer sized by viewLength before the bounds-checked thisDataBuf.setBytes(..., dataBuf, dataOffset, viewLength) ever runs. A corrupted source view (let's say copyFrom/copyFromSafe on data loaded from an untrusted IPC stream) drives the same large-allocation DoS this PR fixes in getData, through the copy path instead.

[jbonofre]

Named in #1217 😄

target.allocateOrGetLastDataBuffer(stringLength) allocated before currentDataBuf.setBytes(currentOffset, dataBuf, readBufOffset, stringLength) bounds-checks the source read, and stringLength (from getValueLength(i)) is never validated first.

This runs on every split/slice/transfer of a View vector, so it's a fairly reachable path for the same bug class this PR addresses elsewhere.

[jbonofre]

Named in #1217 😄

No array allocation here, but bufIndex/dataOffset from the view record are used to build an ArrowBufPointer with zero validation. Under arrow.enable_unsafe_memory_access=true, this is the "reads arbitrary native heap" case, not just a DoS.

[jbonofre]

Same issue as getDataPointer: dataOffset/bufIndex feed ByteFunctionHelpers.hash(hasher, dataBuf, dataOffset, dataOffset + length) with no validation.

[jbonofre]

ArrowBuf.setInt(index, value) takes a byte offset, not a field-slot index. The view record layout is length@0, prefix@4, bufferIndex@8, dataOffset@12 (4 bytes each), but setInt(1, 0), setInt(2, 0), setInt(3, 1024) write to byte offsets 1/2/3, overlapping the length field instead of landing on prefix/bufferIndex/dataOffset.

I just tested it: length ends up 64 (unchanged), prefix=4 (collateral overlap), bufferIndex=0, dataOffset=0 (not the intended 1024).

The test passes, but I believe because dataLength=64 exceeds the tiny data buffer's capacity, not because of an out-of-range dataOffset like the test name implies.

I suggest to use setInt(12, 1024) to actually cover the corrupted-offset scenario.

[Copilot]

🟡 Changes recommended

The split-and-transfer path can still allocate from an unvalidated length.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds eager bounds checks for variable-width view vectors to prevent oversized allocations from corrupt offsets.

Changes:

• Centralizes checked view-data access.
• Adds regression coverage for invalid offsets.
• Documents risks of disabling bounds checks.

File summaries

File	Description
BaseVariableWidthViewVector.java	Adds checked data access and updates callers.
TestVariableWidthViewVector.java	Tests invalid view offsets.
ArrowBuf.java	Adds checked array-copy helper.
BoundsChecking.java	Documents unsafe-access risks.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.