Skip to content

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

Merged
AdamGS merged 2 commits into
vortex-data:developfrom
ksj1230:fix/bitbuffermut-checked-arithmetic
Jul 13, 2026
Merged

Fix integer overflow in BitBufferMut::truncate and append_n#8474
AdamGS merged 2 commits into
vortex-data:developfrom
ksj1230:fix/bitbuffermut-checked-arithmetic

Conversation

@ksj1230

@ksj1230 ksj1230 commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

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 thread vortex-buffer/src/bit/buf_mut.rs Outdated
Comment thread vortex-buffer/src/bit/buf_mut.rs Outdated
@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 improve performance by 13.04%

⚡ 5 improved benchmarks
✅ 1632 untouched benchmarks
⏩ 38 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation encode_varbin[(10000, 2)] 977.1 µs 836.1 µs +16.87%
Simulation bitwise_not_vortex_buffer_mut[128] 244.4 ns 215.3 ns +13.55%
Simulation chunked_varbinview_into_canonical[(100, 100)] 307.4 µs 271.5 µs +13.21%
Simulation compare_int_constant 298.7 µs 268.8 µs +11.1%
Simulation bitwise_not_vortex_buffer_mut[1024] 304.7 ns 275.6 ns +10.58%

Tip

Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.


Comparing ksj1230:fix/bitbuffermut-checked-arithmetic (0d12875) with develop (d2b2378)

Open in CodSpeed

Footnotes

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

@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
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

This PR has been marked as stale because it has been open for 14 days with no activity. Please comment or remove the stale label if you wish to keep it active, otherwise it will be closed in 7 days

@github-actions github-actions Bot added the stale This PR is stale and will be auto-closed soon label Jul 3, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This PR was closed because it has been inactive for 7 days since being marked as stale.

@github-actions github-actions Bot closed this Jul 12, 2026
@robert3005 robert3005 reopened this Jul 12, 2026
@robert3005

Copy link
Copy Markdown
Contributor

@AdamGS anything missing here?

@robert3005 robert3005 added changelog/fix A bug fix and removed stale This PR is stale and will be auto-closed soon labels Jul 13, 2026
Signed-off-by: Robert Kruszewski <github@robertk.io>

@AdamGS AdamGS left a comment

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.

No, just lost track of it

@AdamGS

AdamGS commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

@ksj1230 thank you for you contribution! I apologize for taking so long.

@AdamGS AdamGS merged commit 371c984 into vortex-data:develop Jul 13, 2026
75 checks passed
@ksj1230

ksj1230 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for merging! Happy to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants