fix(buffer): include the bit offset in BitBufferMut::from_buffer's bounds check - #9879
Conversation
…unds check `BitBuffer::new_with_offset`, `BitBufferView::new` and `BitBufferView::slice` all check `len.saturating_add(offset) <= buffer.len().saturating_mul(8)`. `BitBufferMut::from_buffer` checked only `len <= buffer.len() * 8`, ignoring the offset — so a caller could construct a bit view whose logical range extends past the underlying allocation. The offset parameter was added in vortex-data#4952 without updating the assertion that vortex-data#4937 introduced in the same release. Signed-off-by: jackylee-ch <qcsd2011@gmail.com>
Merging this PR will regress 3 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | random_i8[0.5] |
70.2 µs | 93.8 µs | -25.14% |
| ❌ | Simulation | decompress[u64, (4000, 1024)] |
70.5 µs | 87.5 µs | -19.38% |
| ❌ | WallTime | words_gather_scalar_avx2[65536] |
8.3 µs | 9.4 µs | -11.96% |
| ⚡ | WallTime | dict_canonicalize_gt_u8_avx2[16000000] |
10.8 ms | 6.8 ms | +59.93% |
| ⚡ | WallTime | arrow_checked_add_u32_neon[16384] |
20.4 µs | 13.4 µs | +52.6% |
| ⚡ | WallTime | filtered_owned_i64_avx2[OneNullInEight] |
27.1 µs | 21.9 µs | +23.81% |
| ⚡ | Simulation | random_i16[0.8] |
95.4 µs | 77.2 µs | +23.52% |
| ⚡ | WallTime | filtered_owned_i64_avx512[OneNullInEight] |
26.7 µs | 22.6 µs | +17.87% |
| ⚡ | Simulation | allocate_drop_bytes[0] |
575.7 ns | 521.6 ns | +10.39% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing jackylee-ch:fix/bitbuffermut-offset-bound (c711221) with develop (6465462)
Footnotes
-
218 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
BitBuffer::new_with_offset(bit/buf.rs:123),BitBufferView::new(bit/view.rs:77) andBitBufferView::slice(bit/view.rs:269) all assertlen.saturating_add(offset) <= buffer.len().saturating_mul(8).BitBufferMut::from_bufferasserted only
len <= buffer.len() * 8, so a caller could build a bit range extending past theallocation and have
value/setread or write outside it —freeze()would then panic pointingat
new_with_offset, a constructor the caller never called.The offset parameter arrived in #4952, two days after #4937 added this assertion without one.
The field doc claimed the offset is "always less than 8"; that holds for
BitBuffer, whichnormalises via
offset / 8, but not here —BitPackedArraypasses itsu16chunk offset(
bitpacking/compute/compare_fused.rs:122), so the doc is corrected rather than enforced.Tests
cargo test --release -p vortex-buffer --lib: 880 passed, 871 before.-p vortex-fastlanes(319 + 4) and
-p vortex-array --lib(3454, two pre-existingarrays::listviewfailures) areunchanged, which is what covers the two in-tree callers. Restoring the old assertion fails 4 of
the 9 new cases; the accepting cases pass either way.
AI assistance
Written with agentic AI assistance; I read all four assertions and both callers before changing
one.