Skip to content

fix: avoid u32 overflow in statistics for full bitmaps - #358

Merged
Kerollmops merged 2 commits into
RoaringBitmap:mainfrom
SAY-5:fix-statistics-overflow
Oct 26, 2026
Merged

Kerollmops merged 2 commits into
RoaringBitmap:mainfrom
SAY-5:fix-statistics-overflow

Conversation

@SAY-5

@SAY-5 SAY-5 commented Jun 18, 2026 •

Copy link
Copy Markdown
Contributor

RoaringBitmap::full().statistics() accumulates 2^32 values in run containers. The u32 run-count field overflows with overflow checks enabled and wraps to zero without them. Widen n_values_run_containers to u64 and remove the narrowing cast.

Keep n_values_array_containers as u32: at most 65,536 array containers can each hold 4,096 values, so its maximum is 2^28. The regression tests check the full bitmap and preserve the array field's existing u32 API.

Changing the public run-count field from u32 to u64 is a breaking API change. Release placement is up to the maintainers.

Validation on Rust 1.90.0 / macOS ARM64: the affected integration file passes all 12 tests in direct std and no-std builds with overflow checks enabled. Direct Clippy checks of the library and that test target pass with -D warnings in both modes, and cargo fmt --all -- --check passes. The full Cargo suite was stopped during dependency compilation at the local build budget, before tests ran; serde, SIMD, property tests, doc tests and benchmarks remain unverified locally.

Closes #357.

The follow-up scope correction and verification were assisted by OpenAI Codex.

@Kerollmops Kerollmops added the breaking This change will require a bump of the minor or major version. label Jul 10, 2026
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
@fjmaco

fjmaco commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Tested on top of current main (0439d57):

  • On main and 0.11.5, RoaringBitmap::full().statistics() panics at statistics.rs:92 ("attempt to add with overflow") in a debug build. In release it silently reports n_values_run_containers: 0. With this branch it's 4294967296 in both, and statistics_full_run fails on main and passes here.
  • Other bitmaps are unaffected. The Statistics output is identical on main and this branch for empty, 1M random, dense, run-heavy, mixed, half-range and full-minus-two bitmaps. (Only an exactly-full bitmap overflows; 2^32 − 1 values still fit.) cargo test -p roaring passes, and the branch merges cleanly into main.

One note for release planning: changing a public field's type is a breaking change. let n: u32 = stats.n_values_run_containers; compiles against 0.11.5 and fails with E0308 here, so this probably belongs in the next minor (0.12) rather than a 0.11.x patch. Also, only the run field can actually overflow: array containers hold at most 4096 values, so n_values_array_containers tops out at 65536 × 4096 = 2^28.

@SAY-5

SAY-5 commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Restored n_values_array_containers to u32 in ffd45be, with a typed regression covering values across two array containers. The run count stays u64 so a full bitmap reports 2^32. I also removed the redundant conversion flagged by Clippy and corrected the description. The run-field type change still needs a release that permits a breaking API change.

@Kerollmops Kerollmops left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey @SAY-5 and @fjmaco 👋

Thank you for the fix and tests. I'll merge this right now as we will have breaking changes in #379 anyway. Merging.

Have a nice week 🌵

@Kerollmops
Kerollmops added this pull request to the merge queue Oct 26, 2026
Merged via the queue into RoaringBitmap:main with commit 6982fa5 Oct 26, 2026
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking This change will require a bump of the minor or major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

roaring::RoaringBitmap::full().statistics() panics if overflow checks are enabled

3 participants