Wasm: accept subtypes in i31.get_s/u and any.convert_extern, survive a failed BBQ compile - #532
Wasm: accept subtypes in i31.get_s/u and any.convert_extern, survive a failed BBQ compile#532robobun wants to merge 1 commit into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it relaxes Wasm GC type validation (which gates what the JIT tiers will compile), a human familiar with the Wasm backends should confirm each tier's addI31GetS/U and addAnyConvertExtern handle a bottom-typed (always-null) operand safely.
What was reviewed
- The three
isSubtypechecks in WasmFunctionParser.h match the existing pattern used forarray.len(line 2700),ref.castexternref (line 2927), andextern.convert_any(line 3037). - The BBQPlan null-check: verified
compileFunction()callsfail()before returningnullptr(line 174-177), and the early-return mirrors the existingdidFailToAllocate()handling just below. - The new stress test follows JSTests/README.md conventions:
wasmTestLoopCountfor tier-up, throwing assertions, no stdout, correct directory.
Extended reasoning...
Overview
This PR touches three files: Source/JavaScriptCore/wasm/WasmFunctionParser.h (three one-line validator changes replacing exact-type checks with isSubtype checks for i31.get_s, i31.get_u, and any.convert_extern), Source/JavaScriptCore/wasm/WasmBBQPlan.cpp (a 4-line null-check after compileFunction() in BBQPlan::work()), and a new regression test JSTests/wasm/stress/i31-get-bottom-type.js. The parser change brings these three opcodes in line with the Wasm GC spec, which types their operands as (ref null i31) / (ref null extern) and therefore admits any subtype including the bottom types none / noextern. The BBQPlan change hardens against a null-pointer dereference when a BBQ-tier compile fails validation after the module already loaded.
Security risks
The validator change is the security-relevant piece: it widens the set of operand types the parser accepts for three GC opcodes. If any downstream JIT tier's addI31GetS/U or addAnyConvertExtern implementation assumed the operand type was exactly i31ref/externref and did something unsafe with a (ref none) or (ref noextern) operand, this could introduce type confusion. The PR description states that each JIT tier already emits a runtime null trap for nullable operands and IPInt checks null unconditionally, and the new test exercises the null-trap path across tiers via wasmTestLoopCount. The pattern is identical to how array.len, ref.cast, and extern.convert_any already validate their operands in the same file, which strongly suggests the tiers are built for subtype inputs. Still, validator relaxations in a JIT engine are the kind of change where a second pair of eyes from someone who knows the BBQ/OMG lowering paths is prudent. The BBQPlan null-check is purely defensive and reduces risk (crash → graceful degradation to interpreter tier).
Level of scrutiny
Medium. The diff is small (~15 lines of production code), mechanically follows established patterns in the same file, and comes with a targeted regression test that the author reports fails on main and passes with the fix under debug ASAN. However, per the JSC CLAUDE.md guidance that "a change in one tier usually needs changes in others," and because Wasm type validation gates what the JIT compiles, a reviewer familiar with the Wasm tier implementations should sign off rather than an automated approval.
Other factors
I confirmed BBQPlan::compileFunction() does call fail() before returning nullptr (WasmBBQPlan.cpp:174-177), so the new early-return correctly relies on the plan already being completed — matching the linkBuffer.didFailToAllocate() branch immediately below it. No CODEOWNERS file governs these paths. The test file adheres to JSTests conventions (uses wasmTestLoopCount, throwing assertions from ../assert.js, no logging, placed in wasm/stress/, includes a //@ runDefaultWasm directive to force tier-up thresholds). No outstanding reviewer objections in the timeline.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 5 included reviews per hour; 2 remain after this review. WalkthroughThe change updates WebAssembly subtype validation, prevents failed BBQ compilation from installing missing code, and adds stress tests for bottom-type references, null traps, externref conversion, and ChangesWebAssembly bottom-type handling
Merge Risk: ⚪ Minimal · up to This PR broadens valid WebAssembly reference typing and prevents a failed tiered compile from crashing the process; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the problem, fix, background, affected behavior, and verification results. However, it does not follow the repository template because it omits the bug title, Bugzilla link, review status, and changed-file/function list.
Comment |
Preview Builds
|
…operands i31.get_s, i31.get_u and any.convert_extern required their operand type to be exactly i31ref or externref. The wasm GC spec accepts any subtype, including the bottom types none and noextern. Modules that other engines accept failed to validate with 'expected I31ref'. The same exact-match check, combined with a block-result typing bug the previous WebKit upgrade fixed, made the background BBQ tier-up compile fail on modules that already validated, and BBQPlan::work dereferenced the failed result: the segfault reported in issue #40770. The engine now checks subtypes (oven-sh/WebKit#532) and a failed BBQ compile leaves the function on the interpreter tier instead of crashing. Fixes #40770
…a failed BBQ compile The validator required the operand of i31.get_s and i31.get_u to be exactly I31ref, and the operand of any.convert_extern to be exactly Externref. The wasm GC spec types these operands as (ref null i31) and (ref null extern), so any subtype is valid, including the bottom types none and noextern. A module with (i31.get_s (ref.null none)) was rejected with 'expected I31ref' even though other engines accept it. Use isSubtype, like every other reference-typed operand check in the parser. Each tier already handles a nullable operand with a runtime null trap, and the IPInt interpreter checks null unconditionally. BBQPlan::work also dereferenced the result of compileFunction without a null check. A BBQ-tier validation failure cannot happen for a module that already validated, but bugs that broke that invariant (such as the block-result typing fixed in 8f229fb) turned into a segfault on the compiler thread, reported in oven-sh/bun#40770 as a crash in computeExceptionHandlerAndLoopEntrypointLocations. Return early instead; compileFunction already completed the plan through fail(), and the function keeps running in the interpreter tier. JSTests/wasm/stress/i31-get-bottom-type.js covers the accepted subtypes, the runtime trap, and the br_on_null block-result pattern from the bun issue across the BBQ and OMG tiers.
966a3d7 to
8c9008b
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. |
…operands i31.get_s, i31.get_u and any.convert_extern required their operand type to be exactly i31ref or externref. The wasm GC spec accepts any subtype, including the bottom types none and noextern. Modules that other engines accept failed to validate with 'expected I31ref'. The same exact-match check, combined with a block-result typing bug the previous WebKit upgrade fixed, made the background BBQ tier-up compile fail on modules that already validated, and BBQPlan::work dereferenced the failed result: the segfault reported in issue #40770. The engine now checks subtypes (oven-sh/WebKit#532) and a failed BBQ compile leaves the function on the interpreter tier instead of crashing. Fixes #40770
1 participant
Problem
i31.get_sandi31.get_uto be exactly I31ref, and the operand ofany.convert_externto be exactly Externref. The spec types these operands as(ref null i31)and(ref null extern), so any subtype is valid, including the bottom typesnoneandnoextern.(i31.get_s (ref.null none))fails with "expected I31ref". Other engines and the reference interpreter accept it.BBQPlan::workdereferences the result ofcompileFunctionwithout a null check. When a BBQ-tier compile fails, it segfaults atfunction->bbqLoopEntrypoints(address 0x28) on the compiler thread. Segfault in the BBQ wasm JIT oni31.get_s (br_on_null ...)of a block result bun#40770 hit this through the block-result typing bug fixed in 8f229fb: the BBQ pass kept(ref none)for a block result where the validating pass used the declaredi31ref, so only the BBQ pass failed the exact-match check.Fix
isSubtypefor the three checks, like every other reference-typed operand check in the parser. Each JIT tier already emits a runtime null trap for a nullable operand, and the IPInt interpreter checks null unconditionally, so a bottom-typed (always null) operand traps correctly at runtime.BBQPlan::workwhencompileFunctionreturns null.compileFunctionalready completed the plan throughfail(). The function keeps running in the interpreter tier instead of crashing the process.JSTests/wasm/stress/i31-get-bottom-type.jsfails on main ("doesn't validate: i31.get_s ref to type (ref null none) expected I31ref") and passes with the fix.run-jsc-stress-tests JSTests/wasm.yaml --filter i31and--filter externpass on a debug ASAN build, except two pre-existing failures that fail identically on an unfixed shell (gc/i31.wast.js.wasm-collect-continuously, aJSPromiseassertion flake, andexternref-result-tuple.js, which needs shared memory enabled).Background
nonefor internal refs,noexternfor external refs. A(ref null none)value is always null. Validation-level subtyping admits it anywhere an internal ref type is expected.FunctionParserchecks at compile time. A check that passes at load time must pass in every tier, or the tier-up compile fails on a module the embedder already accepted.