GH-45086: [C++] Fix heap buffer overflow in FillNullForward/Backward … - #50843
Open
tonyroberts wants to merge 3 commits into
Open
GH-45086: [C++] Fix heap buffer overflow in FillNullForward/Backward …#50843tonyroberts wants to merge 3 commits into
tonyroberts wants to merge 3 commits into
Conversation
…kward on chunked boolean arrays FillNullForwardChunked and FillNullBackwardChunked sized each output chunk's data buffer as `type->byte_width() * chunk->length()`. For BooleanType, byte_width() returns 0 (bit_width() / 8, truncated by integer division), so the buffer was allocated with 0 bytes while the chunk's declared length was unchanged, and filling it wrote real bit data past the end of the allocation. Add DataType::bytes_required(num_elements), a virtual method alongside byte_width()/bit_width() that correctly rounds up for bit-packed types, and use it at both call sites instead of the byte_width()-based calculation. Add regression tests exercising fill-null-forward and fill-null-backward on a chunked boolean array with a chunk large enough to reproduce the crash.
|
|
|
|
1 similar comment
|
|
uros-b
approved these changes
Aug 10, 2026
Member
|
Thank you @tonyroberts for the well targeted and correct fix! |
zanmato1984
reviewed
Aug 10, 2026
Avoid adding a new virtual method to DataType. Switch to the existing
arrow::util::internal::PreallocateFixedWidthArrayData helper, already
used by vector_selection_{take,filter}_internal.cc for this kind of
chunk allocation and correctly handles boolean's bit-packed layout.
Also fixes the same byte_width() * chunk->length() overflow in
ReplaceMaskChunked, with a regression test added to match.
|
|
zanmato1984
approved these changes
Aug 12, 2026
zanmato1984
left a comment
zanmato1984
left a comment
Contributor
There was a problem hiding this comment.
+1. @pitrou, what do you think of the current approach?
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
3 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
Fixes issue #45086 by fixing a heap heap buffer overflow in FillNullForward/Backward on chunked boolean arrays.
What changes are included in this PR?
FillNullForwardChunked and FillNullBackwardChunked sized each output chunk's data buffer as
type->byte_width() * chunk->length(). For BooleanType, byte_width() returns 0 (bit_width() / 8, truncated by integer division), so the buffer was allocated with 0 bytes while the chunk's declared length was unchanged, and filling it wrote real bit data past the end of the allocation.Add DataType::bytes_required(num_elements), a virtual method alongside byte_width()/bit_width() that correctly rounds up for bit-packed types, and use it at both call sites instead of the byte_width()-based calculation. Add regression tests exercising fill-null-forward and fill-null-backward on a chunked boolean array with a chunk large enough to reproduce the crash.This change uses
arrow::util::internal::PreallocateFixedWidthArrayDatato allocate the correct sized buffer.Are these changes tested?
Yes, and new unit tests have been added.
Are there any user-facing changes?
No
breaking changes. A new method, DataType::bytes_required(num_elements), was added.AI Disclosure
Claude was used to help fix this issue, but all the code has been reviewed and tested locally (on Windows only, built using gcc).