Use the null buffer's own offset when validating its length - #10709
Open
alliasgher wants to merge 1 commit into
Open
Use the null buffer's own offset when validating its length#10709alliasgher wants to merge 1 commit into
alliasgher 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #7379.
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.