Bun__inspect: don't panic when user JS throws during error-message formatting - #30980
Bun__inspect: don't panic when user JS throws during error-message formatting#30980robobun wants to merge 1 commit into
Conversation
|
Updated 5:29 AM PT - Jul 2nd, 2026
❌ @robobun, your commit e42fedb has some failures in 🧪 To try this PR locally: bunx bun-pr 30980That installs a local version of the PR into your bun-30980 --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:
WalkthroughThe PR updates ChangesObject inspection error handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted fix swapping the Vec<u8> sink to core::fmt::Write via the existing VecWriter helper so a throwing Display impl surfaces as Err instead of panicking; regression test included.
Extended reasoning...
Overview
This PR touches three files: a one-line logic change in src/runtime/api/BunObject.rs (bun_inspect), a trivial assert → ASSERT macro swap in src/jsc/bindings/wtf-bindings.cpp, and a new regression test in test/js/bun/util/inspect-error.test.js. The core change replaces write!(&mut Vec<u8>, ...) (which resolves to std::io::Write::write_fmt and panics when a Display impl returns fmt::Error without an underlying I/O error) with write!(bun_core::fmt::VecWriter(&mut array), ...), which goes through core::fmt::Write and returns the error so the existing .is_err() branch actually fires.
Security risks
None. This is a panic-to-graceful-error conversion in a value-formatting helper. No auth, crypto, permissions, parsing of untrusted input beyond what was already happening, or new attack surface. If anything, it removes a user-triggerable process crash.
Level of scrutiny
Low. The fix is mechanical and follows an established pattern — bun_core::fmt::VecWriter already exists precisely for this purpose (per its doc comment at src/bun_core/fmt.rs:800-820) and is used identically in FrameworkRouter.rs, JSGlobalObject.rs, clap/lib.rs, and shell_parser/braces.rs. The local use core::fmt::Write; doesn't conflict with the module-level use std::io::Write as _; (anonymous import), and VecWriter only implements core::fmt::Write, so method resolution is unambiguous. The ASSERT swap in wtf-bindings.cpp is a debug-build hygiene fix matching surrounding WTF conventions.
Other factors
A regression test is included that crashes on the baked Bun and passes with the fix. The inline note about the same pattern in jest.rs::format_label is explicitly pre-existing and non-blocking — worth a follow-up but out of scope here. No CODEOWNERS cover these paths and there are no outstanding reviewer comments.
There was a problem hiding this comment.
LGTM — the inline notes are pre-existing sibling sites, fine for a follow-up.
Extended reasoning...
Overview
Three small changes: (1) bun_inspect in src/runtime/api/BunObject.rs now writes through core::fmt::Write via the existing bun_core::fmt::VecWriter adapter instead of std::io::Write on Vec<u8>, so a fmt::Error from ZigFormatter's Display impl (when user JS throws inside inspect.custom) surfaces as a Result and hits the existing .is_err() early-return instead of panicking inside std's default_write_fmt; (2) a regression test in cookie-expires-validation.test.ts exercising the JSValueToStringSafe → Bun__inspect path; (3) assert → ASSERT in wtf-bindings.cpp to fix a debug-build break from a missing transitive <cassert>.
Security risks
None. This narrows a user-triggerable native panic into a normal JS throw; no new data exposure, parsing, or privilege surface. The ASSERT swap is debug-only and semantically equivalent.
Level of scrutiny
Low–medium. The Rust change is a one-expression trait-resolution swap onto an established in-tree helper (VecWriter is already used in JSGlobalObject.rs, FrameworkRouter.rs, braces.rs, etc.) whose write_str is infallible, so .is_err() can only be true when the Display impl itself errored — exactly the intended behavior. The test and the ASSERT change are trivial.
Other factors
The two inline notes (jest.rs format_label and expect.rs print_value) are explicitly pre-existing instances of the same pattern in code this PR doesn't touch — non-blocking and reasonable to defer. Since my prior review the only delta is relocating the regression test into cookie-expires-validation.test.ts. The fix is minimal, targeted, has a regression test, and strictly reduces crash surface.
| if write!( | ||
| bun_core::fmt::VecWriter(&mut array), | ||
| "{}", | ||
| value.to_fmt(&mut formatter) | ||
| ) | ||
| .is_err() | ||
| { |
There was a problem hiding this comment.
🟣 🟣 Heads up (pre-existing): there is one more instance of this same std::io::Write::write_fmt panic in src/runtime/test_runner/expect.rs ExpectMatcherUtils::print_value (expect.rs:2866). writer is a &mut MutableString whose std::io::Write::write() is infallible (MutableString.rs:476-480) and only std::io::Write is in scope (expect.rs:2850), so let _ = write!(writer, "{}", value.to_fmt(&mut formatter)); panics inside write_fmt when ZigFormatter's Display returns fmt::Error — reachable from user JS via this.utils.stringify/printExpected/printReceived in expect.extend matchers. Same one-line core::fmt::Write / VecWriter fix; might be worth folding in alongside the jest.rs site so the Fuzzilli fingerprint is fully eliminated.
Extended reasoning...
Summary
This is a third site (in addition to bun_inspect fixed by this PR and jest.rs format_label already flagged in the earlier comment) with the identical std::io::Write::write_fmt-on-infallible-sink panic. It lives in src/runtime/test_runner/expect.rs inside ExpectMatcherUtils::print_value, the helper that backs this.utils.stringify, this.utils.printExpected, and this.utils.printReceived for custom Jest matchers.
This is pre-existing: the PR does not touch expect.rs, add callers, or change its behavior. Flagging it non-blocking because it is the same root cause / Fuzzilli fingerprint the PR is closing out, and the previous comment only listed the jest.rs sites.
Code path
expect.rs:2850—use std::io::Write as _;(nocore::fmt::Writeanywhere in the file).expect.rs:2856—let writer = mutable_string.writer();whereMutableString::writerreturns&mut Self(MutableString.rs:58-60), i.e.&mut MutableString.expect.rs:2866—let _ = write!(writer, "{}", value.to_fmt(&mut formatter));with aConsoleObject::Formatter.MutableStringimplementsstd::io::Write(MutableString.rs:476-480) with an infalliblewrite()that justextend_from_slices into aVec<u8>and returnsOk(buf.len()). There is nocore::fmt::Writeimpl forMutableStringin the repo.value.to_fmt(&mut formatter)returns aZigFormatter, whoseDisplay::fmt(ConsoleObject.rs:1922-1953) maps bothTag::getfailures andformatter.formatfailures toErr(core::fmt::Error)via.map_err(|_| core::fmt::Error)?— i.e. it returnsfmt::Errorwhen user JS throws during inspection.
Because only std::io::Write is in scope and MutableString only implements std::io::Write, write!(writer, …) resolves to <MutableString as std::io::Write>::write_fmt. Exactly as described in this PR for bun_inspect, std's default_write_fmt panics with "a formatting trait implementation returned an error when the underlying stream did not" when the Display impl errors but the underlying MutableString sink never set an I/O error.
Why nothing prevents it
The let _ = discards a Result it never receives — the panic happens inside write_fmt before it returns. The wrapping print_value_catched (expect.rs:2880-2887) only handles JsResult errors via unwrap_or_else; it cannot catch a Rust panic. The asymmetric-matcher self-print path (expect.rs:2784) hits the same helper.
Step-by-step proof
import { expect, test } from "bun:test";
expect.extend({
myMatcher(received) {
return { pass: false, message: () => this.utils.printReceived(received) };
},
});
const bad = { [Symbol.for("nodejs.util.inspect.custom")]() { throw new Error("boom"); } };
test("x", () => expect(bad).myMatcher());- The matcher fails and Jest evaluates
message(), which callsthis.utils.printReceived(bad)→Expect_print_received(expect.rs:2905-2911) →print_value_catched→print_value. - At expect.rs:2866,
bad.to_fmt(&mut formatter)constructs aZigFormatter. write!(writer, "{}", <ZigFormatter>)resolves to<MutableString as std::io::Write>::write_fmt(onlystd::io::Writeis in scope;MutableStringhas nocore::fmt::Writeimpl).- Inside
write_fmt,<ZigFormatter as Display>::fmtruns the console formatter, which invokesbad'sinspect.custom. That throws; the formatter maps the JS exception tocore::fmt::ErrorandDisplay::fmtreturnsErr(fmt::Error). std::io::default_write_fmtsees a formatting error with no underlying I/O error (theMutableStringsink'swrite()never fails) and panics with "a formatting trait implementation returned an error when the underlying stream did not".- The
let _ =is never reached;print_value_catched'sunwrap_or_elsenever runs; thebun testprocess crashes.
Impact
A user-supplied object with a throwing custom inspect, passed to a custom matcher that uses this.utils.stringify / printExpected / printReceived (the standard way to build matcher messages), crashes the entire bun test process instead of failing the test. Same crash class and Fuzzilli fingerprint (panic:a formatting trait implementation returned an error when the) that this PR is closing out.
Suggested fix
Same as bun_inspect: bring core::fmt::Write into scope and write through a core::fmt::Write sink (e.g. bun_core::fmt::VecWriter(&mut mutable_string.list), or add a core::fmt::Write impl for MutableString) so the error surfaces as a Result. Then the existing let _ = / print_value_catched fallback degrades gracefully (or you can render a placeholder) instead of panicking.
There was a problem hiding this comment.
LGTM — targeted fix with a regression test; my earlier notes about jest.rs/expect.rs are pre-existing and non-blocking.
Extended reasoning...
Overview
This PR fixes a process-crash panic in bun_inspect (src/runtime/api/BunObject.rs:773-793), the native helper behind JSValueToStringSafe used to render received values into error messages like ERR_INVALID_ARG_VALUE. When the value being inspected has a throwing [Symbol.for("nodejs.util.inspect.custom")], ZigFormatter's Display impl returns fmt::Error. Previously the sink was Vec<u8> via std::io::Write, whose write_fmt panics (rather than returning Err) when a Display impl errors but the underlying stream did not — so the existing .is_err() guard never ran. The fix routes through core::fmt::Write via bun_core::fmt::VecWriter (verified at src/bun_core/fmt.rs:812-826: public tuple struct implementing only core::fmt::Write), making the error observable so the function returns BunString::empty() and the user's thrown exception propagates as a JS error. A regression test is added to cookie-expires-validation.test.ts. The PR also swaps assert → ASSERT in wtf-bindings.cpp, matching the WTF convention already used elsewhere in that file.
Security risks
None. This is a crash-hardening change in error-message formatting; no auth, crypto, permissions, parsing of untrusted input beyond what already existed, or new attack surface. The behavior change is strictly panic → clean JS throw.
Level of scrutiny
Low-to-moderate. The diff is ~15 lines across three files, the mechanism is well-understood (trait resolution: VecWriter only implements core::fmt::Write, so write! resolves to core::fmt::Write::write_fmt which returns Result instead of panicking), and the PR includes a regression test that exercises the exact crash path. The local use core::fmt::Write; does not conflict with the file-level use std::io::Write as _; because VecWriter does not implement std::io::Write. The assert → ASSERT change is a trivial convention/build fix.
Other factors
I previously left two non-blocking inline comments noting that the identical std::io::Write::write_fmt-on-infallible-sink panic pattern remains in jest.rs format_label and expect.rs print_value. Those are pre-existing, not introduced here, and explicitly flagged as optional follow-ups — they do not block this fix. The bug-hunting system found no issues in the current revision, and the only commit since my last comment is a CI retrigger.
5182c5b to
9bb53a7
Compare
There was a problem hiding this comment.
LGTM — targeted crash fix with a regression test; my earlier notes about jest.rs/expect.rs are pre-existing and non-blocking.
Extended reasoning...
Overview
This PR fixes a Rust panic in bun_inspect (src/runtime/api/BunObject.rs) that fires when user JS throws inside [Symbol.for("nodejs.util.inspect.custom")] while Bun is rendering a value into an error message (e.g. ERR_INVALID_ARG_VALUE for Bun.Cookie's expires option). The fix swaps the sink from std::io::Write on Vec<u8> — whose write_fmt panics when a Display impl returns fmt::Error without an underlying I/O error — to core::fmt::Write via the existing bun_core::fmt::VecWriter, so the error becomes an observable Result and the function returns an empty string as originally intended. A regression test is added to test/js/bun/cookie/cookie-expires-validation.test.ts.
I verified VecWriter at src/bun_core/fmt.rs:812-826: it's a public tuple struct over &mut Vec<u8> with an infallible core::fmt::Write::write_str. With the local use core::fmt::Write; and a VecWriter receiver (which does not implement std::io::Write), write! unambiguously resolves to core::fmt::Write::write_fmt, so .is_err() now actually catches the formatter error. The pattern is already used elsewhere (src/clap/lib.rs:1043, src/shell_parser/braces.rs:467).
Security risks
None. This converts an unconditional process abort into the originally-intended graceful fallback (empty string + the user's thrown JS exception propagates). No new inputs are accepted, no parsing/validation is relaxed, and the behavior on the success path is byte-identical.
Level of scrutiny
Low–medium. The diff is ~12 lines of Rust plus a 14-line test. The mechanism is well-understood (std's io::Write::write_fmt panic-on-fmt-error is documented behavior), the replacement helper is pre-existing and used elsewhere in the repo, and the local use is scoped to the function so it can't perturb trait resolution for other write! calls in the file.
Other factors
- The PR description mentions a
wtf-bindings.cppassert→ASSERTdrive-by, but the current diff only contains the two files above; that hunk appears to have been dropped, which is fine. - I previously left two inline comments noting the same panic pattern still exists in
jest.rs(format_label) andexpect.rs(ExpectMatcherUtils::print_value). Those are explicitly pre-existing, out of this PR's scope, and non-blocking — worth a follow-up but not a reason to hold this fix. - Bug-hunter found no issues; CodeRabbit had no actionable comments.
9bb53a7 to
2f3e1ba
Compare
There was a problem hiding this comment.
LGTM — correct, minimal fix with a regression test; my earlier notes on the jest.rs/expect.rs sites are pre-existing and non-blocking.
Extended reasoning...
Overview
This PR fixes a panic in bun_inspect (src/runtime/api/BunObject.rs) by switching the formatting sink from std::io::Write on Vec<u8> to core::fmt::Write via the existing bun_core::fmt::VecWriter wrapper. When ZigFormatter's Display impl returns fmt::Error (because user JS threw during inspection), core::fmt::Write::write_fmt propagates that as an Err instead of panicking inside std's default_write_fmt. The existing .is_err() branch then returns BunString::empty() and the pending JS exception surfaces normally. A regression test in cookie-expires-validation.test.ts exercises the exact crash path.
I verified VecWriter at src/bun_core/fmt.rs:812-826 — it's a thin core::fmt::Write impl over &mut Vec<u8> already used in src/clap/lib.rs and src/shell_parser/braces.rs, so this follows an established pattern with no new abstractions.
Security risks
None. This is error-path hardening in a value-rendering helper; no auth, crypto, parsing of untrusted input beyond what was already there, and no new data exposure. The change strictly reduces attack surface by converting a process abort into a catchable JS error.
Level of scrutiny
Low. ~10 lines of mechanical change (trait swap + comment) in a single function, plus a focused test. The logic is identical to the original intent — the .is_err() check was already there, it just couldn't fire because the panic preempted it. The fix makes the existing code work as written.
Other factors
- Bug hunting system found no issues.
- I previously left two inline comments flagging the same pre-existing pattern in
jest.rsformat_labelandexpect.rsprint_value; both are explicitly non-blocking and out of scope for this PR. - The local
use core::fmt::Write;is scoped to the function body, so it can't accidentally changewrite!resolution elsewhere in the file. - Regression test is well-placed and asserts
.toThrow()rather than a specific message, so it won't be brittle.
2f3e1ba to
72da2b6
Compare
There was a problem hiding this comment.
LGTM — the core::fmt::Write / VecWriter switch correctly surfaces the formatter error as a Result instead of panicking inside std::io::default_write_fmt. (My earlier notes on the jest.rs/expect.rs sibling sites are pre-existing and non-blocking; fine as a follow-up.)
Extended reasoning...
Overview
Two-file change: src/runtime/api/BunObject.rs swaps the write! sink in bun_inspect from Vec<u8> (via std::io::Write) to bun_core::fmt::VecWriter(&mut array) (via core::fmt::Write), and test/js/bun/cookie/cookie-expires-validation.test.ts adds a regression test that passes a throwing [Symbol.for("nodejs.util.inspect.custom")] object as expires to Bun.Cookie.
I verified VecWriter at src/bun_core/fmt.rs:829-843 — it's a thin core::fmt::Write adapter over &mut Vec<u8> whose write_str just extend_from_slices and returns Ok(()). With core::fmt::Write in scope, write!(VecWriter(...), ...) resolves to core::fmt::Write::write_fmt, which propagates fmt::Error from ZigFormatter's Display impl as a Result rather than triggering std's "formatting trait returned an error when the underlying stream did not" panic. The existing .is_err() branch then returns BunString::empty() as originally intended, and the pending JS exception propagates to the caller.
Security risks
None. This is purely a panic-to-graceful-error conversion in an inspection/formatting helper; no auth, crypto, parsing of untrusted input boundaries, or permission logic is touched.
Level of scrutiny
Low. The diff is ~12 lines of substantive change in one function plus a 14-line test. The pattern (VecWriter + core::fmt::Write) is already used elsewhere in the repo (e.g. FrameworkRouter.rs, braces.rs, JSGlobalObject.rs), so this follows an established idiom. No CODEOWNERS apply to the touched paths.
Other factors
I previously left two non-blocking heads-up comments noting the identical panic pattern still exists in jest.rs format_label and expect.rs ExpectMatcherUtils::print_value. Those are explicitly pre-existing, out of scope for this PR, and were framed as optional follow-ups — they do not block this fix, which is correct and self-contained for the Bun__inspect / JSValueToStringSafe path it targets. The added regression test directly exercises the crash-before/throw-after behavior.
72da2b6 to
2066eb7
Compare
There was a problem hiding this comment.
LGTM — targeted swap from std::io::Write to core::fmt::Write via the existing VecWriter so the formatter error surfaces as a Result instead of panicking. The test-structure nit and my earlier pre-existing-site notes are non-blocking.
Extended reasoning...
Overview
This PR changes ~12 lines in src/runtime/api/BunObject.rs (bun_inspect) to route the write! call through core::fmt::Write (via the existing bun_core::fmt::VecWriter helper) instead of std::io::Write on Vec<u8>. The latter panics inside default_write_fmt when a Display impl returns fmt::Error but the in-memory sink never errored — which is exactly what ZigFormatter does when user JS (e.g. a throwing inspect.custom) throws during formatting. With core::fmt::Write, the error surfaces as the Result the existing .is_err() branch was already written to handle, and bun_inspect returns an empty string so the user's JS exception propagates normally. A 14-line regression test is added to test/js/bun/cookie/cookie-expires-validation.test.ts.
I verified VecWriter at src/bun_core/fmt.rs:819-833: it is a public tuple struct over &mut Vec<u8> whose core::fmt::Write::write_str just extend_from_slices — semantically identical to writing into the Vec directly, minus the panic.
Security risks
None. This is a defensive change that converts a process-level Rust panic (DoS-class only, triggered by adversarial user JS objects passed into Bun APIs) into a clean catchable JS error. No new inputs, no parsing, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-medium. It is native Rust at the FFI boundary, but the change is mechanical: only the Write trait the macro resolves to changes; the bytes written are identical on the success path, and the error path now returns BunString::empty() as the original code clearly intended. VecWriter is an existing repo helper designed for exactly this purpose.
Other factors
- The one new inline finding is a nit about test structure (CLAUDE.md convention prefers spawned-subprocess fixtures for crash regressions). It is a fair test-quality note but does not affect the correctness of the fix; on regression the in-process test will still take the file down and CI will go red.
- My two earlier inline comments on this PR flagged pre-existing sibling instances of the same panic pattern in
jest.rsandexpect.rs. Those were explicitly non-blocking follow-up suggestions and remain valid for a separate PR; they do not gate this fix. - No CODEOWNERS entries match the touched paths.
- No human reviewer comments are outstanding.
2066eb7 to
ac2eb96
Compare
There was a problem hiding this comment.
LGTM — targeted fix routes the formatter through core::fmt::Write so the error is observable, and the regression test now runs in a subprocess per the repo convention.
Extended reasoning...
Overview
This PR changes one function, bun_inspect in src/runtime/api/BunObject.rs, to write the ZigFormatter output through core::fmt::Write (via the existing bun_core::fmt::VecWriter adapter at fmt.rs:819-833) instead of std::io::Write on a Vec<u8>. The previous code's .is_err() branch was unreachable because std::io::default_write_fmt panics when a Display impl returns fmt::Error but the underlying in-memory sink never errored; the new code makes that branch reachable and returns BunString::empty() as originally intended, letting the pending JS exception propagate. A regression test in test/js/bun/cookie/cookie-expires-validation.test.ts exercises the path via new Bun.Cookie(..., { expires: obj }) where obj has a throwing inspect.custom.
Security risks
None. This is a crash-hardening change in a value-rendering helper used to build error messages. No auth, crypto, permissions, parsing of untrusted formats, or new external surface. The behavior change replaces a process abort with a normal JS throw.
Level of scrutiny
Low. The Rust diff is ~10 lines and mechanical: it swaps the write! sink from &mut Vec<u8> (resolves to std::io::Write::write_fmt) to bun_core::fmt::VecWriter(&mut array) (resolves to core::fmt::Write::write_fmt), with a local use core::fmt::Write; to bring the trait into scope. VecWriter is an existing, already-used adapter whose write_str is infallible (extend_from_slice + Ok(())), so the only way write! returns Err is when the Display impl itself errors — exactly the case being handled. Control flow on the success path is unchanged (array is still the byte buffer passed to BunString::clone_utf8).
Other factors
- My earlier feedback about CLAUDE.md:199 (crash fixes need a spawned fixture) was addressed in the latest push: the test now spawns
bunExe() -ewithbunEnv, captures stdout/stderr/exitCode, and assertsexitCode: 0+stdout: "threw:boom from inspect.custom", so a regression surfaces as a clean per-test failure. - The two earlier "heads up" comments about identical pre-existing patterns in
jest.rs format_labelandexpect.rs print_valueare explicitly out-of-scope follow-ups (not introduced or touched by this PR) and were marked non-blocking when raised. - No new bugs were found by the bug-hunting system on this revision.
VecWriteralready has other in-tree callers (e.g.braces.rs:496,scanImportsAndExports.rs:560), so this follows an established pattern rather than introducing a new abstraction.
ac2eb96 to
ff3c9f8
Compare
There was a problem hiding this comment.
LGTM — targeted fix routes the write through core::fmt::Write so the formatter error is observable, and the subprocess regression test now covers it cleanly.
Extended reasoning...
Overview
This PR changes one function, bun_inspect in src/runtime/api/BunObject.rs, swapping write!(&mut Vec<u8>, ...) (which resolves to std::io::Write::write_fmt and panics when a Display impl returns fmt::Error on an infallible sink) for write!(bun_core::fmt::VecWriter(&mut array), ...) via core::fmt::Write, which surfaces the error as a Result so the existing .is_err() branch can return BunString::empty(). VecWriter is an existing in-tree helper (src/bun_core/fmt.rs:819) that implements exactly core::fmt::Write, so trait resolution is unambiguous. A subprocess-spawned regression test in test/js/bun/cookie/cookie-expires-validation.test.ts exercises the crashing input and asserts the specific thrown message plus exitCode: 0.
Security risks
None. The change turns a native panic (process abort) into the intended empty-string fallback + propagated JS exception. No new inputs are accepted, no parsing is added, and no trust boundary changes.
Level of scrutiny
Low. This is a ~11-line mechanical trait-resolution fix using an existing repo helper, with a well-formed subprocess regression test. The fix is idiomatic and matches the helper's documented purpose. My earlier feedback (that the crash test should be spawned rather than in-process toThrow()) was addressed in the 2026-06-19 revision and the thread is resolved.
Other factors
The only new finding in this run is a style nit (4-line comment vs the 3-line CLAUDE.md cap) — non-blocking and easy to fold in on merge or ignore. My two earlier heads-up comments about identical pre-existing panic sites in jest.rs and expect.rs were explicitly non-blocking (not introduced here) and remain valid follow-up work; they don't gate this fix. No CODEOWNERS on the touched paths, no outstanding human review comments.
ff3c9f8 to
63ff14a
Compare
There was a problem hiding this comment.
LGTM — the fix is correct and my prior feedback has been addressed; the remaining inline nit (4-line test comment) is non-blocking.
Extended reasoning...
Overview
This PR fixes a native Rust panic in bun_inspect (src/runtime/api/BunObject.rs), the helper backing JSValueToStringSafe in ErrorCode.cpp. When the console formatter's Display impl returns fmt::Error (because user JS threw in [Symbol.for("nodejs.util.inspect.custom")]), writing to a Vec<u8> via std::io::Write::write_fmt panics inside std before the existing .is_err() check can observe it. The fix routes the write through core::fmt::Write via bun_core::fmt::VecWriter, so the error surfaces as a Result and the function returns an empty string as originally intended, letting the pending JS exception propagate. A subprocess-spawned regression test in test/js/bun/cookie/cookie-expires-validation.test.ts covers the crash.
Security risks
None. This is a crash-hardening fix in a diagnostic/error-message rendering path. No auth, crypto, permissions, or untrusted-input parsing surface is touched; the change strictly narrows behavior from "panic" to "return empty + propagate the already-pending JS exception".
Level of scrutiny
Low-to-medium. The diff is ~10 native lines plus one test. The mechanism is well-understood (std::io::default_write_fmt panics on formatter error with an infallible sink; core::fmt::Write::write_fmt returns Err instead), VecWriter is an existing in-tree helper (src/bun_core/fmt.rs:819), and the original .is_err() guard is preserved so intent is unchanged. The regression test follows the repo's crash-fix convention (spawned subprocess asserting {stdout, exitCode}).
Other factors
All substantive feedback from my earlier reviews has been addressed: the test was rewritten to spawn a subprocess so a regression fails cleanly rather than aborting the file, and the BunObject.rs comment was condensed to 3 lines. My two earlier heads-up notes about identical pre-existing panics in jest.rs format_label and expect.rs print_value were explicitly non-blocking and out of this PR's scope; the author reasonably kept the change focused. The only new finding this run is a nit — the 4-line test comment exceeds the CLAUDE.md rule-13 3-line cap — which is cosmetic, has zero functional impact, and does not gate merge.
63ff14a to
9fcca5e
Compare
There was a problem hiding this comment.
LGTM — targeted panic fix with a proper subprocess regression test; all prior nits addressed. The pre-existing sibling sites in jest.rs/expect.rs remain noted for follow-up but don't block this.
Extended reasoning...
Overview
This PR fixes a native panic in bun_inspect (src/runtime/api/BunObject.rs), the FFI helper backing JSValueToStringSafe used to render values into error messages like ERR_INVALID_ARG_VALUE. The fix swaps the write! sink from std::io::Write on Vec<u8> (whose write_fmt panics when a Display impl returns fmt::Error without an underlying I/O error) to core::fmt::Write via the existing bun_core::fmt::VecWriter, so the error surfaces as a Result and the pre-existing .is_err() guard actually fires. A subprocess regression test is added to test/js/bun/cookie/cookie-expires-validation.test.ts. Net change is ~15 lines.
Security risks
None. This is error-message formatting only — no auth, crypto, permissions, or untrusted-input parsing changes. The behavioral change replaces a process-aborting panic with a catchable JS error, which is strictly a hardening improvement (a reachable panic on user input was itself a DoS vector).
Level of scrutiny
Low-to-moderate. The mechanism is well-understood and narrowly scoped: VecWriter is an existing in-tree helper (src/bun_core/fmt.rs:819) that implements core::fmt::Write by extend_from_slice, so the only behavioral difference is that fmt::Error from ZigFormatter now propagates as Err instead of panicking inside std. The success path is byte-identical. The 3-line comment at the call site explains why the indirection exists, which guards against a future "simplification" reintroducing the panic.
Other factors
All prior review feedback on this PR has been addressed: the regression test was rewritten to spawn a subprocess (so a future regression fails cleanly rather than aborting the test file), and both new comments were condensed to ≤3 lines per repo convention. The test follows the harness patterns (bunExe()/bunEnv, concurrent pipe drain, asserts {stdout, exitCode} before implicitly checking stderr via the object match). I previously flagged two pre-existing sibling sites with the identical panic pattern (jest.rs format_label and expect.rs print_value) — those were explicitly non-blocking heads-ups about code this PR does not touch, and leaving them for a follow-up is a reasonable scoping call that doesn't affect the correctness of this fix.
9fcca5e to
e42fedb
Compare
There was a problem hiding this comment.
LGTM — all prior feedback addressed; the pre-existing sibling sites in jest.rs/expect.rs are non-blocking and can go in a follow-up.
Extended reasoning...
Overview
This PR changes ~10 lines in bun_inspect (src/runtime/api/BunObject.rs) to route the write! call through core::fmt::Write via bun_core::fmt::VecWriter instead of std::io::Write on Vec<u8>. The old path panics inside std::io::default_write_fmt when ZigFormatter's Display impl returns fmt::Error (which it does when user JS throws during inspection), because the underlying Vec<u8> sink never sets an I/O error. The new path surfaces the error as a Result, and the pre-existing .is_err() branch returns BunString::empty() so the caller's JS exception propagates normally. A spawned-subprocess regression test is added to test/js/bun/cookie/cookie-expires-validation.test.ts.
I verified bun_core::fmt::VecWriter exists (fmt.rs:819) and implements core::fmt::Write, so the trait resolution is unambiguous with the local use core::fmt::Write;.
Security risks
None. This is a crash-hardening change on the error-message rendering path; no auth, crypto, permissions, or untrusted-input parsing is touched. If anything it removes a user-triggerable process abort (mild DoS surface).
Level of scrutiny
Low-to-medium. The mechanism is a well-understood Rust std footgun (std::io::Write::write_fmt panics on formatter error over an infallible sink), the fix is the canonical remedy (route through core::fmt::Write), and behavior is otherwise byte-identical. The 3-line comment prevents accidental "simplification" back to the panicking form. The test follows harness conventions: spawned subprocess via bunExe()/bunEnv, pipes drained concurrently with Promise.all, asserts on {stdout, stderr, exitCode}.
Other factors
All actionable feedback from my earlier reviews has been addressed and the threads are resolved: the test was rewritten as a spawned fixture, and both new comments were condensed to 3 lines per CLAUDE.md rule 13. My two remaining inline notes (identical pre-existing panics in jest.rs format_label and expect.rs print_value) were explicitly filed as non-blocking pre-existing issues; the author is entitled to keep this PR scoped to the Fuzzilli-reported site and address the siblings separately. The bug-hunting system found no new issues on the current revision.
|
@robobun rebase push |
|
Rebased onto main (ace8f42) and pushed. Clean rebase, no conflicts; the regression test passes on the rebuilt debug binary. |
e42fedb to
3f85ebb
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // std::io::Write::write_fmt on Vec<u8> panics if a Display impl returns | ||
| // fmt::Error with no underlying I/O error (to_fmt does when user JS | ||
| // throws). Use core::fmt::Write so the error is observable. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
3f85ebb to
2456c7c
Compare
|
Dropped the inline comment in |
There was a problem hiding this comment.
LGTM — the VecWriter/core::fmt::Write swap is the established idiom for this exact failure mode, and all prior feedback (spawned-subprocess test, comment-cop) has been addressed.
What was reviewed
- Confirmed
VecWriterimplements onlycore::fmt::Write(fmt.rs:794), sowrite!resolves to the non-panickingwrite_fmtand.is_err()now actually observes the error; the file-leveluse std::io::Write as _at BunObject.rs:74 can't interfere sinceVecWriterdoesn't implement it. - Pattern matches existing call sites in
JSGlobalObject.rs,FrameworkRouter.rs, andscanImportsAndExports.rs. - Regression test spawns a subprocess per the crash-fix convention, drains stdout/stderr/exited concurrently, and asserts
exitCode: 0+ the propagated JS error message — fails cleanly on regression rather than aborting the file. - The two pre-existing sibling sites I flagged earlier (
jest.rs format_label,expect.rs print_value) remain out of scope here; they're tracked in the earlier non-blocking comments.
Extended reasoning...
Overview
Two-file change: src/runtime/api/BunObject.rs swaps the write! sink in bun_inspect from &mut Vec<u8> (which resolves to std::io::Write::write_fmt and panics when a Display impl returns fmt::Error on an infallible stream) to bun_core::fmt::VecWriter(&mut array) under core::fmt::Write, so the formatter error surfaces as a Result and the existing .is_err() branch returns BunString::empty(). test/js/bun/cookie/cookie-expires-validation.test.ts gains a spawned-subprocess regression test that constructs a Cookie with an expires object whose inspect.custom throws.
Security risks
None. This is a crash-hardening change on the error-message rendering path (JSValueToStringSafe → Bun__inspect). No new inputs are accepted, no privilege boundaries are crossed, and the behavior on error (return empty string, let the pending JS exception propagate) is what the pre-existing .is_err() branch already intended.
Level of scrutiny
Low-to-moderate. The Rust change is a 7-line mechanical swap to an in-tree helper (bun_core::fmt::VecWriter) that exists precisely for this purpose — its doc comment and the three other call sites in the repo confirm this is the canonical shape. I verified the file-scope use std::io::Write as _; at BunObject.rs:74 cannot cause the wrong write_fmt to be picked because VecWriter does not implement std::io::Write. The success path is byte-identical (VecWriter::write_str just extend_from_slices into the same Vec<u8>).
Other factors
Every piece of prior review feedback on this PR has been addressed and resolved: the in-process toThrow() test was rewritten to a spawned subprocess; both 4-line comments were condensed; the final github-actions comment-cop note was addressed by dropping the inline comment entirely (robobun's 2026-08-04 19:32 reply), and the current diff confirms no comment remains. Jarred requested a rebase which completed cleanly, and robobun reports the test passes on the rebuilt debug binary. My earlier non-blocking notes about the identical pattern in jest.rs and expect.rs are pre-existing, out of scope, and already recorded on the PR — they don't block this fix.
2456c7c to
c5a940f
Compare
There was a problem hiding this comment.
LGTM — the VecWriter/core::fmt::Write route is correct and the comment-cop feedback is addressed.
What was reviewed:
bun_core::fmt::VecWriterimplements onlycore::fmt::Write(fmt.rs:794), sowrite!now returnsErrinstead of panicking insidestd::io::default_write_fmt;.is_err()observes it and returnsBunString::empty(), letting the pending user exception propagate.- Regression test spawns a subprocess (per the crash-fix rule), drains stdout/stderr/exited concurrently, and asserts the specific
"threw:boom from inspect.custom"message — so it locks in that the user's thrown error surfaces, not a generic ERR_INVALID_ARG_VALUE. - The two pre-existing sibling sites I flagged earlier (
jest.rs format_label,expect.rs print_value) remain unchanged; they were noted as non-blocking and can be follow-ups.
Extended reasoning...
Overview
Two files touched: src/runtime/api/BunObject.rs (7-line change to bun_inspect, swapping write!(&mut Vec<u8>, ...) — which resolves to std::io::Write::write_fmt and panics when a Display impl returns fmt::Error on an infallible sink — for write!(bun_core::fmt::VecWriter(&mut array), ...) via core::fmt::Write, which returns the error as a Result) and test/js/bun/cookie/cookie-expires-validation.test.ts (adds one subprocess-spawning regression test).
Security risks
None. This is an error-propagation fix inside the console formatter's inspect path. No parsing of untrusted input, no auth/crypto/permissions surface. It converts a user-reachable native panic (DoS class) into a normal JS throw, which is strictly a hardening.
Level of scrutiny
Low-to-medium. The change is mechanical: it swaps the Write trait the write! macro dispatches to so the existing .is_err() check actually fires. I verified VecWriter (fmt.rs:786-800) implements only core::fmt::Write with an infallible write_str, so the only way write! returns Err is ZigFormatter's Display::fmt returning fmt::Error — exactly the case being handled. The pattern matches existing usage in JSGlobalObject.rs:1461 and scanImportsAndExports.rs:537. The use core::fmt::Write; is scoped locally to the function, avoiding trait-resolution ambiguity elsewhere in the file.
Other factors
All prior review feedback is resolved: the test was rewritten to spawn a subprocess (so a regression fails cleanly rather than aborting the file), both over-length comments were condensed, and after the github-actions comment-cop fired post-rebase, the inline Rust comment was dropped entirely — VecWriter's doc comment already carries the rationale. The two pre-existing sibling panic sites in jest.rs and expect.rs I flagged earlier remain, but were explicitly marked non-blocking; they're separate subsystems and reasonable follow-ups. Robobun confirmed the regression test passes on the rebuilt debug binary after rebasing onto main. No bugs surfaced in this run's bug hunt.
…36911) ### Problem A `test.each` / `describe.each` title interpolating a value via `$path` (or `%p`) aborts the test runner when formatting that value throws, e.g. a throwing `[Symbol.for("nodejs.util.inspect.custom")]`: ```js import { test } from "bun:test"; test.each([{ a: { b: { [Symbol.for("nodejs.util.inspect.custom")]() { throw 1 } } } }])("case $a.b", () => {}); ``` ``` panic: a formatting trait implementation returned an error when the underlying stream did not ``` ### Cause `format_label` in `src/runtime/test_runner/jest.rs` formats non-string interpolated values with `write!(&mut list, "{}", value.to_fmt(&mut formatter))` through `std::io::Write`. `ZigFormatter`'s `Display` impl collapses the pending JS exception to `fmt::Error`, and `std::io::Write::write_fmt` panics when the `Display` impl errors while the sink (`Vec<u8>`) did not. The `%p` branch in the same function has the identical bug. ### Fix Add `Formatter::format_value`, a fallible single-value entry point that propagates the `JsError` (thrown exception, termination, OOM) instead of collapsing it to `fmt::Error`, and use it at both `format_label` sites. The thrown exception now surfaces as a test error and `bun test` exits 1 instead of crashing. The `Formatter::format_value` hunk is byte-identical to the one in #36912 so the two merge cleanly in either order. Sibling sites with the same `write!`-through-`Display` pattern that are intentionally not touched here because they already have their own PRs: `ExpectMatcherUtils::print_value` in `expect.rs` (#36912) and `bun_inspect` in `BunObject.rs` (#30980). ### Verification New test in `test/cli/test/bun-test.test.ts` covers `test.each` and `describe.each` for both the `$path` and `%p` sites, one fixture file per variant with a distinct error message (the declaration throw aborts module evaluation, so a single file can only exercise the first one). It aborts with the panic on a build without the fix and passes with it. The full `bun-test.test.ts` file passes. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/test/bun-test.test.ts <!-- robobun:evidence:end -->
…ven-sh#36911) ### Problem A `test.each` / `describe.each` title interpolating a value via `$path` (or `%p`) aborts the test runner when formatting that value throws, e.g. a throwing `[Symbol.for("nodejs.util.inspect.custom")]`: ```js import { test } from "bun:test"; test.each([{ a: { b: { [Symbol.for("nodejs.util.inspect.custom")]() { throw 1 } } } }])("case $a.b", () => {}); ``` ``` panic: a formatting trait implementation returned an error when the underlying stream did not ``` ### Cause `format_label` in `src/runtime/test_runner/jest.rs` formats non-string interpolated values with `write!(&mut list, "{}", value.to_fmt(&mut formatter))` through `std::io::Write`. `ZigFormatter`'s `Display` impl collapses the pending JS exception to `fmt::Error`, and `std::io::Write::write_fmt` panics when the `Display` impl errors while the sink (`Vec<u8>`) did not. The `%p` branch in the same function has the identical bug. ### Fix Add `Formatter::format_value`, a fallible single-value entry point that propagates the `JsError` (thrown exception, termination, OOM) instead of collapsing it to `fmt::Error`, and use it at both `format_label` sites. The thrown exception now surfaces as a test error and `bun test` exits 1 instead of crashing. The `Formatter::format_value` hunk is byte-identical to the one in oven-sh#36912 so the two merge cleanly in either order. Sibling sites with the same `write!`-through-`Display` pattern that are intentionally not touched here because they already have their own PRs: `ExpectMatcherUtils::print_value` in `expect.rs` (oven-sh#36912) and `bun_inspect` in `BunObject.rs` (oven-sh#30980). ### Verification New test in `test/cli/test/bun-test.test.ts` covers `test.each` and `describe.each` for both the `$path` and `%p` sites, one fixture file per variant with a distinct error message (the declaration throw aborts module evaluation, so a single file can only exercise the first one). It aborts with the panic on a build without the fix and passes with it. The full `bun-test.test.ts` file passes. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/test/bun-test.test.ts <!-- robobun:evidence:end -->
…ws during formatting Bun__inspect (used by JSValueToStringSafe to render values in error messages like ERR_INVALID_ARG_VALUE) wrote the console formatter output to a Vec<u8> via std::io::Write. When the value being formatted has a throwing [inspect.custom] (or any JS callback that throws during formatting), ZigFormatter's Display impl returns fmt::Error — but the Vec<u8> sink itself never errors, so std::io::Write::write_fmt panics with 'a formatting trait implementation returned an error when the underlying stream did not' instead of returning Err. Route through core::fmt::Write (via VecWriter) so the error is observable and we return an empty string as originally intended.
c5a940f to
f28a7bb
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![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/js/bun/cookie/cookie-expires-validation.test.ts`:
- Around line 103-107: Update the expected object in the subprocess assertion
around Promise.all to include stderr: "" alongside stdout and exitCode, ensuring
the test validates all three subprocess results.
🪄 Autofix
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: 3cd3ac7a-43c6-4bed-ac69-d85c5789df2b
📒 Files selected for processing (2)
src/runtime/api/BunObject.rstest/js/bun/cookie/cookie-expires-validation.test.ts
There was a problem hiding this comment.
LGTM — the core::fmt::Write/VecWriter swap is the right fix and all prior feedback (subprocess-based crash test, comment length, comment-cop) has been addressed.
What was reviewed:
- Confirmed
VecWriterimplements onlycore::fmt::Write(fmt.rs:794), sowrite!no longer routes throughstd::io::default_write_fmtand the.is_err()branch is now reachable. - Checked the file-level
use std::io::Write as _;(line 74) doesn't shadow —VecWriterhas noio::Writeimpl, so resolution is unambiguous. - Regression test spawns a subprocess, drains all three pipes concurrently, and asserts
{stdout, exitCode}together — fails cleanly on regression rather than aborting the file. - The two pre-existing sibling sites I flagged earlier (jest.rs
format_label, expect.rsprint_value) remain untouched; those are separate and non-blocking here.
Extended reasoning...
Overview
This PR fixes a Fuzzilli-found panic in Bun__inspect (src/runtime/api/BunObject.rs), the native helper C++ calls via JSValueToStringSafe to render a received value into error messages like ERR_INVALID_ARG_VALUE. When the value's [Symbol.for("nodejs.util.inspect.custom")] throws, ZigFormatter::fmt returns fmt::Error; the previous sink was &mut Vec<u8> via std::io::Write, whose write_fmt panics ("a formatting trait implementation returned an error when the underlying stream did not") before the existing .is_err() guard could observe it. The fix routes the write through bun_core::fmt::VecWriter under core::fmt::Write, which surfaces the error as a Result so the function returns BunString::empty() and the pending JS exception propagates. A subprocess-based regression test is added to test/js/bun/cookie/cookie-expires-validation.test.ts.
Security risks
None. This is an error-formatting path; the change replaces a process abort with propagation of the user's own thrown exception. No new inputs are parsed, no bounds are changed, and no allocation sizing is affected.
Level of scrutiny
Low-to-medium. The Rust change is 8 lines and mechanically swaps trait resolution to an existing in-tree adapter (VecWriter, fmt.rs:786) that is already used the same way in scanImportsAndExports.rs and braces.rs. VecWriter implements only core::fmt::Write and its write_str is infallible, so the only way .is_err() triggers is the Display impl erroring — exactly the case being handled. I checked that the file-level use std::io::Write as _; at BunObject.rs:74 cannot re-hijack resolution because VecWriter has no io::Write impl.
Other factors
This PR has been through several review rounds and every piece of feedback has been applied: the crash test was rewritten to spawn a subprocess (so a regression fails the test rather than aborting the runner); both new comments were condensed to ≤3 lines; and after the comment-cop bot fired post-rebase, the inline BunObject.rs comment was dropped entirely — the VecWriter doc comment already carries the rationale, and the call site now matches the other in-tree uses without a per-site note. The gate evidence in the PR description shows the test failing on both debug/ASAN and release without the fix and passing with it. My earlier non-blocking heads-ups about identical pre-existing patterns in jest.rs and expect.rs are recorded on the PR for follow-up but do not block this focused fix.
|
Closing: the panicking code no longer exists. #31831 (merged 2026-08-06) switched Verified on current main (bdb7382): |
2 participants
What does this PR do?
Fixes a panic in
Bun__inspect(the native helper used byJSValueToStringSafeinErrorCode.cppto render values in messages likeERR_INVALID_ARG_VALUE).When the value being rendered has a throwing
[Symbol.for("nodejs.util.inspect.custom")](or any user JS that throws while the console formatter walks the value),ZigFormatter'sDisplayimpl returnsfmt::Error. The sink was aVec<u8>viastd::io::Write, whosewrite_fmtpanics withbecause the in-memory stream never errored. The existing
.is_err()check never saw the error — the panic happens insidewrite_fmtbefore it returns.Fix: write through
core::fmt::Write(bun_core::fmt::VecWriter) so the error is observable and we return an empty string as the code originally intended. The user's thrown exception then propagates normally.Repro (crashed before, throws cleanly after)
How did you verify your code works?
panic:a formatting trait implementation returned an error when the) with the minimal repro above; symbolicated stack confirmsstd::io::default_write_fmt::<Vec<u8>>→bun_inspect→JSValueToStringSafe.test/js/bun/cookie/cookie-expires-validation.test.tsthat crashes on the baked Bun and passes with this change.[stamp-90s] gate passed · iteration 15 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 15
evidence per changed file