Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughChangesMap and Set size coercion
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:11 AM PT - Aug 26th, 2026
❌ @robobun, your commit 8052536 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 39237That installs a local version of the PR into your bun-39237 --bun |
|
CodeRabbit hit its review limit on this one, so there is no automated review here. The change itself is the two coerce_to_i32 lines in pretty_format.rs plus the new test; CI is running on #98791. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — two-line swap from to_int32() to coerce_to_i32(global)?, mirroring the identical size handling in ConsoleObject.rs.
What was reviewed:
- Confirmed
ConsoleObject.rs:4659/4801uses the samecoerce_to_i32pattern for Map/Setsize. - Checked the early
?doesn't leak formatter state —quote_stringsis saved/mutated only after the coercion succeeds. - The remaining
to_int32()calls inpretty_format.rs(ExpectCloseTocached number/digits) are pre-validated numbers, not user-shadowable getters — different class. - Test covers all four tag variants (WeakSet/WeakMap/Set/Map) with distinct non-numeric
sizetypes and asserts full matcher output via inline snapshot.
Extended reasoning...
Overview
Two-line change in src/runtime/test_runner/pretty_format.rs: the Tag::Map and Tag::Set branches now call length_value.coerce_to_i32(self.global_this)? instead of length_value.to_int32() when reading the size property. to_int32() is a non-throwing accessor that assumes the value is already a number and falls through to JSC's asInt32() (a debug assertion / release UB) on anything else. coerce_to_i32 runs the full ToInt32 abstract operation and propagates any exception via JsResult. Accompanied by a new subprocess test that exercises WeakSet, WeakMap, Set and Map with shadowed non-numeric size properties.
Security risks
None. This is the jest pretty-formatter used for expect() diffs and snapshots — output-only, in-process, no I/O or auth surface. The change replaces an unchecked cast with a proper coercion, which is strictly safer.
Level of scrutiny
Low. The fix is mechanical and copies the exact pattern already used at src/jsc/ConsoleObject.rs:4657-4659 and :4799-4801 for the same property on the same JS types. The surrounding function already returns JsResult (the .get(...)? on the preceding line confirms it), so the added ? composes cleanly. I checked that the early return on a throwing coercion doesn't leave self.quote_strings or self.indent in a bad state — both are saved/mutated only after the coercion, so no restoration is needed on that path.
Other factors
- Bug-class coverage: grepped for other
to_int32()calls inpretty_format.rs; the two remaining ones (lines 2834-2835) read cachedExpectCloseTovalues that are validated at matcher construction — not the same shadowable-prototype-getter class. - Test quality: spawns a subprocess with
bunExe()/bunEnv, drains stdout/stderr/exited concurrently, asserts stderr is empty, usesnormalizeBunSnapshot+ inline snapshot on the full matcher output for all four types, and assertsexitCodelast — matches the harness conventions in CLAUDE.md. The PR description states it fails on the current release ("Type error") and aborts on unfixed debug, satisfying the fails-for-the-right-reason requirement. - No prior reviews or outstanding comments.
|
Nothing to change from the review. Format, clippy and the lint jobs are green; waiting on the Buildkite build (#98791) before this is ready to merge. |
The jest pretty formatter used for expect() failure messages read the size property off Map, Set, WeakMap and WeakSet values and passed it to to_int32(), which only accepts numbers. A user-defined size property holding anything else hit the isInt32() assertion in debug builds and read garbage in release builds, where a WeakSet or WeakMap then failed with a bare TypeError instead of the matcher message. Coerce the value the same way ConsoleObject already does.
5e1dc93 to
8052536
Compare
|
The fuzzer reported a second instance of this crash at 93eecdc, a WeakMap whose const v2 = new WeakMap();
v2.size = v2;
const v3 = {};
v3[Symbol] = v2;
const v5 = Bun.jest();
v5.expect(v5).toStrictEqual(v3);It is the same |
|
The CodeRabbit pass on the rebased head (8052536) found nothing to act on. Format, clippy and the lint jobs are green again; the Buildkite build is #106177. |
There was a problem hiding this comment.
LGTM — two-line fix aligning the jest pretty formatter with the identical coerce_to_i32(self.global_this)? pattern already used in ConsoleObject.rs.
What was reviewed:
- Both
Tag::MapandTag::Setbranches now coerce and propagate via?; the early-return-on-zero and indent/quote_strings restore paths still hold. - Checked the remaining
to_int32()calls in this file (lines 2810-2811) — those read internally-cachedExpectCloseTovalues, not user-shadowable properties, so not the same bug class. - Test follows harness conventions (bunExe/bunEnv, concurrent pipe drain, stderr/stdout asserted before exit code, inline snapshot); the new commit adds a nested self-referential
sizecase.
Extended reasoning...
Overview
The PR fixes a crash in the test-runner's jest-style pretty formatter (src/runtime/test_runner/pretty_format.rs) when formatting a Map/Set/WeakMap/WeakSet whose size property has been shadowed with a non-number. Two call sites change to_int32() (which asserts isInt32() and is UB on non-numbers) to coerce_to_i32(self.global_this)?, which performs proper JS ToInt32 coercion and propagates any thrown exception. This exactly mirrors the existing handling in src/jsc/ConsoleObject.rs:4570 and :4712 for the same property read. A new test spawns a subprocess exercising five variants (WeakSet with {}, WeakMap with "abc", Set with defined-property {}, Map with null, and a nested WeakMap whose size is itself) and snapshots the matcher output.
Security risks
None. This is output formatting for test-failure messages; the only user-controlled data is the value being formatted, and the change moves from an unchecked cast to a proper throwing coercion — strictly safer. No auth, crypto, network, or filesystem paths are touched.
Level of scrutiny
Low-to-moderate. The diff is two lines of native code plus a test. The fix is mechanical (swap a non-throwing accessor for the coercing equivalent), follows an established in-tree pattern verbatim, and satisfies REVIEW.md's "coercing conversions (toInt32(global), never asInt32 on user values)" rule directly. Exception propagation via ? is correct here because the enclosing function already returns JsResult and both branches already use ? on the preceding .get() call. I checked the two other to_int32() calls in the same file; they operate on internally-cached matcher values (ExpectCloseTo number/digits), not user-shadowable prototype getters, so they are not part of this bug class.
Other factors
The test file follows the repo's harness conventions cleanly: bunExe()/bunEnv, -e for a single-file subprocess, await using on the spawn handle, Promise.all to drain both pipes concurrently, stderr and stdout asserted before the exit code, and normalizeBunSnapshot with an inline snapshot. The PR body includes gate evidence showing the test fails on main (debug assertion / release TypeError) and passes with the fix. No CODEOWNERS entry covers src/runtime/test_runner/ or test/js/bun/test/. The only prior review was COMMENTED (non-blocking), and a follow-up commit since then only extended test coverage. Exit reason was dry_streak.
|
Nothing to change from the second review either. The Buildkite build (#106177) has finished: 180 of 181 jobs passed. The one red test, test/cli/run/require-cache.test.ts on debian 13 x64-asan, is a timeout in a require.cache leak test that also fails on main and does not touch this code. I reported it for main-break triage separately. The other failures in that build passed on retry. The new test here passed on every lane. |
|
Closing: this was fixed on main by #41148 (commit 4e1eeff). That change handles a non-numeric Verified on a debug build of main at 1564c1e:
One difference between the two approaches: #41148 treats any non-number |
1 participant
What does this PR do?
Found by fuzzing. A failing matcher on a
Map,Set,WeakMaporWeakSetthat carries a non-numericsizeproperty took down the process while the failure message was being built.Debug build:
Release build: the matcher message is replaced by a bare
TypeError: Type errorforWeakSet/WeakMap(the garbage length is non-zero, so the formatter tries to iterate a non-iterable), and aSet/Mapwhosesizewas shadowed happens to print by accident.Cause
The
Tag::MapandTag::Setbranches of the jest pretty formatter (src/runtime/test_runner/pretty_format.rs, used forexpect()diffs and snapshots) readsizewith an ordinary property get and then calledto_int32()on it.to_int32()only handles numbers; for anything else it falls through toJSC__JSValue__toInt32, which is a plainasInt32().sizeis only a prototype getter, so an own property of any type shadows it. This predates the Rust port, the Zig version had the sametoInt32()call.console.log/Bun.inspect(ConsoleObject.rs) read the same property but run it throughcoerce_to_i32, which is why they never crashed on the same values.Fix
Use
coerce_to_i32(global)?in both branches, matchingConsoleObject.rs. A non-numericsizenow coerces the way it does forconsole.log({}-> 0,"2"-> 2), and asizewhosevalueOfthrows propagates the exception the same way a throwingsizegetter already did.With the fix the repro above reports a normal matcher failure:
How did you verify your code works?
WeakMap, shadowedSetand shadowedMapvariants all print a matcher failure and exit 1 with the debug build instead of aborting.test/js/bun/test/expect-map-set-size-crash.test.tsfails on the current release ("Type error"messages) and on an unfixed debug build (abort), passes withbun bd test.test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts(coversMap/Set/WeakMap/WeakSetpretty printing) andexpect-formdata-tojson-crash.test.tsstill pass with the debug build.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file