Conversation
…ric matchers expect([,1]).toEqual([expect.any(Date), 1]) segfaulted at 0x5: the hole arrives in matchAsymmetricMatcherAndGetFlags as the empty JSValue, and isString()/isObject()/hasInstance() dereference asCell() == nullptr. Normalizing empty to jsUndefined() at the top of the function makes every matcher path safe and also fixes expect.anything() incorrectly accepting a hole (Jest treats holes as undefined, which anything() rejects).
|
Status: CI is green on current main (build #114787). Ready for review. Reproduced on stock bun 1.4.3-canary.1 with With the debug build both new tests pass. Full |
|
Updated 11:13 AM PT - Sep 12th, 2026
✅ @robobun, your commit e0c9310714227af09327590e9d386fcd6b2eaf5c passed in 🧪 To try this PR locally: bunx bun-pr 34647That installs a local version of the PR into your bun-34647 --bun |
|
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 (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughThe change normalizes empty array properties to ChangesAsymmetric matcher array-hole handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change is covered by passing targeted and full expect tests, with no unresolved merge-blocking concerns. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — minimal, well-placed crash fix with thorough test coverage.
What was reviewed:
- Verified all four call sites of
matchAsymmetricMatcherguardmatcherPropas non-empty, so onlyotherPropneeds normalizing — the fix is at the right layer and protects every caller. - Checked that
readFlagsAndProcessPromisecannot re-introduce an empty value after normalization. - Test covers every crashing matcher branch (
any,stringContaining,stringMatching,objectContaining,closeTo), both hole directions, nested arrays,Array(n)/delete, and theexpect.anything()semantic fix.
Extended reasoning...
Overview
This PR fixes a segfault in bun:test when a sparse array element (hole) is compared against an asymmetric matcher via toEqual. The fix is a 4-line normalization at the top of matchAsymmetricMatcherAndGetFlags in src/jsc/bindings/bindings.cpp that converts an empty JSValue to jsUndefined() before any matcher branch touches it. A 26-line test block is added to test/js/bun/test/expect.test.js covering the full variant matrix.
Security risks
None. This is a defensive null-guard on an internal comparison path in the test runner. No user-controlled input reaches new allocations, syscalls, or external boundaries.
Level of scrutiny
Low-to-medium. The mechanism is well-explained (empty JSValue → isCell() true → asCell() null deref), the fix is a single defensive branch at the function entry, and the semantics (hole ≡ undefined) match both Jest and the existing toEqual array-hole handling elsewhere in Bun__deepEquals. The change cannot regress non-hole inputs since isEmpty() is false for every real JSValue.
Other factors
- Right layer: REVIEW.md asks for fixes at the layer owning the invariant. All four callers of
matchAsymmetricMatcher(AndGetFlags)already guard the matcher side with!isEmpty()/ non-null cell checks, but pass the other side unguarded — normalizing inside the callee protects all of them at once (including theBun__deepMatchpath used bytoMatchObject/objectContaining). - Test quality: Follows repo guidance — added to the existing
expect.test.jsfile inside thedeepEquals with asymmetric matchersdescribe block, covers every matcher branch that dereferencesotherProp, both directions (hole on received vs expected), all three hole-production forms (literal,Array(n),delete), a nested case, theexpect.anything()semantic bug, and a positive sanity assertion. PR description confirmsUSE_SYSTEM_BUN=1reproduces the crash and the debug build passes. - No collateral risk:
otherPropis a by-value parameter; reassigning it has no effect on callers.readFlagsAndProcessPromiseround-trips the (now-defined) value through encode/decode, which is safe forjsUndefined().
There was a problem hiding this comment.
LGTM — clean segfault fix, guard placed at the shared helper.
What was reviewed:
- Traced all four
matchAsymmetricMatchercall sites;matcherPropis always cell-guarded, onlyotherPropcan arrive empty — normalizing at function entry covers every path. - Walked each matcher branch (
anything,any,stringContaining/Matching,objectContaining,arrayContaining,closeTo) withjsUndefined()— all resolve to FAIL as Jest specifies; no branch relied on the empty sentinel. - Test covers both hole directions, all crashing matcher types, the
anything()false-positive, and alternate hole producers (Array(n),delete).
Extended reasoning...
Overview
The PR fixes a segfault in bun:test's deep-equality path when a sparse-array hole is compared against an asymmetric matcher (expect.any(...), expect.stringContaining, etc.). Bun__deepEquals reads holes via getIndexWithoutAccessors, which returns the empty JSValue; that empty value flowed into matchAsymmetricMatcherAndGetFlags where isCell() reports true for empty and subsequent asCell()-based type queries dereference null. The fix is a 3-line normalization at the top of matchAsymmetricMatcherAndGetFlags (bindings.cpp:313) turning empty into jsUndefined(), plus a comprehensive regression test in expect.test.js.
Security risks
None. This is test-runner deep-equality code; no untrusted input, network, or filesystem surface. The change strictly hardens against a null-cell dereference.
Level of scrutiny
Low-to-medium. The fix is a defensive one-liner in a shared helper. I verified it's placed at the right layer per REVIEW.md guidance: matchAsymmetricMatcher has four call sites (bindings.cpp:673, 683, 1685, 1699), and normalizing inside the callee protects all of them without duplicating guards. matcherProp cannot be empty at these sites (each guards isCell() && !isEmpty() or equivalent before the call), so only otherProp needed the check. I walked every downstream branch with jsUndefined() substituted — isUndefinedOrNull() → FAIL for anything() (fixing the false-PASS), isCell()/isString()/isObject()/isNumber() all false so each matcher returns FAIL, and hasInstance(globalObject, jsUndefined()) is well-defined. Nothing depended on the empty sentinel semantically.
Other factors
The test is thorough: it exercises the hole on both received and expected sides (both call-site orderings), every matcher type that crashed plus closeTo, the anything() correctness bug (not just the crash), nested arrays, and all three hole-production methods ([,x], Array(n), delete). It includes a positive sanity assertion that holes still deep-equal undefined. PR description confirms USE_SYSTEM_BUN=1 reproduces the crash and the debug build passes; full expect.test.js remains green. CI failures listed by robobun are unrelated pre-existing flakes. No prior human review comments to address.
…e-asymmetric-matcher-crash
An index past the end of the shorter array, and an accessor at the index, come back from getIndexWithoutAccessors as the empty JSValue, the same as a hole. expect([expect.any(Number)]).not.toEqual([]) took the same path into matchAsymmetricMatcherAndGetFlags and crashed at 0x5. The existing normalization covers it. Add tests for the length-mismatch shapes (toEqual, toBeOneOf, toContainEqual, Map entries) and make the comment name every source of the empty value.
There was a problem hiding this comment.
LGTM — the empty→jsUndefined() normalization at the top of matchAsymmetricMatcherAndGetFlags is the right layer for this and covers all four matchAsymmetricMatcher call sites.
What was reviewed:
- Traced the empty
JSValuefromgetIndexWithoutAccessorsthrough the non-strict array loop at bindings.cpp:993 into the recursiveBun__deepEquals→matchAsymmetricMatcherpath; the guard sits before everyotherProp.isCell()/asCell()/hasInstanceuse. - Checked that
matcherPropitself is alreadyisCell()-guarded at every call site (829/840/2250/2264), so onlyotherPropneeded the empty check. - Test coverage hits the full variant matrix (every matcher type, both sides, nested,
Array(n)/delete, off-end index,toBeOneOf/toContainEqual/Map, accessor index) plus theexpect.anything()wrong-verdict case; all in-process, no timing or network.
Extended reasoning...
Overview
This PR fixes a segfault in bun:test's deep-equality path when an asymmetric matcher (expect.any(), expect.stringContaining(), etc.) is aligned against an array hole or an index past the end of the shorter array. The fix is a 4-line guard at the top of matchAsymmetricMatcherAndGetFlags in src/jsc/bindings/bindings.cpp that normalizes an empty JSValue otherProp to jsUndefined(). Two new it() blocks in test/js/bun/test/expect.test.js (~50 lines) cover the variant matrix.
Security risks
None. This is test-runner assertion logic; no untrusted-input parsing, no auth/crypto/permission surface. The change turns a null-cell dereference (crash) into a well-defined undefined comparison.
Level of scrutiny
Low-to-moderate. The C++ change is 4 lines with an obvious mechanism: JSC's empty JSValue reports isCell() as true with a null cell, and downstream code (otherProp.asCell()->type(), constructorObject->hasInstance(globalObject, otherProp), otherProp.isString()) dereferences it. Normalizing to jsUndefined() at function entry is consistent with how the non-strict array loop already treats holes (bindings.cpp:1009 short-circuits when the other side is undefined, and the second loop at :1019 treats empty as undefined). I verified the four call sites of matchAsymmetricMatcher all guard the matcher-side argument with isCell() before calling, so only otherProp can arrive empty — the single-sided guard is sufficient. The one-line comment names the provenance of the empty value, which is load-bearing per REVIEW.md.
Other factors
Test coverage is thorough per REVIEW.md's "cover the variant matrix" rule: every built-in asymmetric matcher, hole on both received and expected sides, nested arrays, alternate hole producers (Array(n), delete), matcher past end of shorter array, entry via toBeOneOf/toContainEqual/Map, an accessor-defined index, plus sanity checks. Tests are pure in-process assertions (no spawning, timing, or network), placed in the existing describe("expect()") block. The PR description states stock bun exits 139 on these, satisfying the fails-under-USE_SYSTEM_BUN=1 requirement. No CODEOWNERS entry covers these paths. Since the last review, commits a7d3016 and e0c9310 added the off-end-index test block and shortened the C++ comment; the bug-hunt ran to dry_streak with no findings.
|
Closing in favor of #42536. #42536 fixes the same crash: an asymmetric matcher that receives the empty value for an array hole, or for an index past the end of the shorter array. It fixes it at the source. The non-strict array loops in I ran all 28 assertions from the two test blocks of this PR against a debug (ASAN) build of #42536 at e287ff9. All of them pass. I also ran 95 more shapes, each in its own process. They cover The guard in |
Problem
bun testwithpanic(main thread): Segmentation fault at address 0x5(ASAN:SEGV on unknown address 0x000000000005):expect([, 1]).toEqual([expect.any(Date), 1])andexpect([expect.any(Number)]).not.toEqual([]).Bun__deepEqualsreads elements withgetIndexWithoutAccessors(src/jsc/bindings/bindings.cpp:682). It returns the emptyJSValuefor a hole and for an index past the end of the shorter array. The array loop (:993) passes it tomatchAsymmetricMatcherAndGetFlagsasotherProp.isCell()is true for the empty value, soisString(),isObject()andhasInstance()read a null cell.expect.anything()does not crash, but it accepts the empty value.Fix
matchAsymmetricMatcherAndGetFlags(bindings.cpp:344) replaces an emptyotherPropwithjsUndefined()first.toEqualalready reads a hole or a missing trailing element asundefined. Jest does the same: its non-strictequalscalls the matcher withundefinedfor a key that the other side does not have.test/js/bun/test/expect.test.js, two new tests. Stock bun exits 139 on both. Also ranjest-extended.test.jsand bothexpect-extend*files.Background
expect.any(Number)returns.toEqualgives it the value on the other side and uses its verdict.JSValueis the "no value" encoding in JavaScriptCore, notundefined.isCell()is true for it andasCell()is null, so code must testisEmpty()first.toStrictEqualis not affected. It returns on a hole or a length mismatch before it reaches a matcher.Notes
Shapes that crash on stock bun 1.4.3-canary.1 (
Segmentation fault at address 0x5, exit 139). All pass with this change:The same crash with
expect.any(String),expect.any(Foo),expect.stringContaining,expect.stringMatchingandexpect.objectContaining.Wrong verdict, no crash, on stock bun:
expect([, 1]).toEqual([expect.anything(), 1])andexpect([expect.anything()]).toEqual([])pass.isUndefinedOrNull()is false for the empty value. Both are a mismatch now.Shapes that were already a clean result on stock bun: the matcher in the shorter array (
expect([]).not.toEqual([expect.any(Number)]), the second loop atbindings.cpp:1019returns false),toStrictEqual,toMatchObject(it compares array lengths first),toHaveBeenCalledWithwith more or fewer arguments.The accessor test uses a getter whose value the matcher rejects. The assertion holds whether
toEqualskips the accessor or calls it, so the test pins the crash and not that choice.Jest agrees with the new verdicts. Its non-strict
equals(packages/expect-utils/src/jasmineUtils.ts) compares array lengths only fortoStrictEqual. For a key that holds a matcher on one side and is missing on the other side, it calls the matcher withundefined. Soexpect([expect.not.stringContaining("a")]).toEqual([])passes in Jest, and it passes with this change.The other direction is not changed here. With the matcher in the longer expected array, the second loop (
bindings.cpp:1019) returns a mismatch and does not call the matcher. The verdict differs from Jest only for a matcher that acceptsundefined, for exampleexpect([]).toEqual([expect.not.stringContaining("a")]). The same holds for a missing object key. That is a separate change.[human-review] gate passed · iteration 6 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 6
evidence per changed file