Conversation
…mat 4) A trace string identified its build by platform char plus 7-char sha, which is not unique: one commit can be published as more than one link of the same platform (the x64 and x64-baseline zips of a commit, or a re-run release step), and bun.report had no way to tell which one a trace came from, so it symbolized against whichever debug file the platform char mapped to. Format 4 adds two fields after the sha: a build-flags VLQ (bit 0 = canary, replacing the 1/2 version-char split) and the id the linker stamps into both the executable and its debug info (PDB GUID, GNU build-id, LC_UUID) as a VLQ byte count plus lowercase hex, in the byte order the platform tools print. The id is read from the loader-mapped image headers inside the crash handler without allocating.
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 8:53 AM PT - Aug 15th, 2026
✅ @robobun, your commit 8120f233079e95c99b2e22212cac586c55850239 passed in 🧪 To try this PR locally: bunx bun-pr 38838That installs a local version of the PR into your bun-38838 --bun |
|
Status: fixed here (encoder) and in oven-sh/bun.report#31 (decoder + picking the build by id); blocked on #31 being deployed before this merges.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the trace-string wire format (with an explicit landing-order dependency on bun.report accepting '4' first) and adds ~250 lines of new platform-specific unsafe code that runs inside the crash handler across PE/ELF/Mach-O, a human should sign off on the format change and coordinate the merge with the companion bun.report PR.
Checked: every offset read in debug_id.rs is bounds-checked against the mapped image range and nothing allocates; BoundedArray::from_slice/Deref, bun_fmt::bytes_to_hex_lower, exe_image_range, LoadCommandIterator::cast, and callback_ctx all exist with matching signatures; tracePayload() pipes stdout without draining it but fixture-crash.js writes nothing there so it cannot deadlock; the test's independent on-disk PE/ELF/Mach-O readers use file offsets (PointerToRawData, p_offset) while the handler uses RVAs, so they genuinely cross-check rather than duplicate.
Extended reasoning...
Overview
This PR bumps the crash-handler trace-string format to version '4' and inserts two new fields between the 7-char sha and the features bitset: a build-flags VLQ (bit 0 = canary, replacing the old '1'/'2' version-char split) and the executable's debug id as a VLQ byte count followed by lowercase hex. The debug id is read at crash time from loader-mapped image headers by a new src/crash_handler/debug_id.rs (~250 lines): the PE CodeView RSDS GUID on Windows via exe_image_range(), LC_UUID on Mach-O via _dyld_get_image_header(0) + LoadCommandIterator, and the NT_GNU_BUILD_ID note on ELF via dl_iterate_phdr. encode_trace_string in lib.rs gains ~10 lines to emit the new fields. A ~200-line test in run-crash-handler.test.ts crashes via panic and segfault, independently reads the debug id from the executable file on disk (separate PE/ELF/Mach-O parsers using file offsets rather than RVAs), and decodes the full payload end-to-end to prove nothing after the new fields shifted.
Security risks
None user-facing. The new code only reads the process's own loader-mapped image headers; it accepts no external input. Every offset dereference in the PE and ELF-note walkers goes through bounds-checked .get(..) / checked_add, and the Mach-O path reuses the existing LoadCommandIterator::cast which length-checks before read_unaligned. The output is a fixed-size hex string written into a stack buffer ([0u8; 2 * MAX_LEN]), so there's no allocation inside the signal handler.
Level of scrutiny
High. This is (a) new unsafe code across three binary formats that runs inside a signal handler after the process is already in a bad state, and (b) a wire-format change to the trace string that every crash report carries. The PR description is explicit that bun.report must land its '4' decoder first or new canaries' reports stop parsing, and that the in-repo bun-tracestrings pin needs a follow-up bump. That landing-order coordination is a human decision, not something an automated review can green-light.
Other factors
The change is very well-executed: careful bounds-checking, SAFETY comments on every unsafe block, reuse of existing helpers (exe_image_range, LoadCommandIterator, callback_ctx, bytes_to_hex_lower, BoundedArray), and a strong test that cross-checks the in-memory reader against an independent on-disk reader and round-trips the reason payload. The PR description documents verification on Linux x64 and Windows x64 debug builds plus cargo check across all five target OSes. I found no correctness issues. The one candidate raised by finders — tracePayload() piping stdout without draining it — is not a problem because fixture-crash.js writes nothing to stdout. Deferring purely on scope (cross-platform unsafe crash-path code) and the stated external merge-order dependency, not on any defect.
|
The decoder side is up as oven-sh/bun.report#31: it parses format 4, reads the same id out of the On the review note about CI on 605e221: the two hard failures are both on the Windows 2019 x64 lane and unrelated to this diff. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
27a7178 trims the comments the comment linter flagged: debug_id.rs is down to the struct offsets it reads and the GUID byte-order note, the module doc is four lines, and the format table on |
…g a 2s sleep The Windows 2019 agents without AVX take longer than 2s to cold-start PowerShell, so the system-PowerShell upload test failed on every run of this file there while the three auto-reporter tests, which await the ack, passed. Same change as #36550.
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunt found no issues. Deferring to a human because this bumps the trace-string wire format (with an explicit cross-repo landing-order dependency on oven-sh/bun.report#31) and adds ~230 lines of new unsafe image-header parsing across three platforms inside the crash handler — both warrant a maintainer sign-off on timing and design.
What was reviewed:
encode_trace_stringformat-4 layout vs. the test decoder — build-flags VLQ and length-prefixed hex id sit between the sha and features without shifting later fields.- Bounds-checking in
debug_id.rs: PE RVA reads usechecked_addand sliceget(); ELF note walk aligns to 4 and clamps the tail; Mach-O goes throughLoadCommandIterator— none allocate. - The PowerShell test change replaces the 2s sleep race with awaiting the ack, matching the auto-reporter tests above it.
Extended reasoning...
Overview
This PR introduces trace-string format '4', which appends a build-flags VLQ and the executable's linker debug id (PDB GUID / GNU build-id / LC_UUID) after the 7-char sha in crash-report URLs. It adds a new src/crash_handler/debug_id.rs (~230 lines) that reads the id from the loader-mapped image headers on Windows (PE CodeView), macOS (Mach-O LC_UUID), and ELF (NT_GNU_BUILD_ID via dl_iterate_phdr), wires it into encode_trace_string in lib.rs, and adds a ~200-line test in run-crash-handler.test.ts that independently re-derives the id from the executable file on disk and decodes the full payload end-to-end. It also replaces a 2s-sleep race in the pre-existing PowerShell upload test with an awaited ack.
Security risks
Low. The new code only reads from the process's own read-only mapped image headers; there is no attacker-controlled input. Every offset dereference goes through bounds-checked slice get() / checked_add, and nothing allocates (fixed BoundedArray<u8, 20> + a 40-byte stack hex buffer), which is what the crash-handler context requires. The unsafe blocks are for from_raw_parts over loader-guaranteed mappings and each carries a SAFETY comment naming the guarantee.
Level of scrutiny
High. Three independent reasons: (1) the author states a hard landing-order requirement — bun.report#31 must be deployed first or new canaries' crash reports stop parsing, and the bun-tracestrings pin bump is still pending; a human needs to coordinate merge timing. (2) This is new unsafe binary-format parsing that runs after a crash, where a wrong offset would fault inside the handler and lose the report entirely — the ELF path's PT_NOTE-inside-PT_LOAD assumption and the PE AddressOfRawData != 0 guard are the kind of thing a maintainer familiar with these formats should eyeball. (3) It changes a wire format with an external consumer, replacing the '1'/'2' canary encoding with a flags VLQ.
Other factors
The test coverage is strong: it re-implements the id extraction independently (file offsets rather than mapped RVAs) for all three formats and round-trips the entire payload including the reason bytes, so a mis-encoded field would show up. The comment-cop bot flags were addressed in 27a7178 and the remaining ones are format-layout tables the author reasonably kept. No human reviewer has looked at this yet.
Instead of two fixed fields after the hash, the header is a VLQ field count followed by (tag, char count, chars) fields: tag 0 the build flags VLQ, tag 1 the debug id in hex. A decoder skips tags it does not know, so the next field added to a trace string needs neither a new version char nor a decoder deploy ahead of the emitter.
|
Two changes since the last round, both from going back over whether this PR is worth having in this shape; the body is rewritten to match.
Test coverage here is unchanged in kind: the new tests decode the header generically and require exactly the two fields, with the id checked against an independent read of the executable file. Still blocked on bun.report#31 deploying; the |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Deferring to a human because it introduces a new trace-string wire format ('4') with a cross-repo landing-order dependency (bun.report#31 must be deployed first, and the bun-tracestrings pin bump is still pending), and adds ~230 lines of new unsafe Rust that walks PE/ELF/Mach-O image headers inside the crash handler — both the format design and the merge timing want a maintainer sign-off.
What was reviewed:
debug_id.rs: bounds-checking on all header offset reads (checked_add, slice::get) — every deref is guarded; no allocation in the crash path.encode_trace_string: the new tagged-header encoding — field count matches emitted fields, and the features/frames/reason tail is unchanged, confirmed by the test decoding it end-to-end.- The independent on-disk PE/ELF/Mach-O readers in the test vs. the in-memory readers — same byte-order handling for the PDB GUID's little-endian fields.
- The PowerShell test change replaces the 2s sleep race with the same
await acked.promisepattern the auto-reporter tests already use.
Extended reasoning...
Overview
This PR bumps the crash-handler trace-string format to '4' and inserts a tagged header after the 7-char sha: a VLQ field count followed by (tag, char-count, chars) triples, currently carrying build flags (bit 0 = canary, replacing the '1'/'2' split) and the executable's debug id in lowercase hex. A new src/crash_handler/debug_id.rs reads that id from the loader-mapped image headers — the PE CodeView RSDS GUID on Windows, the NT_GNU_BUILD_ID note on ELF, LC_UUID on Mach-O — with every offset bounds-checked and no allocation. test/cli/run/run-crash-handler.test.ts gains a describe block that crashes via panic and segfault, decodes the emitted URL byte-by-byte, and compares the id against an independent on-disk read of the executable file. It also fixes the pre-existing PowerShell upload test to await the ack instead of racing a 2s sleep.
Security risks
None user-facing. The new code reads the process's own image headers (already mapped read-only by the loader) and emits a hex string into the crash URL. The unsafe blocks are carefully scoped: from_raw_parts over ranges the loader guarantees mapped, with all subsequent reads going through bounds-checked .get() / checked_add. The worst case for a malformed header is None, which just omits the debug-id field. This runs post-crash, so it does not expand the attack surface of a healthy process.
Level of scrutiny
High. This is not a mechanical change: it defines a new extensible wire format that an external service (bun.report) must decode, with an explicit landing-order constraint the author calls out — bun.report#31 must be deployed before this merges or new canaries' reports stop parsing, and the in-repo bun-tracestrings pin still needs bumping. The tagged-header design (skip-unknown-tags forward compatibility) is a real API decision a maintainer should ratify. The debug-id readers are ~230 lines of platform-conditional unsafe code that run inside a signal handler after a crash; while the bounds-checking looks thorough and the tests cross-validate against an independent file-based reader, this is exactly the kind of code that benefits from a second pair of eyes on the PE/ELF/Mach-O offset arithmetic.
Other factors
The tests are strong: they decode the full payload (header, features, frame list, reason) to prove nothing after the new fields shifted, and read the id independently from disk on all three platforms. CI passed on darwin (Mach-O) and the author verified Windows and Linux manually against llvm-readobj/llvm-pdbutil/readelf -n. The one open comment-cop flag on lib.rs:2407 targets the VERSION_CHAR format table, which the author has already argued (and I agree) is documenting the wire format rather than justifying a workaround. Given the cross-repo coordination and the wire-format design decision, a human should confirm the bun.report side is deployed and sign off on the format shape before merging.
…g platform characters (#39801) Merge after oven-sh/bun.report#32 is deployed. bun.report rejects the new characters until then. ### Problem - `Platform::CURRENT` (src/crash_handler/lib.rs) emits `'l'`/`'L'` for every Linux build. The glibc, musl and android builds of a commit are three binaries with three profile zips, and bun.report picks one by this character. Every musl or android crash report is symbolicated against the glibc binary and shows plausible but wrong frames. - BUN-4MRJ is one: reported as a segfault in `NestedRuleParser::parse_block` (`css_parser.rs:1738`), it is a `VariableEnvironment` lookup on a bad `UniquedStringImpl*` against the musl profile (notes). ### Fix - musl builds emit `'u'`/`'U'`, android builds `'a'`/`'A'`, and `'l'`/`'L'` mean glibc. Lowercase is x86_64, uppercase aarch64. bun.report#32 decodes them. - Shipped binaries keep emitting `'l'`/`'L'`, which bun.report keeps decoding as glibc. - The format is unchanged. #38838 (a debug id in a new format) verifies the downloaded file instead. The two compose: here the first download is right, and the URL says which build crashed. - Verified: CI build 101799 ran an assertion on the Alpine x64 and aarch64 lanes that the build emits `'u'`/`'U'`. It passed and was then removed at review, since it only mirrored the table. Also `cargo check -p bun_crash_handler` for gnu, musl and android, and clippy. ### Background - Trace string: the `https://bun.report/<version>/<payload>` URL in a crash report. The payload starts with the platform character, then command character, format version and sha. bun.report maps the character to an artifact name. - The baseline characters (`'B'`, `'b'`, `'e'`) did the same for the old baseline binaries until #34782 made x64 one binary. <details><summary>Notes</summary> The human-readable report header already says `musl` or `Android ... bionic`; only the trace string lacked the distinction. BUN-4MRJ, trace `1.4.0/L_134cbb9aEggggC+98pvDA2Dhggw6jC` (commit `34cbb9a40`), one address `0x37a79df`, fault address `0xFFFFFFFFBC2C0000`. `llvm-symbolizer --inlines --relative-address` against the 1.4.0 release profiles: - `bun-linux-aarch64-profile.zip` (what bun.report used): `<NestedRuleParser<BundlerAtRuleParser> as AtRuleParser>::parse_block`, `src/css/css_parser.rs:1738`. - `bun-linux-aarch64-musl-profile.zip`, fault pc `0x37a79e0`: `WTF::InlineMap<PackedRefPtr<UniquedStringImpl>, JSC::VariableEnvironmentEntry, 9>::findKeyOrEmpty<const UniquedStringImpl*>` (`InlineMap.h:725`) > `findKeyOrEmptyInStorage` (`InlineMap.h:703`) > `JSC::IdentifierRepHash::hash` (`Identifier.h:235`) > `SymbolImpl::existingSymbolAwareHash` > `StringImpl::isSymbol` (`StringImpl.h:334`). That is the load of `m_hashAndFlags` (offset 0x10) through the lookup key, so the key the caller passed was `0xFFFFFFFFBC2BFFF0`: a user-space pointer truncated to 32 bits and sign-extended. The scope had more than 9 variables (out-of-line storage). The trace has no caller frame, so the report says nothing more about the crash. It is not a CSS crash. It is almost certainly a musl build: the musl reading accounts for the fault address exactly (key + 0x10), the glibc reading needs a frame pointer that changed between two adjacent loads, and in the android build the address is data (`.text` ends at 0x32ea060). Which byte values: `'u'`/`'U'` and `'a'`/`'A'` are unused in bun.report's table (`w e W m b M l B L f F`). The reason and command characters are separate positions, so they do not constrain the choice. `scripts/ci-remap-server` pins `bun-tracestrings` at bun.report 912ca63, which does not decode these characters (nor the `'a'`/`'b'` reasons or FreeBSD from #29 and bun.report#29). A crash on the Alpine CI lanes prints the raw trace string in the annotation instead of a remap until the pin is bumped, as SIGABRT and SIGTRAP crashes do on every lane today. #38838 bumps the pin as part of its landing. </details>
This branch has not been deployed
2 participants
Blocked on oven-sh/bun.report#31 (the decoder) being deployed first; see Landing order below.
Problem
encode_trace_string, src/crash_handler/lib.rs). That is not a binary.Platform::CURRENTonly encodes os and arch, so the glibc, musl and android builds of a commit all report'l'/'L', while .buildkite/scripts/upload-release.sh publishes all three per arch (bun-linux-x64-profile.zip,-musl-profile.zip,-android-profile.zip), and bun.report downloads the glibc one for every'l'trace. Every musl or android crash report is remapped against a different binary's symbols today, and the output looks like a normal remap.-baselinebinary is where the Windows x64 reports a downstream build of commit8bb8d04c4sends come from (BUN-4B1Q, BUN-4BST, BUN-4B64, BUN-4BW6, BUN-4B2Y, about 60 events a day): both of its Windows x64 links report'w', and symbolized against the first link's PDB every frame lands mid-instruction and the groups read as date/time-zone crashes; against the right PDB they are a GC marking-thread crash and anIntl.Segmentercrash (evidence in crash_handler(windows): unwind through LLInt and vmEntryToJavaScript frames and stop the walk at non-code PCs #38789). Upstream CI has shipped one x64 binary under both names since ci: single arm64 debian-13 build host; ThinLTO everywhere; baseline-only x64; rust+link merge; sysroots; WebKit a36c188; rust 2026-07-20 #34782, so re-emitting the old baseline platform characters would fix neither case.Fix
'4': after the sha, a header of tagged fields: a VLQ field count, then per field a VLQ tag, a VLQ character count and that many characters (HeaderField,write_header_field). Tag 0 is the build flags VLQ (bit 0 = canary, replacing the'1'/'2'version-character split), tag 1 the executable's debug id in lowercase hex, omitted when the executable has none. Everything after the header is unchanged.NT_GNU_BUILD_IDnote on ELF,LC_UUIDon Mach-O. Every offset is bounds-checked and nothing allocates. The bytes are emitted in the order the platform's tools print them, so the hex in a URL compares directly withreadelf -n,llvm-readobj --coff-debug-directory/dumpbin, ordwarfdump --uuidoutput.bun-tracestringspin in package.json thatscripts/runner.node.mjsuses for the CI remap server (already a year behind: it predates the'a'/'b'reasons and the FreeBSD characters); it gets bumped to the bun.report commit in this PR once addnpm installcommand before running bun #31 lands, so both consumers switch in the same merge. Open PRs whose tests decode trace strings positionally (crash_handler(windows): unwind through LLInt and vmEntryToJavaScript frames and stop the walk at non-code PCs #38789, crash_handler: symbolize frame 0 of a fault trace at the fault pc, not one byte before it #37533) will need to read past the header once this lands; sys/exe_format: LoadCommand borrows its bytes; patch commands by offset #37569 changes theLoadCommandIteratorAPI debug_id.rs uses, so whichever lands second adapts.trace string identifies the build: crashes viapanicandsegfault, checks the version character and sha, decodes the header generically and requires exactly a build-flags field matchinggetFeatureData().is_canaryand a debug-id field equal to an independent read of the executable file (ELF program headers, PE debug directory viaPointerToRawDatawhere the handler uses the RVA, Mach-O load commands), then decodes the features, frame list and reason payload to show nothing after the header moved. Fails on the current build (Expected: "4", Received: "2"), passes withbun bd test(debug + ASAN) on Linux x64.944668034eead8624c4c44205044422e) equals thePDBGUIDllvm-readobj prints forbun-debug.exeand theGUIDllvm-pdbutil prints forbun-debug.pdb. The darwin CI lanes passed the test as well, covering the Mach-O reader.Build IDline ofreadelf -n; the stripped release binary carries the same note asbun-profile(-Wl,--build-id=sha1is unconditional in scripts/build/flags.ts), which is what lets bun.report match a user's binary to the profile artifact. The trace from this build is a parse fixture in bun.report#31.cargo check -p bun_crash_handlerfor the linux, windows-msvc, apple-darwin, freebsd and android targets;cargo clippyclean on linux.Background
https://bun.report/<version>/<payload>URL the crash handler prints and uploads. The payload is positional: platform character, command character, format version character, 7-character sha, then VLQ-encoded fields (features bitset, image-relative frame addresses, reason). bun.report'slib/parser.tsdecodes it and downloads the commit's debug file to symbolize the addresses;scripts/runner.node.mjsruns the same decoder locally against the binary under test.RSDS) record referenced from the debug data directory and repeats it in the PDB; ELF keeps a hash of the output in a.note.gnu.build-idnote inside aPT_NOTEsegment; Mach-O keeps a UUID in anLC_UUIDload command that the dSYM repeats. All three live in the image headers, which the loader maps read-only, so they can be read after a crash without trusting heap state.-baselinecopies of the x64 zips that have been the same binary since ci: single arm64 debian-13 build host; ThinLTO everywhere; baseline-only x64; rust+link merge; sysroots; WebKit a36c188; rust 2026-07-20 #34782 (they used to be a separate no-AVX build with its own platform characters, and some downstream trees still build one).