fix(base64): don't overflow on VLQ::encode(i32::MIN) in the crash handler path - #31861
Conversation
encode_slow_path negates the value to get its magnitude, which overflows for i32::MIN: debug builds panic with "attempt to negate with overflow" and release builds wrap. The crash handler reaches this by bitcasting raw u32 address halves to i32 in write_u64_as_two_vlqs, so a backtrace address half of exactly 0x80000000 panics the panic hook and aborts the process before the crash report is written. Compute the magnitude with unsigned_abs() instead. i32::MIN still has no sign-magnitude representation in a 32-bit VLQ, so it keeps wrapping to "-0" (the existing release behavior and wire format), but no longer panics. Adds VLQ unit tests covering the roundtrip domain and i32::MIN.
|
Warning Review limit reached
More reviews will be available in 15 minutes and 26 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line unsigned_abs() swap that matches release-build behavior and stops the debug-mode panic, with regression tests.
Extended reasoning...
Overview
This PR changes a single expression in encode_slow_path in src/base64/lib.rs: the negative branch of the sign-magnitude VLQ encoding switches from ((-value << 1) | 1) as u32 to (value.unsigned_abs() << 1) | 1. It also adds a small #[cfg(test)] module with a roundtrip test over the representable domain (including i32::MAX / i32::MIN + 1, pinning the documented "+/////D" / "//////D" encodings) and a test that VLQ::encode(i32::MIN) no longer panics and decodes to 0.
Correctness
For every negative value other than i32::MIN, value.unsigned_abs() equals (-value) as u32, so (unsigned_abs() << 1) | 1 produces the identical u32 the old expression produced — encodings are byte-for-byte unchanged. For i32::MIN, the old debug build panicked on -value; the old release build wrapped -i32::MIN → i32::MIN, then << 1 → 0, | 1 → 1 ("-0"). The new code yields 0x80000000u32 << 1 → 0, | 1 → 1, i.e. exactly the release-build result, now consistently in both profiles and without panicking inside the crash handler. unsigned_abs() is const-stable, so the const fn and the comptime VLQ_LOOKUP_TABLE are unaffected.
Security risks
None. This is pure integer arithmetic in a leaf encoding helper; no I/O, allocation, auth, or user-controlled data paths are touched. The only behavioral change is that a debug-build nested panic in the crash handler is avoided.
Level of scrutiny
Low. The diff is one functional line plus tests and a comment, in a self-contained encoder with no external side effects. The PR description precisely explains the overflow, the wire-format invariance, and why i32::MIN degrading to "-0" is the intended (and pre-existing release) behavior.
Other factors
The bug-hunting system found no issues. New unit tests cover both the regression and the unchanged roundtrip behavior, and bun_base64 is in MIRI_CRATES so CI exercises them under miri. No CODEOWNERS entry covers src/base64/, and there are no outstanding reviewer comments.
Crafts an InternalSourceMap blob whose first sync entry starts at generated column i32::MIN, driving VLQ::encode(i32::MIN) through the existing internal-for-testing toVLQ surface. Fails on the unfixed encoder with "attempt to negate with overflow" in debug builds.
There was a problem hiding this comment.
LGTM — one-line unsigned_abs() fix for the i32::MIN negation overflow, with unit and regression tests pinning the behavior.
Extended reasoning...
Overview
This PR changes a single expression in encode_slow_path in src/base64/lib.rs: the negative-branch magnitude computation ((-value << 1) | 1) as u32 becomes (value.unsigned_abs() << 1) | 1. It adds an explanatory comment, two Rust unit tests in the vlq module (a roundtrip over the representable domain including i32::MAX/i32::MIN+1, and an encode(i32::MIN) no-panic check), and a JS regression test that drives VLQ::encode(i32::MIN) through the bun:internal-for-testing toVLQ surface in a child process.
Correctness
For every negative value other than i32::MIN, value.unsigned_abs() equals (-value) as u32, so (mag << 1) | 1 produces the identical bit pattern as before — the encoding is unchanged across the entire representable domain. For i32::MIN, the old -value panicked in debug (and wrapped in release); the new path yields 0x80000000u32 << 1 = 0 then | 1 = 1, i.e. the "-0" encoding ("B"), which is exactly what release builds already emitted and what bun.report already decodes. unsigned_abs() is a const fn, so encode_slow_path remains const and the 0..=255 lookup-table const-eval is unaffected. I confirmed the crash-handler caller (write_u64_as_two_vlqs in src/crash_handler/lib.rs) does bitcast u32 address halves to i32 before calling VLQ::encode, making i32::MIN reachable as described.
Security risks
None. This is a pure arithmetic change in a base64-VLQ encoder; no parsing of untrusted input, no auth/crypto/permissions, no new I/O. The only behavioral change is that a debug-build panic inside the panic hook becomes a defined value.
Level of scrutiny
Low-to-moderate. The production change is one expression with a well-understood, easily-verified semantic equivalence over the whole input domain minus one value, and that one value's new behavior matches existing release behavior. The rest is tests and comments. No CODEOWNERS cover these paths, the bug hunter found nothing, and there are no outstanding reviewer comments.
Other factors
The added Rust unit tests pin the documented extreme encodings (+/////D / //////D) and the i32::MIN wrap, and the JS test runs in a child process so a regression would fail the test rather than abort the runner. The JS test depends on the InternalSourceMap blob layout, but that's a test-only concern — if the layout drifts the test fails visibly, it doesn't affect production code.
2 participants
Problem
Fuzzilli hit a nested panic while Bun was already processing a crash in a debug build:
The crash handler encodes backtrace addresses into the bun.report trace string by splitting each u64 into two u32 halves and bitcasting them to
i32(write_u64_as_two_vlqsinsrc/crash_handler/lib.rs).vlq::encode_slow_pathcomputes the magnitude of negative values with-value, which overflows fori32::MIN. So an address half of exactly0x80000000panics the panic hook: debug builds abort before the crash report or backtrace is written, and release builds silently wrap. The Zig reference (src/sourcemap/VLQ.zig) has the same checked negation, so this was inherited by the port; sourcemap callers never passi32::MIN, only the crash handler's bitcast address halves do.Fix
Compute the magnitude with
value.unsigned_abs()inencode_slow_path(src/base64/lib.rs). The sign-magnitude VLQ format cannot representi32::MINin 32 bits regardless (its magnitude is 2^31), so that one value keeps degrading to "-0" exactly as release builds already emit, and the wire format bun.report decodes is unchanged. Everything in the representable domain-(2^31 - 1)..=2^31 - 1encodes identically to before. The encoder just can't panic anymore, which matters because it runs inside the panic hook.Test
JS-level regression test in
test/js/bun/sourcemap/internal-sourcemap-roundtrip.test.ts:InternalSourceMapsync-entry state is raw i32 andappendVLQTocomputes deltas withsaturating_sub, so a hand-crafted blob whose first window starts at generated columni32::MINdrivesVLQ::encode(i32::MIN)through the existingbun:internal-for-testingtoVLQsurface. On the unfixed encoder the spawned process aborts with "attempt to negate with overflow" (debug builds); with the fix it emits the wrapped "-0" encoding (BAAA) and exits 0.Also unit tests in the
vlqmodule ofbun_base64: a roundtrip over the representable domain including both extremes (pinning the documented"+/////D"/"//////D"encodings), andencode(i32::MIN)decoding to 0 without panicking. The latter fails on the unfixed code with the same overflow panic (cargo test -p bun_base64).bun_base64is inMIRI_CRATES, so CI runs these viacargo miri teston changes undersrc/base64/; verified locally that miri passes.The crash handler trigger itself (a backtrace address half equal to
0x80000000) is not controllable from a test; the sourcemap path exercises the identical encoder edge.The primary crash in the fuzzer report (the
protection_countassert) is fixed separately in #31859.