GH-50762: [C++][Feather] Avoid reading outside sliced bitmap buffers - #50763
Open
tcmartin wants to merge 1 commit into
Open
GH-50762: [C++][Feather] Avoid reading outside sliced bitmap buffers#50763tcmartin wants to merge 1 commit into
tcmartin wants to merge 1 commit into
Conversation
Track logical bit lengths separately from byte lengths when shifting sliced bitmaps, stop before the first byte outside the declared slice, and clear unused output bits. Add an exact-sized-buffer regression test that checks both serialized output and round-trip behavior. Constraint: Preserve the streaming writer and Feather V1 byte layout. Rejected: Copy the entire bitmap into a temporary allocation | adds allocation and abandons the existing bounded streaming design. Confidence: high Scope-risk: narrow Directive: Keep bitmap length in bits; byte length alone cannot determine whether the final shifted source byte is valid. Tested: ASan/UBSan standalone control and trigger; full arrow-feather-test suite (84 passed, 5 expected skips). Not-tested: Full Arrow repository test suite.
|
|
taepper
reviewed
Jul 31, 2026
Comment on lines
+95
to
111
| for (int64_t i = 0; i < length; ++i) { | ||
| uint8_t r = static_cast<uint8_t>(*data++ >> bit_shift); | ||
| uint8_t l = static_cast<uint8_t>(*data << lshift); | ||
| uint8_t l = | ||
| data == data_end ? 0 : static_cast<uint8_t>(*data << lshift); | ||
| uint8_t value = l | r; | ||
| if (i == length - 1 && bit_length % 8 != 0) { | ||
| value &= bit_util::LeastSignificantBitMask<uint8_t>(bit_length % 8); | ||
| } | ||
| *buffer_it++ = value; | ||
| if (buffer_it == buffer_end) { | ||
| RETURN_NOT_OK(stream->Write(buffer, buffersize)); | ||
| buffer_it = buffer; | ||
| } | ||
| } | ||
| if (buffer_it != buffer) { | ||
| RETURN_NOT_OK(stream->Write(buffer, buffer_it - buffer)); | ||
| } |
Contributor
There was a problem hiding this comment.
Would it be easier to let the loop run only until i + 1 < length.
And then change L109-111:
uint8_t r = static_cast<uint8_t>(*data++ >> bit_shift);
*buffer_it++ = r;
RETURN_NOT_OK(stream->Write(buffer, buffer_it - buffer));
I.e. always handle the last byte after the loop.
Contributor
There was a problem hiding this comment.
Ah, it might still need masking of the upper bits of r.
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
2 participants
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.
Rationale for this change
The Feather V1 bitmap shift loop unconditionally read the byte after the final
source byte. For an exact-sized sliced bitmap, this caused a one-byte
out-of-bounds read and copied data outside the logical slice into unused bits of
the serialized bitmap.
Closes #50762.
What changes are included in this PR?
bitmaps.
byte and round-trip result.
Are these changes tested?
Yes.
arrow-feather-testsuite under ASan/UBSan: 84 passed, 5 expectedskips.
control and offset-one trigger both complete cleanly after the fix.
Are there any user-facing changes?
No public API changes. Feather V1 output no longer incorporates bits outside a
logical bitmap slice.
This PR contains a Critical Fix. It prevents an out-of-bounds read and
incorrect data from being copied into serialized output when the documented
array-buffer contract is upheld.
AI assistance
I used an AI coding assistant to help trace the writer, draft the implementation
and regression test, and run the verification commands. I reviewed the complete
diff, reproduced the original fault under ASan, verified the bit-boundary
logic, and ran the full Feather test suite. No maintainer was tagged or pinged.