jbonofre commented on code in PR #1291:
URL: https://github.com/apache/arrow-java/pull/1291#discussion_r3979282085
##########
vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java:
##########
Review Comment:
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?
##########
vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java:
##########
Review Comment:
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.
##########
vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java:
##########
Review Comment:
Same issue as `getDataPointer`: `dataOffset`/`bufIndex` feed
`ByteFunctionHelpers.hash(hasher, dataBuf, dataOffset, dataOffset + length)`
with no validation.
##########
vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java:
##########
Review Comment:
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.
##########
vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java:
##########
Review Comment:
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.
##########
vector/src/test/java/org/apache/arrow/vector/TestVariableWidthViewVector.java:
##########
@@ -2921,4 +2921,20 @@ public void testValidate() {
assertTrue(e.getMessage().contains("Not enough capacity for data
buffer"));
}
}
+
+ @Test
+ public void testValidateInvalidOffsets() {
Review Comment:
`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.
--
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]
