semver: widen comparator counter to u32 to avoid debug overflow panic#31010
Conversation
|
Updated 9:10 PM PT - May 18th, 2026
✅ @robobun, your commit e21ce998a90551dc62d009ef118f8071ce0b96b2 passed in 🧪 To try this PR locally: bunx bun-pr 31010That installs a local version of the PR into your bun-31010 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR widens the semver parser's local token counter, adds a regression test that spawns a Bun subprocess evaluating a 300-fragment ChangesSemver Comparator Count Regression Test & Parser Fix
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
@robobun fix review comments |
|
✅ Review feedback addressed — diff is the Verified: Buildkite: all test lanes that run are green. The only reds are macOS aarch64 lanes reporting |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/install/semver.test.ts`:
- Around line 751-753: Capture the subprocess stderr alongside stdout and exited
by adding proc.stderr.text() to the Promise.all call (keep variables named
stdout, stderr, exitCode) and apply the "house" pattern: after awaiting, if
(exitCode !== 0) { expect(stderr).toBe(""); } immediately before
expect(exitCode).toBe(0) so failures surface stderr content for diagnostics;
update the Promise.all usage where stdout and exitCode are currently awaited in
semver.test.ts to include stderr and adjust subsequent assertions accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed1b9ebb-ab8d-474f-9868-c591decd166b
📒 Files selected for processing (2)
src/semver/SemverQuery.rstest/cli/install/semver.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/semver/SemverQuery.rs:725-729— The sibling Zig implementation atsrc/semver/SemverQuery.zig:572still declaresvar count: u8 = 0;, and that parser is the live code path forbun install(viasrc/install/dependency.zig:882andsrc/install/lockfile/Package.zig:1045/1052) — so a dependency range with >=256||comparators still overflows there (panic in Debug/ReleaseSafe, silent wrap to 0 in ReleaseFast). The PR description already calls the Zig wrap "a latent bug that merely didn't crash"; applying the same one-tokenu8→u32change at SemverQuery.zig:572 would complete the fix for parity. Non-blocking since the input is pathological and the Zig file is outside this diff.Extended reasoning...
What the bug is
This PR widens
countfromu8tou32insrc/semver/SemverQuery.rs:729, but the Zig implementation it was ported from —src/semver/SemverQuery.zig:572— still declaresvar count: u8 = 0;with an uncheckedcount += 1;at line 770. The Rust file's own trailing comment says// ported from: src/semver/SemverQuery.zig, and the new code comment added in this PR explicitly states "Zig's ReleaseFast wrapped (256 -> 0) silently" — so the author is aware the Zig sibling has the identical defect, but only the Rust port is patched.The Zig parser is the live
bun installpathThe Zig
SemverQuery.parseis not dead code.src/semver/semver.zig:9exportspub const Query = @import("./SemverQuery.zig"), andbun installresolves dependency ranges through it:src/install/dependency.zig:882—Semver.Query.parse(allocator, dependency, sliced)src/install/lockfile/Package.zig:1045and:1052—Semver.Query.parse(allocator, ...)
These call sites pass an
allocatorargument, which matches the Zig signature, not the Rust one. Only the JS APIBun.semver.satisfies(the path the new regression test exercises) goes through the Rust port viabun_semver_jsc::SemverObject::satisfies. So apackage.jsondependency like"foo": "1.0.0 || 1.0.0 || ..."with 256+ comparators still hits the unfixedu8counter.Why nothing prevents it
There is no length cap on the range string and no saturating arithmetic on
count. In Zig,count += 1on au8at 255:- Debug / ReleaseSafe: panics on integer overflow (the same abort class this PR set out to fix, just via a different entry point).
- ReleaseFast (production): wraps 256 → 0 silently.
Step-by-step proof of the semantic divergence on wrap
Consider a range of 256
1.0.0comparators joined by||, followed by|| >2.0.0as the 257th:- After 256 non-skipped rounds,
countwraps from 255 to 0. - The
||token setsis_or = true,token.tag = .none,skip_round = true. - The 257th comparator
>2.0.0setstoken.tag = .gt(not.version, not.none). - In the dispatch chain,
hyphenateis false,count == 0 and token.tag == .versionis false (tag is.gt), so it falls into the next branch:else if (count == 0)→list.andRange(...). - With a non-wrapped counter,
countwould be 256 and the same token would instead hitelse if (is_or)→list.orRange(...).
So the wrap re-enters the first-comparator special case and turns an OR into an AND, changing which versions satisfy the range. This is exactly what the PR description calls "a latent bug that merely didn't crash".
Impact and fix
Impact is low in practice — 256+ comparators in a real
package.jsonis pathological — but it is the precise sibling of the bug being fixed here, on the code path (bun install) where untrusted package manifests are actually parsed. The fix is the same one-token widening:// src/semver/SemverQuery.zig:572 - var count: u8 = 0; + var count: u32 = 0;
The new regression test only spawns
Bun.semver.satisfies(Rust path), so it does not cover this. Filing as a non-blocking nit for parity since the Zig file is outside this diff and the trigger is implausible, but it's a one-line change that completes the PR's stated goal.
There was a problem hiding this comment.
LGTM — one-token u8→u32 widening on a local counter that's only ever compared to zero; strictly safer than before. (My Zig-side FYI is pre-existing and non-blocking.)
Extended reasoning...
Overview
Single-character production change: let mut count: u8 = 0; → let mut count: u32 = 0; at src/semver/SemverQuery.rs:725. count is a function-local that's only incremented and compared == 0 — never stored, cast, or returned — so widening cannot change behavior except to remove the debug-build overflow panic at 256 comparators. The accompanying regression test in test/cli/install/semver.test.ts follows the house subprocess pattern (bunEnv/bunExe, await using, stderr captured and asserted on failure, exitCode asserted last) and all earlier review nits (mine and CodeRabbit's) were applied in 90793cd.
Security risks
None. No user-controlled data flows anywhere new; this only widens an internal loop counter. No auth, crypto, FS, or network surface touched.
Level of scrutiny
Low. This is a mechanical type-widen on a local variable whose sole observable use is a zero-check, plus a self-contained regression test. The one CI failure on the build dashboard is an infrastructure agent-provisioning error ("Failed to create agent for aarch64 - build-cpp"), not a build or test failure attributable to this diff.
Other factors
I previously left an FYI inline comment noting the parallel u8 counter still exists in src/semver/SemverQuery.zig (the file this was ported from, still used by bun install). That was explicitly flagged as pre-existing and non-blocking — it's a reasonable follow-up but doesn't gate this PR, which strictly improves the Rust path without regressing anything else.
- wtf-bindings.cpp: assert -> ASSERT (same as #30992; #30705 dropped the transitive <cassert> include so debug builds fail to compile) - semver.test.ts: 30s timeout on the 1e5-iteration memory-churn test and the subprocess regression test; both exceed the 5s default under debug+ASAN on constrained runners
There was a problem hiding this comment.
LGTM — one-token type widening plus regression test; all prior review nits addressed.
Extended reasoning...
Overview
Three files touched: src/semver/SemverQuery.rs widens a local count from u8 to u32 (the variable is only ever compared == 0, never stored/cast, so this is strictly a correctness improvement with no behavioral side-effects beyond removing the overflow). src/jsc/bindings/wtf-bindings.cpp swaps libc assert() for the project's WTF ASSERT() macro on one line — a mechanical convention fix to unblock the debug build, consistent with the existing ASSERT at line ~244 of the same file. test/cli/install/semver.test.ts adds the harness imports, bumps one existing slow-loop test's timeout to 30s, and adds a subprocess regression test for the 300-comparator range.
Security risks
None. The semver parser change widens a local integer type with no effect on memory layout, allocation, or external input handling beyond not panicking. The assert→ASSERT swap is a no-op in release and a stricter check in debug. The test only spawns the local bunExe() with a fixed literal payload.
Level of scrutiny
Low. The Rust change is one token; count's only readers are two == 0 checks, so widening cannot change semantics for inputs <256 comparators and only fixes the pathological ≥256 case. The C++ change is a macro-name swap on a null-check that's already guarded by the sole caller. The test follows the repo's documented spawn-test pattern (stderr captured, asserted on non-zero exit, exitCode last).
Other factors
I previously left two inline comments on this PR: a stderr-capture nit (applied in 90793cd/b5e51db — the diff now reads proc.stderr.text() and guards if (exitCode !== 0) expect(stderr).toBe("")) and an FYI about the sibling SemverQuery.zig (author clarified it's a non-compiled porting reference per src/CLAUDE.md, which is a satisfactory answer). CodeRabbit's identical stderr nit is also resolved. All inline threads are marked resolved, no bugs were found by the bug-hunting system, and Buildkite is building the latest commit. Nothing remains outstanding.
There was a problem hiding this comment.
LGTM — one-token type widening with a regression test; the remaining inline note is a non-blocking diagnostic nit.
Extended reasoning...
Overview
Three changes: (1) src/semver/SemverQuery.rs widens a local count from u8 to u32 — the variable is only ever compared == 0 and never stored/cast, so this is strictly more correct and fixes a debug-build overflow panic at ≥256 comparators; (2) src/jsc/bindings/wtf-bindings.cpp swaps assert → ASSERT, a one-token carry from #30992 to unblock the debug build after the WebKit upgrade; (3) test/cli/install/semver.test.ts adds a subprocess regression test and bumps two timeouts to 30s for debug+ASAN headroom.
Security risks
None. No user-facing input handling changes, no auth/crypto/permissions. The semver parser already accepted arbitrary-length input; widening a local counter does not change the attack surface.
Level of scrutiny
Low. The Rust change is a single-token type widening on a local with no downstream type dependencies. The C++ change is a macro rename identical to the one in #30992. The test follows the documented bunExe()/bunEnv spawn pattern from test/CLAUDE.md.
Other factors
All prior inline review comments (mine and CodeRabbit's) are resolved — stderr is now captured, comments removed per src/CLAUDE.md, and the .zig sibling was correctly left untouched per the repo's reference-only convention. The one remaining inline nit (assertion order for better failure diffs) is explicitly diagnostic-quality only; the test is functionally correct either way, and the current order matches an established house pattern (PR #29322 learning). The Buildkite failures on 0a802c5 are unrelated HiveArrayFallback deprecation errors in files this PR does not touch, plus a known jsc-stress Windows-aarch64 flake the author re-rolled.
There was a problem hiding this comment.
LGTM — one-token type widening with a clean regression test; all prior nits addressed.
Extended reasoning...
Overview
Three files touched: (1) src/semver/SemverQuery.rs widens a local count variable from u8 to u32 — the variable is only ever compared == 0 and incremented, never stored or cast, so widening is strictly safer and fixes a debug-build overflow panic at the 256th comparator; (2) src/jsc/bindings/wtf-bindings.cpp swaps lowercase assert for WTF's ASSERT macro on one line inside a non-Windows TTY helper, carried from #30992 to unblock the debug build after #30705 dropped the transitive <cassert> include; (3) test/cli/install/semver.test.ts adds a subprocess regression test for the 300-comparator case plus a 30s timeout on the pre-existing 1e5-iteration test for debug+ASAN headroom.
Security risks
None. The semver parser change widens a local integer used purely as a "first token seen" sentinel — no narrowing casts, no externally observable size, no allocation sizing derived from it. The assert→ASSERT swap is in debug-only TTY termios setup and is semantically equivalent (both compile out in release). The test spawns bunExe() with a fixed inline -e script under bunEnv; no untrusted input flows into the command.
Level of scrutiny
Low. The Rust hunk is a one-token type change to a local with trivially provable correctness (every use site — count == 0 twice and count += 1 once — accepts u32 identically). The C++ hunk is a mechanical macro rename already landing separately in #30992. The test follows the repo's documented spawn-and-assert pattern from test/CLAUDE.md after three rounds of nit feedback (stderr capture, ordering, ASAN happy-path tolerance), all of which are now resolved in the final diff.
Other factors
All three of my prior inline comments and CodeRabbit's nit are marked resolved and verified against the current diff: stderr is read via Promise.all, the if (exitCode !== 0) expect(stderr).toBe("") guard now precedes the stdout assertion so panic backtraces surface, and the Zig sibling (SemverQuery.zig) was correctly identified as a non-compiled porting reference per src/CLAUDE.md. CI is green except for an unrelated Windows aarch64 JSC stress flake (mprotect failed) that the #if !OS(WINDOWS) hunk cannot affect. No outstanding human reviewer comments.
3 participants
A version range with >=256
||/whitespace-separated comparators panicked the debug build (attempt to add with overflow, exit 133) where released Bun fails gracefully. Debug-only: Cargo's dev profile enablesoverflow-checks; the release profile doesn't, so release wrapped (256 -> 0) exactly like Zig's ReleaseFast.SemverQuery::parseusedlet mut count: u8 = 0;withcount += 1;— the 256th increment overflows.countis only ever compared== 0, never stored or cast, so a wider type is strictly more correct (it also avoids Zig's silent u8 wrap, which was a latent bug that merely didn't crash).Widen
counttou32. Regression test (Bun.semver.satisfieswith 300||comparators, run in a subprocess since the failure was an uncatchable abort) passes on the debug build and matches released Bun.Also carried:
wtf-bindings.cpp:assert→ASSERT— identical to wtf-bindings: use WTF ASSERT() so debug builds compile #30992; the WebKit upgrade in Upgrade WebKit to 2b257999 #30705 dropped the transitive<cassert>include, so debug builds currently fail to compile onmain. Needed here so the regression test can run underbun bd.failures does not cause weird memory issuestest (1e5 iterations) and the new subprocess test — both exceed the 5s default under debug+ASAN.Verification
With the
u32change reverted, the new test fails on debug withpanic: attempt to add with overflow (src/semver/SemverQuery.rs:981)on stderr and exit 132.