andygrove commented on PR #1247: URL: https://github.com/apache/arrow-java/pull/1247#issuecomment-6544946511
Thanks for the update, the reordering in `setBigEndian` / `setBigEndianSafe` looks right now. One more path has the same problem and I think it belongs in this fix: `setSafe(int index, long start, ArrowBuf buffer, int length)` (and the `Decimal256Vector` counterpart) never validates `length` either. The bound checks it does run don't cover it: `buffer.checkBytes(start, start + length)` only confirms the *source* range exists, and `valueBuffer.checkBytes(index * TYPE_WIDTH, (index + 1) * TYPE_WIDTH)` checks the slot, not the copy. So with `length = 24` both checks pass and `MemoryUtil.copyMemory(inAddress, outAddress, length)` writes 8 bytes into the next slot, and that happens even with bounds checking enabled, not just under `arrow.enable_unsafe_memory_access`. Related, `length == 0` on the little-endian branch reads `MemoryUtil.getByte(inAddress + length - 1)`, i.e. one byte *before* `start`, to pick the sign-extension pad. The `byte[]` variant treats an empty array as zero, so the `ArrowBuf` variants probably should too rather than reading outside the value. Suggest adding the same up-front `length > TYPE_WIDTH` guard (and a `length == 0` early-out) to `setSafe(int, long, ArrowBuf, int)` in both vectors, with a test mirroring `setBigEndianSafeOversizedHasNoSideEffects`. Then the three public write paths that bypass `ArrowBuf` bounds checking are all covered by one PR. I have a draft covering these in #1418; happy to either drop that in favour of this PR if you fold them in here, or rebase it on top of this once it merges, whichever you prefer. -- 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]
