Skip to content

Fix integer overflow in BitBufferMut::truncate and append_n#8474

Open
ksj1230 wants to merge 1 commit into
vortex-data:developfrom
ksj1230:fix/bitbuffermut-checked-arithmetic
Open

Fix integer overflow in BitBufferMut::truncate and append_n#8474
ksj1230 wants to merge 1 commit into
vortex-data:developfrom
ksj1230:fix/bitbuffermut-checked-arithmetic

Conversation

@ksj1230

@ksj1230 ksj1230 commented Jun 17, 2026

Copy link
Copy Markdown

Summary

Several methods on BitBufferMut use unchecked arithmetic on offset/length values, which can overflow in release mode and lead to undefined behavior reachable entirely from safe code (#![forbid(unsafe_code)]).

Affected methods:

  • truncate: self.offset + len overflows, causing the buffer to be truncated to an incorrect size.
  • append_n: self.offset + self.len + n overflows, causing incorrect capacity calculation.

Fix: add overflow assertions before the unchecked addition.

Reproduction (all use #![forbid(unsafe_code)]):

// 1. truncate
let buffer = ByteBufferMut::zeroed(10);
let mut bb = BitBufferMut::from_buffer(buffer, usize::MAX - 2, 80);
bb.truncate(3);
bb.set(2);

// 2. append_n
let buffer = ByteBufferMut::zeroed(2);
let mut bb = BitBufferMut::from_buffer(buffer, 1000, 0);
bb.append_n(true, usize::MAX);
bb.set(10000);

Related: #7979 (the BufferMut::zeroed_aligned overflow I reported to maintainers in April 2025; this PR addresses additional overflow sites found during the same audit)

Testing

Existing cargo test -p vortex-buffer passes (789 tests + 6 doc-tests).
Both PoCs now panic with overflow message instead of UB.

Happy to adjust the approach if maintainers prefer a different fix strategy.

@ksj1230 ksj1230 requested a review from a team June 17, 2026 14:39
@miniex

miniex commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

sign off the commit to pass DCO: git commit --amend -s --no-edit && git push -f

Comment thread vortex-buffer/src/bit/buf_mut.rs Outdated
Comment on lines 189 to 191
let num_words = len.checked_add(63)
.expect("collect_bool: len overflow") / 64;
let mut buffer: BufferMut<u64> = BufferMut::with_capacity(num_words);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does this overflow?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick review.

You're right. standard div_ceil uses division first, so it doesn't overflow. It only causes out-of-memory, which is not a soundness issue. Removed from this PR.

Comment thread vortex-buffer/src/bit/buf_mut.rs Outdated
Comment on lines +376 to +381
@@ -376,7 +377,8 @@ impl BitBufferMut {
return;
}

let end_bit = self.offset + len;
let end_bit = self.offset.checked_add(len)
.expect("BitBufferMut::truncate: offset + len overflow");

@joseph-isaacs joseph-isaacs Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is gated 3 lines above

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guard bounds len by self.len, but self.offset is independent and can be set to a large value via from_buffer without validation. So self.offset + len can still overflow even when len <= self.len.

Miri confirms UB with the PoC in the description:
error: Undefined Behavior: attempting to offset pointer by
2305843009213693951 bytes, but got alloc192 which is only 11 bytes

Comment thread vortex-buffer/src/bit/buf_mut.rs Outdated
}

let end_bit_pos = self.offset + self.len + n;
let end_bit_pos = self.offset.checked_add(self.len)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of doing checked arithmatic, how about asserting these? The code is much clearer that way IMO.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to assertions and updated the message style. Thanks!

@AdamGS

AdamGS commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

stylistically - I think we use error messagess that read more like "Operation X on Y overflowed"

@codspeed-hq

codspeed-hq Bot commented Jun 17, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 19.69%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚡ 6 improved benchmarks
❌ 13 regressed benchmarks
✅ 1502 untouched benchmarks
⏩ 71 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation slice_empty_vortex 368.3 ns 2,628.6 ns -85.99%
Simulation slice_vortex_buffer[1024] 871.4 ns 1,335 ns -34.73%
Simulation slice_vortex_buffer[16384] 871.4 ns 1,335 ns -34.73%
Simulation slice_vortex_buffer[2048] 871.4 ns 1,335 ns -34.73%
Simulation slice_vortex_buffer[128] 871.4 ns 1,335 ns -34.73%
Simulation slice_vortex_buffer[65536] 871.4 ns 1,335 ns -34.73%
Simulation chunked_bool_canonical_into[(1000, 10)] 20.3 µs 27.7 µs -26.8%
Simulation chunked_varbinview_canonical_into[(1000, 10)] 162.2 µs 198.5 µs -18.27%
Simulation chunked_varbinview_into_canonical[(1000, 10)] 177.7 µs 214.6 µs -17.21%
Simulation search_index_below_min_chunked 1.3 ms 1.5 ms -13.61%
Simulation search_index_mixed_out_of_range_chunked 1.3 ms 1.5 ms -13.32%
Simulation count_i32_clustered_nulls 47 µs 53.9 µs -12.85%
Simulation search_index_full_range_random_chunked 1.4 ms 1.6 ms -12.07%
Simulation take_10k_random 255.8 µs 198 µs +29.19%
Simulation take_10k_contiguous 276.3 µs 218.5 µs +26.43%
Simulation patched_take_10k_contiguous_patches 291 µs 232.3 µs +25.28%
Simulation patched_take_10k_random 303 µs 244.2 µs +24.09%
WallTime cuda/bitpacked_u8/unpack/3bw[100M] 352 µs 299.4 µs +17.58%
Simulation true_count_vortex_buffer[128] 638.3 ns 580 ns +10.06%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing ksj1230:fix/bitbuffermut-checked-arithmetic (3f375ae) with develop (85aad72)2

Open in CodSpeed

Footnotes

  1. 71 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.

  2. No successful run was found on develop (0ed06b3) during the generation of this report, so 85aad72 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@ksj1230 ksj1230 force-pushed the fix/bitbuffermut-checked-arithmetic branch 2 times, most recently from e1b0d3e to 141d008 Compare June 18, 2026 00:22
Signed-off-by: Sungjin Kim <carp1230@gmail.com>
@ksj1230 ksj1230 force-pushed the fix/bitbuffermut-checked-arithmetic branch from 141d008 to 078b397 Compare June 18, 2026 00:52
@ksj1230 ksj1230 changed the title Fix checked arithmetic in BitBufferMut::collect_bool, truncate, and append_n Fix integer overflow in BitBufferMut::truncate and append_n Jun 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants