GH-45086: [C++] Fix heap buffer overflow in FillNullForward/Backward … - #50843
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
|
|
|
Thank you @tonyroberts for the well targeted and correct fix! |
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
left a comment
zanmato1984
left a comment
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>
|
Sorry, I mistakenly merged this PR using The code was merged successfully, but this skipped the script-generated I'll manually correct the PR label and linked issue. I will not rewrite |
3 participants
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).