Repository navigation
fix(bun:test): handle a non-numeric size property when formatting a Set or Map diff - #41148
Conversation
… diff The jest pretty formatter reads the `size` property of Set, Map, WeakSet and WeakMap values and called `to_int32()` on it. WeakSet and WeakMap have no `size` accessor, and a Set or Map instance can shadow its accessor with an own property. A non-number then reached `JSC::JSValue::asInt32()`, which asserts in debug builds and reads garbage in release builds. Only coerce the value when it is a number. Treat any other value as 0, so the diff renders and no ToNumber conversion runs on a user value.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughChangesCollection size formatting
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change safely handles non-numeric Set and Map size values during diff formatting and includes focused regression coverage; no actionable merge-blocking risk remains after normal checks and review. 🚥 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 for the asInt32() assertion, both Map and Set arms guarded identically.
What was reviewed:
- Both formatting arms (Map at pretty_format.rs:1721, Set at :1769) now gate on
is_number()before coercing; non-numericsizefalls back to 0, so Symbol/throwing-valueOf never reach ToNumber. - Checked the two remaining
to_int32()calls in this file (:2818-2819) — those readExpectCloseTo's internally-cached number/digits, not a user-overridable property, so not the same bug class. - Test spawns with
bunExe()/bunEnv -e, drains stdout/stderr/exited concurrently, asserts positive diff output before exit code, and covers WeakSet/WeakMap/Set/Map × constructor/string/object/Symbol.
Extended reasoning...
Overview
This PR fixes a fuzzer-found debug-build assertion (ASSERTION FAILED: isInt32()) in the Jest pretty formatter at src/runtime/test_runner/pretty_format.rs. When rendering a toEqual diff for Set/Map/WeakSet/WeakMap, the formatter reads the size property and previously called to_int32() unconditionally. For non-numeric values (WeakSet/WeakMap have no prototype size, and Set/Map can have it shadowed via Object.defineProperty), to_int32() falls through to JSC__JSValue__toInt32, which calls JSValue::asInt32() and aborts on the debug assertion. The fix guards with is_number() and uses the throwing coerce_to_i32(global)? when numeric, defaulting to 0 otherwise. A new test covers all four collection types with constructor, string, plain object, and Symbol as size.
Security risks
None. This is output formatting in the test runner's diff renderer. No untrusted-input parsing, no filesystem/network access, no auth/crypto. The change strictly narrows the code path (fewer values reach the FFI coercion), and the ? on coerce_to_i32 propagates any exception rather than swallowing it.
Level of scrutiny
Low-to-moderate. The change is 8 lines across two identical sibling arms and directly implements the REVIEW.md rule "Never call non-throwing accessors (asNumber, jsCast, getDirect) on user values without validating type first" / "coercing conversions (toInt32(global), never asInt32 on user values)". I verified the whole bug class was addressed in this file: the only other .to_int32() calls (lines 2818-2819) operate on ExpectCloseTo's internally cached number/digits values, which are set by Bun's own matcher construction rather than read off a user object, so they are not the same class. The is_number() gate also means ToNumber never runs on the user value, so a Symbol or throwing valueOf cannot replace the diff with its own error — the PR description's stated design goal holds.
Other factors
The test follows harness conventions: bunExe()/bunEnv, -e inline script, Promise.all draining stdout/stderr/exited, stdout asserted before exit code, positive assertion on the diff header (not a "no panic" check). It covers the variant matrix across all four collection types and four non-numeric value shapes. No CODEOWNERS entry covers the changed paths. The bug hunt ran to dry_streak with no findings and no outstanding reviewer objections in the timeline. The only minor nit is that the test file's header comment says "Regression test" without a linked issue number and lives in test/js/bun/test/ rather than an existing file — acceptable given there's no obvious existing pretty-format test file to append to, and not worth blocking on.
2 participants
What does this PR do?
Fixes a fuzzer-found assertion failure in the jest pretty formatter:
The formatter in
src/runtime/test_runner/pretty_format.rsreads thesizeproperty ofSet,Map,WeakSetandWeakMapvalues while it renders anexpect().toEqual()failure diff. It calledto_int32()on that value.WeakSetandWeakMaphave nosizeaccessor on their prototype. ASetorMapinstance can also shadow its accessor withObject.defineProperty. In both cases a user-defined ownsizeproperty comes back as is. When that value is not a number,to_int32()falls through toJSC__JSValue__toInt32, which callsJSValue::asInt32(). A debug build aborts on the assertion. A release build reads a garbage length and reports a bareTypeErrorinstead of the diff.Repro. This aborts a debug build before this change:
The fix coerces the value only when it is a number. Any other value counts as 0. ToNumber never runs on the user value, so a
Symbolor a throwingvalueOfcannot replace the diff with its own error. The repro now prints the normal diff:This supersedes #31292. The stale bot closed that PR after 90 days. The change is the same, rebased onto current main, where the crash still reproduces. The earlier review threads (CodeRabbit and the Symbol edge case) are already addressed in this diff. #30373 covers the weak-collection case with a different approach. The two changes compose.
How did you verify your code works?
toEqualerror.test/js/bun/test/pretty-format-overridden-size.test.ts. It coversWeakSet,WeakMap,SetandMapwith a constructor, a string, a plain object and a symbol assize. It fails without the fix (a debug build aborts, a release build printsType errorfor three of the five cases) and passes with the fix.SetandMapdiff output is unchanged.[human-review] gate passed · iteration 6 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 6
evidence per changed file