Conversation
|
Updated 4:54 PM PT - Jul 22nd, 2026
❌ @autofix-ci[bot], your commit 6ab1d4a has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34814That installs a local version of the PR into your bun-34814 --bun |
WalkthroughChangesBun’s crash-handler trace strings now use v3/v4 formats with optional fault register blocks. Native handlers capture registers, JavaScript testing bindings expose a deterministic register-bearing crash, and CLI tests validate fault and non-fault encodings. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/crash_handler/lib.rs (1)
1677-1834: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftAdd direct tests for the raw register readers
The synthetic segfault path is covered, but the rawucontext_t/CONTEXTreaders still have no direct tests. Add a small test-only shim that feeds known register blobs intofault_context_from_ucontext/fault_context_from_windows_contextand assertsFaultRegisters::values(); that would catch a bad offset on any supported platform.🤖 Prompt for 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. In `@src/crash_handler/lib.rs` around lines 1677 - 1834, Add platform-specific direct tests for fault_context_from_ucontext and fault_context_from_windows_context using test-only contexts or register blobs populated with known values, then assert the resulting FaultRegisters::values() in the documented register order. Cover every supported Unix and Windows architecture without affecting production code, so incorrect ucontext/CONTEXT offsets are detected.
🤖 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.
Outside diff comments:
In `@src/crash_handler/lib.rs`:
- Around line 1677-1834: Add platform-specific direct tests for
fault_context_from_ucontext and fault_context_from_windows_context using
test-only contexts or register blobs populated with known values, then assert
the resulting FaultRegisters::values() in the documented register order. Cover
every supported Unix and Windows architecture without affecting production code,
so incorrect ucontext/CONTEXT offsets are detected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4d0b004-304f-431a-bd5b-3efc43379974
📒 Files selected for processing (5)
src/crash_handler/lib.rssrc/js/internal-for-testing.tssrc/runtime/api/crash_handler_jsc.rstest/cli/run/fixture-crash.jstest/cli/run/run-crash-handler.test.ts
|
Pushed bf017c5 to address the clippy On the review suggestion to add direct unit tests for the raw |
![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/crash_handler/lib.rs:2869-2885— Appending the register block unconditionally after the reason payload breaks decodability for reasons'0'(Panic) and'8'(ZigError), whose payloads were end-of-string-terminated in v1/v2 with no length prefix. A v1/v2 decoder that (per this PR's stated compat contract) reads the reason payload to end-of-string will now see<b64>_A/<name>_Aand fail; and for a v3 decoder, reason'8'is genuinely ambiguous because_andAare valid identifier characters — which contradicts the "decoder does not have to branch on which reason codes carry one" comment. Consider restricting the register block to the self-delimiting fault reasons (2–5), or add an out-of-alphabet delimiter / length prefix before it.Extended reasoning...
What the bug is
encode_trace_stringnow unconditionally appends a register block after every reason payload (lib.rs:2869-2885). Two reason encodings have no self-delimiting length — in v1/v2 they were terminated by end-of-string:CrashReason::Panic(reason byte'0', lib.rs:2795-2836) writes standard-alphabet base64 (A-Za-z0-9+/) of the zlib-compressed message, with=padding stripped at line 2836.CrashReason::ZigError(reason byte'8', lib.rs:2861-2864) writes the raw error-name bytes with no length prefix or terminator.
Both call sites pass
regs: Nonetoday, so the appended suffix is_A(StackLine::write_encoded(None)→_at lib.rs:2710, thenVLQ::ZERO→A).Why this is a wire-format problem
The v1/v2 backward-compat claim is false for these two reasons
The
VERSION_CHARdoc (lib.rs:2543-2548) and the PR description both say "a decoder that only knows '1'/'2' can treat '3'/'4' the same and stop at the end of the reason payload; everything after it is additive". That only works for reason payloads that are self-delimiting (1/6/7/9have zero trailing bytes;2-5have exactly two VLQs). For'0'and'8', the v1/v2 contract is "read to end of string" — there is nothing else for a v1/v2 decoder to stop on. Such a decoder now reads<b64>_Aas the panic base64 (invalid:_is not in the standard alphabet, and the trailingAshifts the length mod 4) or<name>_Aas the error name.Reason
'8'is ambiguous even for a v3-aware decoderFor a v3 decoder following the documented format (reason payload → StackLine → VLQ count → count×2 VLQs),
'8'cannot be forward-parsed:_andAare both valid Rust/Zig identifier characters, so8DumpStackTrace_Acannot be split intoname="DumpStackTrace"+ regs=_Avs.name="DumpStackTrace_A"+ without out-of-band knowledge. The only way to decode it is to hard-code that reason'8'always hasregs: Noneand strip a fixed_Asuffix — which directly contradicts the code comment at line 2869-2871 ("Always present so the decoder does not have to branch on which reason codes carry one") and is unenforced by types (nothing prevents a future call site from passingregs: SomewithZigError, at which point the format is unrecoverable — StackLine/VLQ bytes overlap the identifier alphabet).Panic is technically delimitable by a v3 decoder because
_is outside the standard base64 alphabet — but that still requires the decoder to special-case reason'0'("scan for the first non-base64 byte"), again contradicting the "no branching" design goal.Step-by-step proof (ZigError)
dump_stack_trace/handle_root_errorconstructTraceString { reason: CrashReason::ZigError(b"DumpStackTrace"), regs: None, .. }(lib.rs:2152-2156, 3288-3292).encode_trace_stringwrites…b'8'b"DumpStackTrace"(lib.rs:2861-2864).opts.regsisNone→ writesb"_"(lib.rs:2874, 2710) thenVLQ::ZERO=b"A"(lib.rs:2875).- Emitted tail:
…8DumpStackTrace_A(plus optional/view). - A v1/v2 decoder reading reason
'8'payload as "rest of body" gets error nameDumpStackTrace_A. - A v3 decoder scanning forward for the register-block start cannot distinguish this from an error literally named
DumpStackTrace_Awith a missing register block.
Impact
The comment at lib.rs:2793 ("must be kept in sync with bun.report's decoder") confirms there is an external consumer of this wire format. Panic is the common Rust panic path and ZigError is emitted by
handle_root_error/dump_stack_trace, so both are exercised in production. Shipping this format means either bun.report's decoder must special-case reasons'0'/'8'(contradicting the stated design), or those crash types become undecodable / mis-decoded.Suggested fix
Either:
- Emit the register block only for the self-delimiting fault reasons (
2/3/4/5, optionally6/7) — those are the only ones that ever carry a real fault context anyway; or - Prefix the register block with an out-of-alphabet delimiter (a byte that cannot appear in standard base64 or an identifier — e.g.
=or~), or length-prefix reason'8''s error name.
Per REVIEW.md ("For protocols, derive behavior from the spec"), the wire format should be unambiguous by construction rather than relying on the current invariant that Panic/ZigError call sites happen to pass
regs: None.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/crash_handler/lib.rs (1)
1799-1884: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHand-coded Windows
CONTEXToffsets are correct, but remain untested.I independently verified these offsets against the documented
winnt.hlayouts: for x64CONTEXT,Raxat0x78throughRipat0xF8match Microsoft's published field order; forARM64_NT_CONTEXT,Fp/X29at0xF0,Spat0x100, andPcat0x108also match. So the current values are accurate.That said, per the PR discussion this path has no unit test (a fabricated
CONTEXTtest was explicitly deferred), meaning a future edit to these magic offsets could silently corrupt register capture with nothing to catch it — the only coverage is the POSIX-onlysegfaultWithRegistersfixture and cross-targetcargo check. Worth tracking as follow-up test debt even though the offsets check out today.🤖 Prompt for 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. In `@src/crash_handler/lib.rs` around lines 1799 - 1884, Add targeted Windows architecture-specific tests for fault_context_from_windows_context using fabricated CONTEXT memory layouts, validating the documented register offsets, extracted pc/fp values, register ordering, and count for both x86_64 and aarch64. Keep the existing fallback behavior and ensure the tests are gated to their respective Windows targets.
🤖 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.
Outside diff comments:
In `@src/crash_handler/lib.rs`:
- Around line 1799-1884: Add targeted Windows architecture-specific tests for
fault_context_from_windows_context using fabricated CONTEXT memory layouts,
validating the documented register offsets, extracted pc/fp values, register
ordering, and count for both x86_64 and aarch64. Keep the existing fallback
behavior and ensure the tests are gated to their respective Windows targets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54813686-1457-4848-a39b-aff2ce4e82d9
📒 Files selected for processing (3)
src/crash_handler/lib.rssrc/runtime/api/crash_handler_jsc.rstest/cli/run/run-crash-handler.test.ts
|
Addressed in f354cdb:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/cli/run/run-crash-handler.test.ts (1)
158-171: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDecode and assert the deterministic register block.
The length check and incidental
"I"search do not prove the count or values:"I"may occur in the encoded PCStackLine, and malformed register data can still meet the size threshold. Decode the v3 tail and assert the fixture PC, count4, and all four synthetic values.As per coding guidelines, tests must “assert the strongest invariant” and “prove they fail for the intended reason.”
🤖 Prompt for 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. In `@test/cli/run/run-crash-handler.test.ts` around lines 158 - 171, Replace the length and incidental “I” assertions around reasonIdx/afterReason with deterministic decoding of the v3 tail. Use the existing payload/stack-trace decoder to parse the fault StackLine and register block, then assert the fixture PC, register count 4, and all four expected synthetic register values. Keep the segfault-reason presence check, and ensure assertions fail when the encoded count or register values are malformed.Source: Coding guidelines
🤖 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 `@src/crash_handler/lib.rs`:
- Around line 2617-2624: Condense the changed comments to three lines or fewer
while preserving their meaning: in src/crash_handler/lib.rs lines 2617-2624,
summarize the v3 format description; in lines 673-676, summarize the
hardware-fault classification rationale; in lines 2946-2951, summarize encoder
compatibility; and in test/cli/run/run-crash-handler.test.ts lines 194-197,
summarize the non-fault suffix rationale. Make no code changes.
---
Outside diff comments:
In `@test/cli/run/run-crash-handler.test.ts`:
- Around line 158-171: Replace the length and incidental “I” assertions around
reasonIdx/afterReason with deterministic decoding of the v3 tail. Use the
existing payload/stack-trace decoder to parse the fault StackLine and register
block, then assert the fixture PC, register count 4, and all four expected
synthetic register values. Keep the segfault-reason presence check, and ensure
assertions fail when the encoded count or register values are malformed.
🪄 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: 6136bad5-ebc5-43fc-ab2d-bf4d69d534b7
📒 Files selected for processing (2)
src/crash_handler/lib.rstest/cli/run/run-crash-handler.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/crash_handler/lib.rs (2)
2950-2955: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not include raw register contents in default crash reports.
Line 2953 serializes every captured GP register into a trace string that is printed and reported. Registers can retain secrets, user data, and ASLR-sensitive pointers, contradicting the surrounding “redacted crash report” behavior. Make full register capture explicitly opt-in and emit the empty context block by default.
🤖 Prompt for 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. In `@src/crash_handler/lib.rs` around lines 2950 - 2955, Change the crash-report serialization around StackLine::write_encoded so captured register values are emitted only when full register capture is explicitly enabled. By default, write the empty context block instead of iterating over regs.values(), preserving the existing trace format while preventing raw register contents from entering redacted reports.
868-871: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep changed comments to three lines or fewer.
src/crash_handler/lib.rs#L868-L871: condense theFaultRegistersoverview.src/crash_handler/lib.rs#L920-L923: condense the testing-constructor rationale.src/crash_handler/lib.rs#L940-L943: condense the borrowed-register lifetime rationale.src/crash_handler/lib.rs#L1703-L1707: condense the POSIX context-extraction description.As per coding guidelines, “Keep code comments to three lines or fewer.”
🤖 Prompt for 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. In `@src/crash_handler/lib.rs` around lines 868 - 871, Condense the comments for FaultRegisters and the related testing-constructor, borrowed-register lifetime, and POSIX context-extraction descriptions to no more than three lines each. Apply the comment-only changes at src/crash_handler/lib.rs lines 868-871, 920-923, 940-943, and 1703-1707, preserving their existing meaning.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/crash_handler/lib.rs`:
- Around line 2950-2955: Change the crash-report serialization around
StackLine::write_encoded so captured register values are emitted only when full
register capture is explicitly enabled. By default, write the empty context
block instead of iterating over regs.values(), preserving the existing trace
format while preventing raw register contents from entering redacted reports.
- Around line 868-871: Condense the comments for FaultRegisters and the related
testing-constructor, borrowed-register lifetime, and POSIX context-extraction
descriptions to no more than three lines each. Apply the comment-only changes at
src/crash_handler/lib.rs lines 868-871, 920-923, 940-943, and 1703-1707,
preserving their existing meaning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17222066-a81e-4c39-9cbd-82e51681da03
📒 Files selected for processing (2)
src/crash_handler/lib.rstest/cli/run/run-crash-handler.test.ts
|
On the two latest coderabbit findings:
|
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/crash_handler/lib.rs:2921-2937— Appending the register block after the reason payload breaks the two variable-length reasons: for'8'(ZigError) the rawerr_namehas no length prefix and_is a valid identifier byte, so a v3 decoder cannot locate the name/register-block boundary; for'0'(Panic) a v1 decoder reading base64-to-end now sees<b64>_Aand_is not in the standard base64 alphabet, so the "treat '3' as '1'" doc claim atVERSION_CHARis false. Length-prefixerr_name(and the Panic base64) in v3, or place the register block before the reason byte, before this format ships to bun.report.Extended reasoning...
What
encode_trace_stringappends the new register block (src/crash_handler/lib.rs:2921-2937) immediately after the existing reason payload with no delimiter or length prefix. Two reason codes have variable-length payloads that in v1/v2 were defined as "read to end of body":'0'(CrashReason::Panic, lines 2847-2888):'0'+ standard-alphabet base64 of zlib(message),=padding stripped.'8'(CrashReason::ZigError, lines 2913-2915):'8'+err_nameraw bytes.
All other reason codes (
'1'..'7','9') have zero-length or self-terminating (two-VLQ) payloads and are unaffected.Why the v1/v2-compat claim is false
The
VERSION_CHARdoc comment (lines 2595-2597) and the PR description both state that a decoder that only knows'1'/'2'"can treat'3'as'1'and stop at the end of the reason payload; everything after it is additive". But'0'and'8'never had an "end of the reason payload" — v1 decoders read to end-of-body (before the optional/view). Feeding them a v3 body:- ZigError:
js_root_errorcallshandle_root_error("Unexpected", None)→ v3 body ends…8Unexpected_A. A v1 decoder reads the error name asUnexpected_A. - Panic:
bun_base64::encodeuses the simdutf standard alphabet (A-Za-z0-9+/,url_safe=false). v3 body ends…0<b64>_A._is not in that alphabet, so a strict v1 decoder rejects the base64 and the panic message is lost; a URL-safe-tolerant one decodes garbage into the zlib stream.
Why v3 is ambiguous for
'8'The comment at lines 2921-2923 says the block is "Always present so the decoder does not have to branch on which reason codes carry one". But for
'8'a v3 decoder cannot locate the boundary without branching:_is a valid identifier byte.ErrNameis impl'd for&str/&[u8]andhandle_root_erroraccepts any error name; Rust variant names and the existingb"DumpStackTrace"happen not to contain_, but nothing enforces that.- If
regswereSome, the block would begin with aStackLine(VLQ bytes fromA-Za-z0-9+/) — every one of which is also a valid identifier byte.8SomeErrorBabc…gives no way to tell whetherBstarts the pc StackLine or continues the error name.
Today it happens to work only because every
ZigErrorcall site (lines 2205, 3271, 3341) passesregs: None, so the suffix is always exactly_Aand a decoder could special-case "for reason'8', strip the last two bytes". That directly contradicts the stated design and is not enforced anywhere in the encoder.Step-by-step proof
Take
rootErrorfromfixture-crash.js:js_root_error→handle_root_error("Unexpected", None)→CrashReason::ZigError(b"Unexpected"),regs: None.encode_trace_stringwrites…A(frames terminator) +8+Unexpected(line 2915).opts.regs == None→ writes_(StackLine::write_encoded(None)) +A(VLQ::ZERO) (lines 2925-2928).- Body ends
…8Unexpected_A. - A v1 decoder for reason
'8'reads bytes after'8'until end-of-body → error name ="Unexpected_A". Wrong. - A v3 decoder that follows the "decoder does not have to branch" comment scans for the register block after the reason payload — but has no length for the name and no sentinel outside the identifier charset to stop at.
For Panic, the same trailing
_Afollows the base64;_is outsideA-Za-z0-9+/, so v3 can re-delimit it, but v1 cannot, and the doc comment promises it can.Impact
This is a wire-format defect being introduced by this PR. bun.report is the consumer; whatever workaround it adopts ("for
'8'strip trailing_A", "for'0'stop base64 at first non-alphabet byte") is baked in for the life of v3/v4 and constrains future changes (e.g., ever passingregs: Someon aZigErrorpath becomes silently undecodable). REVIEW.md, One source of truth: "any change to cached/serialized output bumps the format version constant" — the version was bumped, but the format itself is not self-delimiting for two of its ten reason codes.Fix
Any of:
- In v3, write a VLQ length before
err_name(and before the Panic base64), matching how the fault-address and register-count fields are already self-delimiting. - Move the register block to before the reason byte (immediately after the frames terminator
A), so every reason payload is again terminal. - Emit an explicit sentinel byte between reason payload and register block that is outside both the standard base64 alphabet and identifier charset (e.g.
~or!), and drop the v1-compat claim for'0'/'8'from theVERSION_CHARcomment.
|
The latest claude bot 🔴 finding (wire-format ambiguity for reasons |
|
Closing the loop on the register-privacy finding: register values stay encoded unconditionally. Upload is already gated by the existing telemetry opt-in ( |
There was a problem hiding this comment.
No bugs found this run; the earlier nits are all addressed in 263cd47. Deferring to a human for the wire-format bump (bun.report needs a matching v3/v4 decoder) and the register-privacy question you flagged as needing maintainer sign-off.
What was reviewed:
- Wire-format ambiguity: register block is now gated on
is_fault()so reasons '0'/'1'/'8'/'9' stay byte-identical to v1 — verified againstencode_trace_stringand the OOM test. - Signal-handler safety:
TraceSeed::Faultborrows the stack-residentFaultRegisters, no allocation on the fault path; buffer bumped to 1536 for the aarch64 33-register tail. FaultRegisters::COUNT/NAMESare tied at compile time via the module-scopeconst _assert; per-arch readers assignr.count = COUNT.- Windows
CONTEXToffsets checked against winnt.h layout (x64 Rax@0x78..Rip@0xF8; arm64 X[0]@0x8, Sp@0x100, Pc@0x108) — no fabricated-CONTEXT unit test yet, as the author noted.
Extended reasoning...
Overview
This PR extends the crash handler to capture the full GP register file from ucontext_t (POSIX) / CONTEXT (Windows) at fault time and encode it into the trace string, bumping the wire format from v1/v2 to v3/v4. Five files touched: src/crash_handler/lib.rs (~250 lines net), a new segfaultWithRegisters internal-for-testing hook in crash_handler_jsc.rs + internal-for-testing.ts, and two new tests in run-crash-handler.test.ts.
Security risks
The register file can contain live pointers (ASLR-sensitive) and, in principle, secret material resident in registers at the fault. The author's position is that v1 already encodes ASLR-adjusted stack addresses and the raw si_addr, and upload is already gated by is_reporting_enabled() (DO_NOT_TRACK, BUN_ENABLE_CRASH_REPORTING). That's reasonable, but the author explicitly said "not changing this without maintainer sign-off" and offered an env-var opt-out — that's a policy call for a maintainer, not a bot.
Level of scrutiny
High. This is signal-handler code (async-signal-safe constraints), it changes a wire format consumed by an external service (bun.report must ship a matching decoder or v3/v4 traces are opaque), and it spans six cfg-gated arch/OS arms including hand-coded winnt.h byte offsets that the compiler cannot check. The Windows path in particular has no unit test (author deferred that as a follow-up). The 1024→1536 trace buffer bump looks sufficient for aarch64's worst case (33 regs × ~14 VLQ bytes ≈ 462 bytes on top of the existing payload), and the encoder writes through a bounded writer that fails gracefully on overflow rather than corrupting.
Other factors
All prior review threads (mine and coderabbit's) are resolved. The encoding path is exercised deterministically via the new segfaultWithRegisters hook so it works under ASAN. cargo check on all 10 targets and clippy are reported clean. But the combination of external-service coordination, an open maintainer question, and untested hand-coded Windows offsets puts this well outside "simple/mechanical" — a human should sign off on the format bump and the privacy default.
There was a problem hiding this comment.
The three nits from my earlier pass are all addressed in 263cd47. I didn't find further issues, but this needs a maintainer's eyes: it bumps the trace-string wire format (bun.report decoder coordination), hand-codes winnt.h CONTEXT byte offsets without a unit test, and the author explicitly deferred the register-privacy opt-in question to maintainer sign-off.
What was reviewed:
- Wire-format compatibility — register block now gated on
is_fault()so'0'/'1'/'8'/'9'payloads are byte-identical to v1; verified againstencode_trace_string. - Signal-handler safety —
TraceSeed::Faultborrows a stack-residentFaultRegisters, no allocation in the handler; buffer bumped 1024→1536 to fit 33×2-VLQ regs. NAMES/COUNT/MAXare now tied by a module-scopeconst _assert; per-arch readers assignr.count = COUNT.- Test's JS
vlq/u64AsTwoVlqsmatcheswrite_u64_as_two_vlqs(hi-then-lo, u32-as-i32 bit reinterpretation).
Extended reasoning...
Overview
Adds a FaultRegisters struct capturing pc, fp, and the full GP register file from ucontext_t (Linux/Android/macOS × x64/aarch64) and the Windows CONTEXT record, threads it through TraceSeed::Fault into encode_trace_string, and appends it as a new v3/v4 trace-string block after the reason payload for hardware-fault reasons '2'..'7'. Bumps VERSION_CHAR from '1'/'2' to '3'/'4'. Adds a segfaultWithRegisters internal-for-testing hook and two tests in run-crash-handler.test.ts. ~350 net-new lines across 5 files.
Security risks
The register file can contain live pointers (ASLR-revealing) and occasionally secret material that happened to be in a register at fault time. The author's position is that the existing v1 trace already carries ASLR-adjusted stack addresses and the raw si_addr, and upload is gated by the same is_reporting_enabled() opt-in — but they explicitly said they won't add a separate register opt-in without maintainer sign-off. That is an open design decision a human needs to make.
Level of scrutiny
High. This is signal-handler code where a secondary panic is catastrophic, it changes a wire format consumed by an external service (bun.report), and it adds six cfg-gated per-platform register readers — the Windows arms read raw byte offsets into an opaque CONTEXT blob with no compile-time check and no unit test (author noted this as a reasonable follow-up). None of that is auto-approvable.
Other factors
- All three of my prior 🟡 findings (inert associated-const assert, 5-line doc comment, vacuous
endsWith("_A")assertion) are fixed in 263cd47; the current diff reflects those fixes. - The earlier 🔴 wire-format ambiguity for reasons
'0'/'8'was fixed in f354cdb by gating the block onis_fault(); I re-verified the encoder branch and theoutOfMemorytest asserts the body ends exactly at'9'. cargo checkreportedly passes on all 10 targets and CI build #76268 is running; the PR body notes local test proof was deferred to CI as platform-specific.- The
trace_str_bufbump to 1536 bytes covers the worst case (aarch64: 33 regs × ~14 VLQ chars ≈ 462 bytes plus pc StackLine).
Manual verification on all targetsTriggered a real segfault (
On every target the decoder consumed the entire payload ( Linux x64 decoder outputmacOS arm64 decoder outputWindows arm64 decoder output |
robobun
changed the title
|
CI at 6e6e3a9: the only failure is Decoder PR: oven-sh/bun.report#28. |
There was a problem hiding this comment.
No new issues found, but this should get a human look: it bumps the trace-string wire format ('1'/'2' → '3' + build-flags VLQ + register block) which needs bun.report decoder coordination, and the author explicitly deferred the register-privacy opt-in question to a maintainer. The Windows path also hand-codes winnt.h CONTEXT byte offsets in unsafe code.
What was reviewed:
- Wire-format ambiguity for reasons '0'/'8' — fixed; register block is now gated on
is_fault()and non-fault encodings end at the reason byte. - Signal-handler safety —
TraceSeed::Faultborrows the stack-localFaultRegisters(no allocation); the local outlives the borrow on both POSIX and Windows call sites. NAMES.len() <= MAXassert hoisted to module scope so it actually evaluates;COUNT = NAMES.len()ties every reader's slot count to the table.- Note: the manual all-targets verification table was captured before 6e6e3a9 (shows
version_char: 4), so the collapsed '3' + build-flags layout hasn't been end-to-end verified on real faults yet — the automated test does cover it.
Extended reasoning...
Overview
This PR extends the crash-handler trace string to carry the faulting pc and general-purpose register file for hardware faults. It touches src/crash_handler/lib.rs (~350 lines: new FaultRegisters struct, per-target ucontext_t/CONTEXT readers for linux/android/macos/windows × x64/aarch64, encoder changes, version bump to '3' with a new build-flags VLQ), a new segfaultWithRegisters internal-for-testing hook in crash_handler_jsc.rs, and two new tests in run-crash-handler.test.ts.
Security risks
Register values at a fault can contain heap/stack pointers and occasionally data-derived values. CodeRabbit flagged this and the author declined to gate it behind a separate opt-in without maintainer sign-off, arguing the existing v1 format already carries ASLR-sensitive stack addresses and the raw si_addr, and upload is already gated by is_reporting_enabled(). That reasoning is sound but the decision was explicitly punted to a maintainer, so it should not be auto-approved.
Level of scrutiny
High. This is (a) a wire-format change to a string consumed by an external service (bun.report) — the decoder side is not in this repo and must be updated in lockstep; (b) unsafe code inside a signal handler across six platform/arch combos, with hand-coded winnt.h byte offsets on Windows (the author acknowledged a fabricated-CONTEXT unit test would be a reasonable follow-up); and (c) the most recent commit (6e6e3a9) is a design change — collapsing the '1'/'2' canary split into a build-flags VLQ — that adds a byte to every trace string, not just fault ones, and postdates the author's manual all-targets verification table.
Other factors
All prior inline findings from earlier runs (dead NAMES, inert associated-const assert, vacuous test assertion, 5-line doc comment, wire-format ambiguity for '0'/'8') have been addressed and the threads are resolved. The bug-hunting pass on the current head found nothing new. Tests look solid: the encoder is exercised deterministically via for_testing, pipes are drained concurrently, and the non-fault case asserts no trailing block. But given the external-decoder coordination requirement and the explicitly-deferred privacy question, a maintainer should be the one to approve.
|
Build #76508 update: the The two Re-roll already spent; needs a maintainer to re-run the darwin-14-x64 job or merge past it. |
There was a problem hiding this comment.
No bugs found this pass, but this defines a new crash-report wire format (v3) coordinated with bun.report#28 and reads raw CONTEXT/ucontext register state inside the signal handler across six OS/arch arms — worth a maintainer's eyes on the format bump and the register-privacy question the author flagged for sign-off.
What was reviewed:
- Encoder: register block only appended for
is_fault()reasons '2'..'7'; '0'/'1'/'8'/'9' unchanged, so v1 payloads for Panic/ZigError/OOM stay end-of-string terminated. - Signal-handler safety:
FaultRegistersis stack-local on the altstack and borrowed viaTraceSeed::Fault(®s), no allocation;trace_str_bufbumped 1024→1536 covers the worst-case aarch64 33×2-VLQ tail. - Per-arch readers: linux/macOS ucontext field access matches libc struct layouts; Windows x64/arm64 byte offsets match winnt.h
_CONTEXT/_ARM64_NT_CONTEXT; all readers setr.count = COUNTand the module-levelNAMES.len() <= MAXassert is live. - Prior 🟡 findings (inert
_CHECKconst, vacuousendsWith("_A"), NAMES doc overclaim, 0x80000000 VLQ collapse) all addressed in 263cd47/694156b3 or resolved decoder-side.
Extended reasoning...
Overview
Adds GP register capture to Bun's crash-report trace string. src/crash_handler/lib.rs gains a FaultRegisters struct plus per-platform readers (fault_context_from_ucontext on linux/android/macOS × x64/aarch64, new fault_context_from_windows_context on Windows x64/arm64) that lift the register file from the kernel-supplied context, and encode_trace_string appends a StackLine-encoded pc + VLQ count + count×2-VLQ register block after fault-reason payloads. VERSION_CHAR moves from '1'/'2' to a single '3' with a new one-byte build-flags VLQ (bit0 = canary) after the sha. A segfaultWithRegisters internal-for-testing hook and two new tests exercise the encoder deterministically. ~300 net lines, mostly cfg-gated unsafe reads.
Security risks
Register values can carry pointers, secrets held in GPRs, and ASLR-revealing addresses. The author's position (timeline, 12:18 UTC) is that upload is already gated by is_reporting_enabled() and that v1 already carries ASLR-adjusted stack addresses plus the raw si_addr, so this is not a new class of exposure — but they explicitly deferred the "should register capture be a separate opt-in" question to a maintainer. That's a design call, not a bug. No injection/auth surface; the data flows outward only.
Level of scrutiny
High. This code runs inside a fatal-signal handler where a secondary fault loses the original crash entirely; it does raw pointer reads at hardcoded byte offsets on Windows; and it's a versioned wire format shared with an external decoder repo, so mistakes are sticky. It also changes what every existing crash report looks like (version char '3', extra build-flags byte), which the pre-existing "automatic crash reporter" tests would catch if broken — they still pass per the CI notes.
Other factors
The PR has been through several bot review rounds; every prior inline finding is resolved. Manual verification tables in the thread show real-fault decodes on all five shipped targets with sane-looking register state (JSC NaN-box tags in the expected slots, sentinel in the right scratch reg). CI is green except for known-flaky infra (darwin-14-x64 localhost connect timeout, win-arm64 agent provisioning). The paired decoder is in a separate PR that a maintainer will need to land in lockstep. Given the format bump, cross-repo coupling, and the open privacy opt-in question, this should be human-reviewed rather than bot-approved.
|
Build #76686 (694156b): |
|
Adversarial review findings addressed in f005bdc:
One finding left as-is: the pc |
There was a problem hiding this comment.
No new findings — the two nits from the last pass (stale v1/v2 in the GIT_SHA comment, and the getFeatureData type gap) are both addressed in e33ded3. Deferring to a maintainer for final sign-off: this is a wire-format bump (VERSION_CHAR '1'/'2' → '3' + build-flags VLQ) that must land in lockstep with oven-sh/bun.report#28, adds ~200 lines of unsafe per-arch ucontext_t/CONTEXT register reads inside the signal handler across six cfg-gated targets, and pulls ~60 new transitive dev-deps into bun.lock via the bun-tracestrings pin bump.
What was reviewed:
- v3 encoding: register block gated on
is_fault()only ('2'..'7'), so '0'/'8' end-of-string payloads stay unambiguous;debug_assert!(regs.is_none())on non-fault paths;trace_str_bufgrown 1024→1536 to fit 33 aarch64 regs. FaultRegisterslifetime: borrowed on the handler's stack (no allocation in signal context);TraceSeed::Fault(®s)outlives thecrash_handlercall at every call site.- Windows
CONTEXTbyte-offset reads useread_unaligned; offsets match the winnt.h layout the comment cites and were verified end-to-end on both win-x64 and win-arm64 per the manual-verification table. i32::MINVLQ collapse for register halves confirmed lossless on the bun.report decoder side (round-trip test added there).
Extended reasoning...
Overview
The PR extends Bun's crash-report trace string to v3: a single '3' version char replaces the '1'/'2' release/canary pair (canary moves into a new one-byte BUILD_FLAGS VLQ after the sha), and hardware-fault reasons ('2'..'7') gain a trailing register block — one StackLine-encoded fault pc, a VLQ count, then count × two-VLQ GP register values. FaultRegisters is populated from ucontext_t (linux/android/macOS × x64/aarch64) and Windows CONTEXT (x64/arm64, via documented winnt.h byte offsets). TraceSeed::Fault now borrows &FaultRegisters so nothing allocates inside the signal handler. A new segfaultWithRegisters internal-for-testing hook, three new tests in run-crash-handler.test.ts, and a bun-tracestrings pin bump (to the commit carrying the v3 decoder) round it out. Seven files, ~+560/-30 lines plus lockfile churn.
Security risks
Register values are process-local CPU state and are appended only for hardware faults; upload remains gated by the existing is_reporting_enabled() telemetry opt-in (DO_NOT_TRACK, BUN_ENABLE_CRASH_REPORTING, BUN_CRASH_REPORT_URL) — the same gate that already covers stack addresses and the fault si_addr. No new external inputs are parsed. The unsafe blocks read kernel-provided ucontext_t/CONTEXT at documented offsets and are guarded by null checks where the pointer can be null (macOS uc_mcontext, Windows ContextRecord). No injection, auth, or data-exposure surface beyond what v1 already carried.
Level of scrutiny
High. The crash handler runs in async-signal context on the faulting thread's altstack; a mistake here means either a secondary crash that loses the report entirely or a corrupted trace string. The change also bumps a wire format that an external service (bun.report) decodes, so merge ordering with oven-sh/bun.report#28 matters — the author explicitly noted "ready to merge after oven-sh/bun.report#28 lands" and the CI ci-remap-server depends on the pinned decoder. Six cfg-gated platform bodies mean five of them are invisible to any single-target cargo check; the author ran cargo check on all ten targets and manually verified real segfaults on five, but that's exactly the kind of cross-platform unsafe surface a maintainer should eyeball.
Other factors
This PR has been through five rounds of review (coderabbit + three of mine), all resolved: the inert _CHECK associated const → module-level const _, vacuous endsWith("_A") assertion dropped, 5-line struct doc trimmed and de-duplicated against NAMES, the overstated "fails at compile time" claim on NAMES softened, the i32::MIN VLQ collapse confirmed recoverable on the decoder side with a round-trip test in bun.report#28, the stale v1/v2 GIT_SHA comment updated, and getFeatureData/getFeaturesAsVLQ typed in internal-for-testing.ts. Test coverage is solid — a deterministic sentinel-register test that runs everywhere including ASAN, a real-fault reader test skipped under ASAN, and a negative test that non-fault reasons carry no register block. CI was green on run-crash-handler.test.ts across all lanes on the last full run. The bun.lock churn (~60 new transitive packages from archiver/tar in the bumped bun-tracestrings) is dev-only but worth a maintainer glance. Given the wire-format design decision, the coordinated external-repo dependency, and the volume of platform-gated unsafe, this should get a human sign-off rather than an auto-approve.
… trace string The signal/exception handler already reads pc/fp from ucontext_t (POSIX) and ExceptionAddress (Windows) to seed the frame-pointer walk, but none of that reached the trace string. For hardware faults the register state is often the only way to recover 'this'/arguments when the stack walk is short or corrupt. - FaultRegisters holds pc, fp, and the full GP register file in a fixed per-arch slot order (x86_64: rax..r15,rip; aarch64: x0..x28,fp,lr,sp,pc). fault_context_from_ucontext now fills it for linux/android/macos on both arches; fault_context_from_windows_context reads CONTEXT by the documented winnt.h offsets on x64/arm64. - encode_trace_string appends a register block after the existing reason payload: one StackLine-encoded fault pc (ASLR removed), a VLQ count, then count * two-VLQ register values. When there is no fault context the block is '_A' (no pc, zero registers). - VERSION_CHAR bumped to '3' (release) / '4' (canary). '3'/'4' is a strict superset of '1'/'2' with the register block appended, so a decoder that only knows '1'/'2' can parse the prefix and ignore the tail; a decoder that knows '3'/'4' reads both old and new strings. - internal-for-testing gains segfaultWithRegisters so the encoding path can be exercised deterministically under ASAN (where our SIGSEGV handler is not installed).
Addresses clippy::large_enum_variant / clippy::large_types_passed_by_value. The register file lives on the signal handler's (alt)stack; boxing it would call malloc from inside a signal handler, so borrow instead.
The register block was appended for every reason, but Panic ('0') and
ZigError ('8') have end-of-string-terminated payloads (base64 / raw
identifier) so a trailing '_A' made them ambiguous: '_' and 'A' are
valid identifier characters, and '_' shifts the base64 length.
Only emit the block for reasons '2'..'7' (segfault/ill/bus/fpe/
misalign/stackoverflow), the only variants that ever arrive via a
signal/exception handler and whose payloads are self-delimiting.
Reasons '0'/'1'/'8'/'9' are now byte-identical to v1/v2.
Also: make FaultRegisters::NAMES the source of truth for the per-arch
register count (COUNT = NAMES.len()) so the ucontext/CONTEXT readers
and the name table cannot drift.
Test now computes the exact VLQ encoding of the synthetic register values and asserts the payload ends with it, and locates the segfault reason by its exact two-VLQ encoding rather than a substring scan. Also condenses four new comments to three lines per the coding guideline.
…t doc/assert
- const _: () = assert!(..) at module scope is always evaluated; the
associated-const version was lazy and never fired.
- FaultRegisters doc points to NAMES for slot order instead of
duplicating it inline.
- drop vacuous endsWith("_A") assertion already implied by
endsWith("A9").
…lags VLQ
Drop the '3'/'4' split (which mirrored '1'/'2') and emit a single
version char '3'. The canary bit moves into a one-byte build-flags VLQ
written right after the 7-char sha and before the packed-features VLQs
(bit 0 = canary). Future build-variant flags can pack into the same VLQ
without burning another version char.
Format (v3): <ver>/<platform><cmd>3<sha7><build_flags vlq>
<features 2-vlq><frames><A><reason>[<reg block>]
Register block emission and slot order are unchanged.
r.count = COUNT ties the encoded count to NAMES.len(), but the per-reader slot-write bodies are not length-checked against NAMES; only the module-scope NAMES.len() <= MAX is a compile-time guard.
- Bump bun-tracestrings to bun.report#1a48fb0 (includes the v3 decoder from oven-sh/bun.report#28) so scripts/runner.node.mjs's ci-remap-server can parse v3 trace strings. - Add a .skipIf(isASAN) test that triggers a real *0xDEADBEEF write and decodes the register block from the actual ucontext/CONTEXT reader: asserts count matches the host arch's FaultRegisters::COUNT, 0xDEADBEEF appears in at least one GP register, rip/pc is non-zero, and no bytes trail the block. - Tighten the build_flags byte assertion to the exact value for the running build (via crash_handler.getFeatureData().is_canary) instead of accepting either 'A' or 'C'.
…e v1/v2 in GIT_SHA comment
54b118f to
6ab1d4a
Compare
|
Build #78089 (6ab1d4a, rebased onto main): |
2 participants
What
Capture the faulting
pcand general-purpose register file fromucontext_t(POSIX) /CONTEXT(Windows) in the fault signal handler and encode them into the crash-report trace string.Decoder: oven-sh/bun.report#28.
Why
For hardware faults (SIGSEGV/SIGILL/SIGBUS/SIGFPE, access violation) the register state at the fault is often the only way to reconstruct
this, arguments, or the dereferenced pointer when the frame-pointer walk is short or corrupt. The handler was already readingpc/fpfromucontext_tto seed the walk, but none of it reached the trace string.How
FaultRegistersholdspc,fp, and the GP register file in a fixed per-arch slot order (FaultRegisters::NAMESis the source of truth):rax rbx rcx rdx rdi rsi rbp rsp r8..r15 rip(17)x0..x28 fp lr sp pc(33)fault_context_from_ucontextfills it on linux/android (glibc + musl) and macOS, both arches.fault_context_from_windows_contextreadsCONTEXTby the documentedwinnt.hbyte offsets on x64 and arm64.TraceSeed::Faultborrows&FaultRegisters(lives on the signal handler's altstack; boxing would allocate inside a signal handler).encode_trace_stringappends a register block after the reason payload for hardware-fault reasons'2'..'7'only: oneStackLine-encoded faultpc(ASLR removed, remappable), a VLQ register count, then count × two-VLQ register values. When the fault path has no saved context (unsupported arch) the block is_A(no pc, zero registers).Trace-string format (v3)
VERSION_CHARis now a single'3'. The even/odd split ('1'/'2'= release/canary) is replaced by a one-bytebuild_flagsVLQ right after the 7-char sha; bit 0 = canary. Future build-variant flags pack into the same VLQ without a format bump.'0'/'1'/'8'/'9'have no register block.'2'..'7'are followed by the register block.'1'/'2'strings (older builds) remain decodable.Testing
test/cli/run/run-crash-handler.test.tsgains atrace string v3describe block:segfaultWithRegisters(new internal-for-testing hook) invokes the handler with a syntheticFaultRegistersso the encoding path is exercised deterministically, including under ASAN where our SIGSEGV handler is not installed. Asserts version char is'3', the build-flags byte follows the sha, and the payload ends with the exact VLQ encoding of the sentinel register values.outOfMemory(non-fault reason'9') asserts no register block is appended.cargo check -p bun_crash_handlerpasses on all 10 supported targets (linux gnu/musl, macos, windows, android, freebsd × x64/aarch64) andcargo clippyis clean.Verified end-to-end with real segfaults on linux-x64, macos-x64, macos-arm64, windows-x64, windows-arm64 (see comments below for decoded output).
no test proof · iteration 7 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/run/run-crash-handler.test.ts