Use the null buffer's own offset when validating its length - #10709
Use the null buffer's own offset when validating its length#10709alliasgher wants to merge 1 commit into
Conversation
ArrayData::validate sized the null bitmap check with len_plus_offset, which folds in ArrayData::offset. That offset does not apply to the null buffer, as ArrayData::nulls documents, so a null buffer at offset 0 backing a sliced array was rejected with 'null_bit_buffer size too small'. Size the check from the null buffer's own offset and length instead. The similar check in try_new is left alone: it takes a raw Buffer for which the data offset does apply.
| let needed_len = bit_util::ceil(len_plus_offset, 8); | ||
| // ArrayData::offset does not apply to the null buffer, which carries its own offset | ||
| let needed_len = bit_util::ceil(nulls.offset() + nulls.len(), 8); | ||
| if actual_len < needed_len { |
There was a problem hiding this comment.
i wonder if we should instead remove this validation; its brought up in the original issue:
Furthermore, this validation is currently redundant. There is no way to create an invalid
BooleanBuffertoday. I don't think it's necessarily a problem to have the validation (if fixed) since there may be a way to create an invalidBooleanBufferin the future. I just mention this to point out that only the "false error" case (and not the "potentially unsafe" case) can be encountered.
and the way this fix handles it, its just validating that the NullBuffer contains enough bytes for its bits, which sounds like something NullBuffer itself should guarantee/check, and not ArrayData here. perhaps its a holdover from some old code that was refactored
2 participants
Which issue does this PR close?
Rationale for this change
ArrayData::validatesizes the null bitmap length check withlen_plus_offset, which folds inArrayData::offset:That offset does not apply to the null buffer.
ArrayData::nullssays so directly: "Note:ArrayData::offsetdoes NOT apply to the returnedNullBuffer". TheNullBuffercarries its own offset.So a null buffer at offset 0 backing an array sliced to offset 50 is rejected, using the reporter's test:
Decoupled offsets are a supported state rather than something the validator was guarding against.
arrow-data/src/ffi.rsalign_nullsexists precisely becausedata.offset() != nulls.offset()is legal: it fast-paths when they match and re-aligns the bits otherwise.As the issue notes, only the false-rejection direction is reachable today, since there is currently no way to build an invalid
BooleanBuffer. This is a correctness fix to the check, not a soundness fix.What changes are included in this PR?
One line in
ArrayData::validate, sizing the check from the null buffer's own offset and length.The near-identical check in
ArrayData::try_newis deliberately left alone. It takes a rawnull_bit_buffer: Option<Buffer>for which the data offset genuinely does apply, and it is pinned byarrow/tests/array_validation.rstest_bitmap_too_small. I confirmed that test still passes.Are these changes tested?
Yes. Added
null_buffer_offset_is_independent_of_data_offsetinarrow-data/src/data.rs, covering the reporter's scenario: 100 values sliced to the last 50, then the same 50 nulls supplied both sliced (offset 50) and unsliced (offset 0). Both must validate.I checked it is not vacuous: reverting only the one-line fix while keeping the test makes it fail with the exact error from the issue,
got 7 needed 13.Ran locally on
5ce0ebe:cargo test -p arrow-data: 44 passed, plus 13 doctestscargo test -p arrow-data -p arrow-array -p arrow-buffer -p arrow-select -p arrow-cast: all greencargo test -p arrow --test array_validation: 58 passed, includingtest_bitmap_too_smallcargo fmt -p arrow-data -- --checkandcargo clippy -p arrow-data --all-targets -- -D warnings: cleanOne pre-existing failure unrelated to this change:
util::test_util::tests::test_happyneeds thetesting/datasubmodule, which my clone did not initialize. It fails the same way on a pristine checkout.Are there any user-facing changes?
ArrayData::validateno longer rejects a valid null buffer whose offset differs from the array's. No API change. No behavior change for null buffers whose offset already matched the data offset.AI disclosure
Per
CONTRIBUTING.md"AI Generated Submissions". I used an AI assistant to help draft the fix, the test and this description. I reproduced the failure with the reporter's verbatim test before changing anything, confirmed thetry_newsite is a separate case that must not change and that its pinning test still passes, and verified the new test fails with the fix reverted. I ran every command listed above myself.