fix: BitMasked builder use-after-free and GrowableBuffer copy bugs#4098
Conversation
Header-only LayoutBuilder/GrowableBuffer fixes found in a code review: - BitMasked stored a `uint8_t& current_byte_ref_` aliasing element 0 of the mask buffer's first panel. `append_begin()`/`clear()` assigned *through* the reference instead of rebinding it, so it forever aliased the freed panel after `clear()` (heap-use-after-free, confirmed with ASan), and mask bytes beyond the first landed in the wrong place. Removed the reference member; `current_byte_`/`current_index_` are now plain state and each completed byte is appended to `mask_` (with the `valid_when` inversion applied) when it fills, or written as the trailing partial byte at finalization (LayoutBuilder.h, BitMasked: constructors, clear, length, buffer_nbytes, to_buffers/to_buffer/to_char_buffers, append_begin/append_end, write_mask). - For BitMasked<valid_when=false> the inversion now happens where each completed byte lands in the buffer, fixing wrong mask bytes past 8 elements. - GrowableBuffer::Panel::concatenate_to_from recursed with `offset + length_` instead of `offset + (length_ - from)`, leaving a `from`-sized hole of uninitialised bytes when the source spanned multiple panels (hit by BitMasked masks > ~8190 elements) (GrowableBuffer.h:135). - GrowableBuffer memcpy sizes computed `length * sizeof(PRIMITIVE) - from`, mixing byte and element counts; corrected to `(length - from) * sizeof(...)` in Panel::append and concatenate_to_from (GrowableBuffer.h:119, :133). - Indexed::extend_index / IndexedOption::extend_valid computed `stop - 1` for size==0 on empty content, underflowing to SIZE_MAX and poisoning max_index_/last_valid_ so is_valid() falsely failed; guard size==0. Also removed the dead `else if (i < 0)` branch on a size_t (always false). Adds header-only/tests/test_4087-bitmasked-growablebuffer.cpp covering builder reuse after clear(), BitMasked valid_when=false, >8 elements, a >1-panel mask (20000 elements), zero-size extend_index/extend_valid, and multi-panel GrowableBuffer concatenate with non-zero `from` and sizeof(PRIMITIVE)>1; the UAF reproduces under ASan against the pre-fix headers. Also adds `enable_testing()` so ctest discovers the suite. The header-only library is standalone; the generated copies under awkward-cpp/ and src/awkward/_connect/header-only (produced by dev/copy-cpp-headers.py at prepare time) are not committed. Assisted-by: ClaudeCode:claude-fable-5
…Buffer PR Remove unused NumpyBuilder alias from test, drop restate-the-code comments (length formula, ceil expression, element-pattern description), and condense LayoutBuilder.h docstrings to remove padding while keeping the non-obvious rationale for the use-after-free fix and valid_when encoding constraint. Assisted-by: ClaudeCode:claude-sonnet-4-6
d6d26c7 to
b5dc4bc
Compare
|
🤖 This comment was written by Claude Code. Why this PR matters for the next release: CI on This PR fixes that. The rework removes the
However, this PR is itself red on the same target_link_libraries(test_4087-bitmasked-growablebuffer
PRIVATE awkward::layout-builder)
target_link_libraries(test_4088-layoutbuilder-form-fixes
PRIVATE awkward::layout-builder)Adding the |
|
I made a PR on this PR's branch that implements the fix described in the comment above: #4171. |
…4171) The test_4087-bitmasked-growablebuffer target was missing its `PRIVATE awkward::layout-builder)` line, so the call swallowed the next command and the header-only test suite failed to configure. Add the missing line so CMake can configure and the tests build. Claude-Session: https://claude.ai/code/session_013yRH8RuTZNTdyREnNLQTK9 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The documentation preview is ready to be viewed at http://preview.awkward-array.org.s3-website.us-east-1.amazonaws.com/PR4098 |
ianna
left a comment
There was a problem hiding this comment.
@henryiii and @TaiSakuma -- Thanks! It looks good. I'll enable auto-merge.
|
It looks promising. Header-only and pre-commit passed already. |
🤖 AI text below 🤖
This PR fixes several memory-safety and correctness bugs in the standalone
header-only/C++17 library (LayoutBuilderandGrowableBuffer), surfaced by an automated multi-agent code review. These changes are AI-assisted (Claude Code).Fixes
BitMasked use-after-free (heap-use-after-free, confirmed with ASan).
BitMaskedstored auint8_t& current_byte_ref_bound to element 0 of the mask buffer's first panel.append_begin()/clear()didcurrent_byte_ref_ = mask_.append_and_get_ref(...), which assigns through the reference rather than rebinding it, so it permanently aliased the original panel; afterclear()freed that panel, every mask-bit write was a UAF, and completed mask bytes past the first landed in the wrong place. The reference member is removed:current_byte_/current_index_are now plain state, and each completed byte is appended tomask_when it fills (in natural order), with the trailing partial byte emitted at finalization via a newwrite_mask()helper. Touches the constructors,clear(),length(),buffer_nbytes(),to_buffers()/to_buffer()/to_char_buffers(), andappend_begin()/append_end()(header-only/layout-builder/awkward/LayoutBuilder.h).BitMasked<valid_when=false> wrong mask bytes. Inversion previously happened only through the broken reference; it now happens where each completed byte lands in the buffer (and for the trailing byte in
write_mask()), fixing wrong bytes once more than 8 elements are appended (LayoutBuilder.h,append_end/write_mask).GrowableBuffer multi-panel hole.
Panel::concatenate_to_fromrecursed withoffset + length_instead ofoffset + (length_ - from), leaving afrom-sized hole of uninitialised bytes when the source spanned multiple panels (hit by BitMasked masks larger than one panel) (header-only/growable-buffer/awkward/GrowableBuffer.h:135).GrowableBuffer memcpy size mixed bytes and elements.
length * sizeof(PRIMITIVE) - fromis corrected to(length - from) * sizeof(PRIMITIVE)inPanel::appendandPanel::concatenate_to_from(previously only correct for 1-byte types orfrom == 0) (GrowableBuffer.h:119,:133).Indexed/IndexedOption zero-size extend underflow.
Indexed::extend_index(size)/IndexedOption::extend_valid(size)computedstop - 1insize_t; forsize == 0on empty content this underflowed toSIZE_MAXand poisonedmax_index_/last_valid_, makingis_valid()falsely fail. Both now guardsize == 0. The deadelse if (i < 0)branch (always false on asize_t) inappend_indexwas removed (LayoutBuilder.h:1367,:1378,:1594).Tests
Adds
header-only/tests/test_4087-bitmasked-growablebuffer.cppcovering: builder reuse afterclear(),BitMasked<valid_when=false>, more than 8 elements, a mask spanning more than one panel (20000 elements), zero-sizeextend_index/extend_valid, and a multi-panelGrowableBufferconcatenate with non-zerofromandsizeof(PRIMITIVE) > 1. Built against the pre-fix headers, the new test reproduces the heap-use-after-free under AddressSanitizer; with the fixes it passes both normally and under ASan. Also addsenable_testing()to the top-level header-onlyCMakeLists.txtsoctestdiscovers the suite.Notes
The
header-only/library is standalone. The generated copies underawkward-cpp/andsrc/awkward/_connect/header-onlyare produced bydev/copy-cpp-headers.pyatnox -s preparetime and are not tracked in git, so no generated copies are committed here.🤖 Generated with Claude Code