collections: check DynamicBitSetList index and set-op lengths in release builds

[robobun]

Problem

• DynamicBitSetList::at(i) (src/collections/bit_set.rs) checked i < n with a debug_assert! only. The release profile has debug assertions off, so an out-of-range i produced a view whose masks pointer lies past the end of the list's heap buffer; DynamicBitSetList::set / set_union then write through it (heap out-of-bounds write) and readers such as count / subset_of read past it. Miri with debug assertions off reports the .add(offset) in at itself: Undefined Behavior: in-bounds pointer arithmetic failed ... at or beyond the end of the allocation at bit_set.rs:913 (main). The callers are the install pipeline (hoisted_install.rs, isolated_install.rs, PackageInstaller::can_run_scripts), indexing by tree id and package id.
• Same file, same shape: DynamicBitSetUnmanaged::zip_masks_raw reads num_masks(self.bit_length) words out of other through raw pointers. set_union / set_intersection / set_exclude guarded the lengths with a debug_assert!; copy_into had no check. DynamicBitSet::copy_into documents copying into a larger set and PackageInstaller::fix_cached_lockfile_package_slices (src/install/PackageInstaller.rs:1089) uses it that way when packages get appended to the lockfile while the installer runs, so whenever the new size needs more words than the old one the copy reads past the end of the old bitset's allocation, in every build, and the bytes that happen to follow it become successfully_installed bits. Miri on main: Undefined Behavior: memory access failed: attempting to access 8 bytes, but got alloc+0x10 which is at or beyond the end of the allocation of size 16 bytes at bit_set.rs:513.
• The doc on at also said that its return value (a ManuallyDrop<DynamicBitSetUnmanaged> holding a raw pointer, no lifetime) must not outlive the list and must not be deinited; both compiled fine and were enforced by the comment only (resize / deinit on it would free the middle of the list's buffer).
• Found by reading the code while fixing the same pattern elsewhere (Check ZStr/WStr constructor and errno discriminant preconditions in release builds #38914); there is no user report. Whether an out-of-range id can reach at from a lockfile on disk depends on the install side (install: validate bun.lockb slice descriptors and ids at load time #32753 is about that); the collection has to be sound either way.

Fix

• at uses assert! (one compare; the SAFETY comment already claimed the index was asserted). init_empty sizes the buffer with checked_mul, so buf_len == slot_words * n holds for every n and the in-bounds argument in at is complete.
• at returns DynamicBitSetListEntry<'_>: the same ManuallyDrop<DynamicBitSetUnmanaged> plus a PhantomData<&'a DynamicBitSetList>. It derefs to DynamicBitSetUnmanaged for reading and exposes only set / set_union / copy_into for writing, so an entry that outlives the list is a compile error (compile_fail,E0597 doctest) and resize / deinit cannot be reached. The three bun_install callers compile unchanged; they use the entry through deref coercion.
• zip_masks_raw asserts that the lengths are equal, and the now redundant per-caller debug_assert!s are removed. The check that keeps a safe fn's unsafe block in bounds is not debug-only, and it belongs in the shared helper rather than in each caller.
• copy_into no longer goes through zip_masks_raw: it copies the words both sets have with ptr::copy (two entries of one list may alias, so the copy has to tolerate overlap), clears the remaining destination words and re-masks the padding bits. Equal lengths behave exactly as before; a shorter source now zero-extends, which is what DynamicBitSet::copy_into's doc promises and fix_cached_lockfile_package_slices needs; a longer source truncates, as before.
• Left as they were: is_set / set / unset / set_range_value keep their debug_assert!s, because they index a slice of num_masks words, so a bad bit index panics or touches a padding bit and cannot leave the allocation. bit_length being a pub field is the remaining way safe code could break the invariant; nothing outside the crate writes it, and it is not touched here.
• Verified:
  • test/internal/source-lints/bit-set-release-checks.test.ts extracts the bodies of at and zip_masks_raw and requires a release-mode assertion and no debug_assert! in each. On main both entries fail (at: debug_assert only; zip_masks_raw: no check); with this branch 3 pass, under bun test and bun bd test. It is a source lint because bun bd builds with debug assertions on, so no test that runs the binary can tell the two kinds of assertion apart.
  • 19 unit tests added to bit_set.rs (cargo test -p bun_collections; the cargo miri test CI job runs them too): out-of-range at / set / set_union panic, length mismatch panics for the three set ops directly and through the list, copy_into with a shorter / same-word-count-shorter / longer / equal / empty operand, the DynamicBitSet::copy_into grow shape (64 to 65 bits), entries writing through to the list and an entry copied onto itself, plus the compile_fail doctest. Against main's implementation: 5 fail in the dev profile (the shorter-source copy produces garbage bits like [1, 67, 68, 69, ...]), 9 fail with --release (nothing panics), and Miri reports the UB quoted above. With the fix: all pass under cargo test, cargo test --release, bun run rust:miri -p bun_collections (54 tests + the doctest), Miri's default Stacked Borrows model, and Miri with --release.
  • cargo check -p bun_install, cargo clippy -p bun_collections, cargo fmt --check clean.
  • bun bd, then bun bd test on test/cli/install/isolated-install.test.ts (66 pass), hoist.test.ts, bun-lockb.test.ts, and bun-install-lifecycle-scripts.test.ts (119 pass; the 3 failures are the tests that need node / bun on PATH, which this container lacks, and do not involve bitsets).

Background

• DynamicBitSetUnmanaged stores bit_length bits as num_masks = ceil(bit_length / 64) words behind a raw masks pointer; the word before masks holds the allocation length for resize and free. Padding bits past bit_length in the last word must stay zero (count and iteration rely on it). Its binary set operations use raw pointers rather than slices because the two operands may be views of the same memory.
• DynamicBitSetList packs n bitsets of one length into a single allocation: n slots of one header word plus num_masks mask words. at(i) builds a DynamicBitSetUnmanaged pointing into slot i; it is a view, not an owner, which is what the ManuallyDrop is for. The list hands these out from &self and keeps its buffer as a raw allocation on purpose (see the struct doc), which is why writing through an entry while holding only &DynamicBitSetList is sound.
• debug_assert! only produces code when debug-assertions is on: in this repo that is the dev profile (bun bd) and the asan / assertions release variants; the shipped release profile has it off. A precondition that a safe function's unsafe block depends on therefore needs assert!.
• Miri interprets the crate's unit tests and reports out-of-bounds accesses and out-of-bounds pointer arithmetic as UB; bun run rust:miri runs it over bun_collections in CI.

Miri output against main's bit_set.rs (new tests only)

$ MIRIFLAGS=-Zmiri-tree-borrows cargo miri test -p bun_collections --lib bit_set
test bit_set::tests::copy_into_from_a_shorter_set_clears_the_rest ... error: Undefined Behavior: memory access failed: attempting to access 8 bytes, but got alloc90353+0x10 which is at or beyond the end of the allocation of size 16 bytes
   --> src/collections/bit_set.rs:513:51
            unsafe { *dst.add(i) = f(*dst.add(i), *src.add(i)) };

$ MIRIFLAGS=-Zmiri-tree-borrows cargo miri test --release -p bun_collections --lib bit_set::tests::list_set_past_the_end_panics
test bit_set::tests::list_set_past_the_end_panics - should panic ... error: Undefined Behavior: in-bounds pointer arithmetic failed: attempting to offset pointer by 8 bytes, but got alloc68500+0x20 which is at or beyond the end of the allocation of size 32 bytes
   --> src/collections/bit_set.rs:913:29
            masks: unsafe { self.buf.as_ptr().add(offset).add(1) },

$ cargo test --release -p bun_collections --lib bit_set      # main's implementation
test bit_set::tests::list_at_one_past_the_end_panics - should panic ... FAILED   (did not panic)
test bit_set::tests::list_set_past_the_end_panics - should panic ... FAILED      (did not panic)
test bit_set::tests::list_set_union_past_the_end_panics - should panic ... FAILED
test bit_set::tests::set_union_with_a_shorter_operand_panics - should panic ... FAILED
test bit_set::tests::copy_into_from_a_shorter_set_clears_the_rest ... FAILED
  left: [1, 69, 128, 132, 134, 135, 136, 193, 196, 197, 198]     (expected [1])
test result: FAILED. 10 passed; 9 failed

[robobun]

^{Updated 12:05 PM PT - Aug 15th, 2026}

❌ @robobun, your commit 2dfbe5a has some failures in Build #98109 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 39055

That installs a local version of the PR into your bun-39055 executable, so you can run:

bun-39055 --bun

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 27 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 13e7d254-947a-4910-ac69-e0c529d1392e

📥 Commits

Reviewing files that changed from the base of the PR and between 88a6398 and 2dfbe5a.

📒 Files selected for processing (2)

• src/collections/bit_set.rs
• test/internal/source-lints/bit-set-release-checks.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: fix pushed, waiting for CI.

Reproduced with the unit tests added in this PR against main's bit_set.rs: Miri reports UB at bit_set.rs:513 (copy_into from a shorter set, every profile) and at bit_set.rs:913 (DynamicBitSetList::set past the end, debug assertions off); cargo test --release shows the out-of-range at / set / set_union calls not panicking. The source lint test/internal/source-lints/bit-set-release-checks.test.ts fails on main for both functions and passes here.

[claude]

I reviewed this PR and didn't find any bugs. Because it reworks unsafe raw-pointer code (new DynamicBitSetListEntry<'_> abstraction, changed at() return type, and a semantic change to copy_into from UB-on-shorter-source to zero-extend), a maintainer look would still be worthwhile.

What was reviewed:

• at() bounds check, checked_mul in init_empty, and the bit_length == 0 slot case (one-past-end pointer, zero-length slice — sound).
• copy_into rewrite: ptr::copy overlap-safety, padding-zero invariant preserved for both same-word-shorter-source and longer-source paths, and the aliasing case where masks_slice_mut() is formed while other: &Self is live (no post-copy read through other.masks).
• All bun_install call sites (PackageInstaller.rs, isolated_install.rs, hoisted_install.rs) — Deref coercion covers the read paths and the entry's own copy_into/set_union cover the write paths; fix_cached_lockfile_package_slices is exactly the shorter-source shape this fixes.
• Source-lint regexes: HARD_CHECK's negative lookbehind correctly excludes debug_assert!; fnBodies brace-matching handles the nested-braces / doc-comment cases in its self-test.

Extended reasoning...

Overview

This PR hardens src/collections/bit_set.rs against release-build UB by upgrading two debug_assert!-only preconditions to assert!: the index bound in DynamicBitSetList::at and the equal-length precondition in zip_masks_raw (which set_union/set_intersection/set_exclude route through). It also fixes an unconditional out-of-bounds read in DynamicBitSetUnmanaged::copy_into when the source is shorter than the destination — a shape that PackageInstaller::fix_cached_lockfile_package_slices reaches in every build. The at() return type changes from ManuallyDrop<DynamicBitSetUnmanaged> to a new DynamicBitSetListEntry<'_> that ties the view's lifetime to the list and hides resize/deinit behind a curated write surface. 19 Rust unit tests (Miri-run in CI), a compile_fail doctest, and a source-lint test are added.

Security risks

None introduced. The change strictly reduces attack surface: preconditions that guard unsafe pointer arithmetic in safe pub fns now hold in release builds, so an out-of-range tree/package id from a lockfile becomes a panic rather than a heap out-of-bounds write. init_empty gains checked_mul so buf_len cannot silently wrap.

Level of scrutiny

High. This is unsafe Rust manipulating raw heap pointers on a hot install path, with a public API change (new return type for at()), a new public type, and a deliberate semantic change to copy_into (shorter source now zero-extends instead of reading garbage). The reasoning is sound and Miri-verified under both borrow models, but the DynamicBitSetListEntry design (Deref-for-read, curated write methods, PhantomData<&'a List>) and the decision to make copy_into length-tolerant rather than length-asserting are choices a maintainer should ratify.

Other factors

Test coverage is unusually thorough: unit tests cover every panic path, every copy_into length combination, self-aliasing, and the exact DynamicBitSet::copy_into grow shape from fix_cached_lockfile_package_slices. I traced every bun_install .at() caller and confirmed they compile via Deref coercion or the entry's own methods with no behavior change. I checked the bit_length == 0 list case: slot_words(0) == 1, at(n-1) computes buf.add(n) which is a valid one-past-end pointer, and num_masks(0) == 0 so it's never dereferenced. I also verified the copy_into aliasing path is SB-sound because masks_slice_mut() is only formed after the last read through other.masks. DynamicBitSetListEntry is reachable via bun_collections::bit_set:: even though it's not in the top-level re-export list, and no caller needs to name it. The source-lint test follows the existing test/internal/source-lints/ pattern.