fix(runtime): make WebAssembly.Module instanceof recognize constructed modules - #7124
fix(runtime): make WebAssembly.Module instanceof recognize constructed modules#7124jdalton wants to merge 1 commit into
Conversation
f568bc4 to
ee99532
Compare
📝 WalkthroughWalkthrough
ChangesWebAssembly Module
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Sequence Diagram(s)sequenceDiagram
participant ModuleCreation
participant WebAssemblyObject
participant ModulePointerRegistry
participant Instanceof
ModuleCreation->>WebAssemblyObject: create WebAssembly module wrapper
WebAssemblyObject->>ModulePointerRegistry: register host module pointer
Instanceof->>WebAssemblyObject: check WebAssembly.Module instanceof
WebAssemblyObject->>ModulePointerRegistry: verify module pointer
ModulePointerRegistry-->>WebAssemblyObject: return registration result
WebAssemblyObject-->>Instanceof: return true or use prototype processing
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/perry-runtime/src/object/global_this_webassembly.rs`:
- Around line 254-258: Update value_wasm_kind_matches to root the NaN-boxed
value in a RuntimeHandleScope before calling named_key; allocate the key while
the value is rooted, then reload the ObjectHeader pointer from the rewritten
handle and use that pointer for js_object_get_field_by_name_f64. Preserve the
existing non-object early return and wasm-kind comparison behavior.
- Around line 251-267: Replace value_wasm_kind_matches with a non-forgeable
internal runtime-metadata check instead of reading the public __wasmKind
property, while preserving rejection of non-module objects. In the tests around
the WebAssembly brand checks, use an authentic wrapper from make_module_object
for the positive case and assert that an ordinary object manually stamped with
__wasmKind is rejected; update both affected sites in
crates/perry-runtime/src/object/global_this_webassembly.rs at lines 251-267 and
1297-1337.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 47b93074-7c4c-4559-9326-944a1a6b7b0f
📒 Files selected for processing (5)
changelog.d/7124-wasm-module-instanceof.mdcrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/global_this_webassembly.rscrates/perry-runtime/src/object/instanceof.rstest-parity/node-suite/globals/webassembly-module-metadata.ts
| fn value_wasm_kind_matches(value: f64, expected: &[u8]) -> bool { | ||
| let Some(obj) = value_object_ptr(value) else { | ||
| return false; | ||
| }; | ||
| let kind = js_object_get_field_by_name_f64(obj, named_key(b"__wasmKind")); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Root value before named_key can allocate.
Line 255 converts value to a raw ObjectHeader pointer. Line 258 then calls named_key, which allocates a string before the runtime uses that pointer.
A moving GC can invalidate obj. Hold value in a RuntimeHandleScope, allocate the key, and reload the object pointer from the rewritten handle before the property lookup.
Based on learnings, raw Rust pointer locals are not GC roots, so NaN-boxed values must be rooted and reloaded across allocating operations.
🤖 Prompt for 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.
In `@crates/perry-runtime/src/object/global_this_webassembly.rs` around lines 254
- 258, Update value_wasm_kind_matches to root the NaN-boxed value in a
RuntimeHandleScope before calling named_key; allocate the key while the value is
rooted, then reload the ObjectHeader pointer from the rewritten handle and use
that pointer for js_object_get_field_by_name_f64. Preserve the existing
non-object early return and wasm-kind comparison behavior.
Source: Learnings
|
The brand check keyed only on the enumerable, user-writable ({ __wasmKind: "module" }) instanceof WebAssembly.Module // was true; Node: false
({ __wasmKind: "module", __wasmModulePtr: 1 }) instanceof WebAssembly.Module // was true; Node: false(The func_ptr RHS identity is genuinely unforgeable, but the instance brand was not — a string tag is not a brand. The original positive unit test even hand-built such a plain object and asserted Fix: record each real module's host pointer at construction and require Rewrote the unit test to build a genuine (registered) wrapper and added forgery-rejection cases (tag-only, and both-fields-but-unregistered); added a |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/perry-runtime/src/object/global_this_webassembly.rs (1)
298-338: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
WebAssembly.Modulebrand is bound to a copyable field value, not to object identity. The check (value_wasm_kind_matches) and the creation site (make_module_object) share one root cause: the brand pointer is stored as a plain enumerable/writable property, so it can be read off a genuine module and copied into a forged object to passinstanceof.
crates/perry-runtime/src/object/global_this_webassembly.rs#L298-L338: changevalue_wasm_kind_matchesto validate thatobjitself (itsObjectHeaderpointer) is a registered wrapper, not that its__wasmModulePtrfield's numeric value happens to match a live pointer.crates/perry-runtime/src/webassembly.rs#L302-L313: when registering inmake_module_object, key the registry by the wrapper's own object pointer (or store the host pointer in a non-user-accessible side table keyed by the wrapper) instead of relying solely on the copyable__wasmModulePtrfield value.🤖 Prompt for 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. In `@crates/perry-runtime/src/object/global_this_webassembly.rs` around lines 298 - 338, The WebAssembly.Module brand currently trusts the copyable __wasmModulePtr field instead of object identity. In crates/perry-runtime/src/object/global_this_webassembly.rs:298-338, update value_wasm_kind_matches to validate obj itself against the registered wrapper registry; in crates/perry-runtime/src/webassembly.rs:302-313, register each wrapper’s own object pointer (or use a side table keyed by it) during make_module_object, while preserving the existing module-kind validation.crates/perry-runtime/src/webassembly.rs (1)
316-331: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject unregistered
__wasmModulePtrbefore calling the wasm host.
extract_module_handlepasses the raw pointer intoperry_wasm_host_module_*functions, which dereference it as aWasmModuleHandlein thewasm-hostbuild. A forged object can set__wasmModulePtrto any number, so these static Module metadata methods can dereference an attacker-chosen address. Reject values returned fromextract_module_handlethat are not registered withis_registered_module_ptr(...).🤖 Prompt for 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. In `@crates/perry-runtime/src/webassembly.rs` around lines 316 - 331, Update extract_module_handle to validate the decoded __wasmModulePtr with is_registered_module_ptr(...) before returning it. Return None for null, non-finite, non-positive, or unregistered pointers, ensuring perry_wasm_host_module_* callers only receive registered WasmModuleHandle addresses.
🤖 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.
Outside diff comments:
In `@crates/perry-runtime/src/object/global_this_webassembly.rs`:
- Around line 298-338: The WebAssembly.Module brand currently trusts the
copyable __wasmModulePtr field instead of object identity. In
crates/perry-runtime/src/object/global_this_webassembly.rs:298-338, update
value_wasm_kind_matches to validate obj itself against the registered wrapper
registry; in crates/perry-runtime/src/webassembly.rs:302-313, register each
wrapper’s own object pointer (or use a side table keyed by it) during
make_module_object, while preserving the existing module-kind validation.
In `@crates/perry-runtime/src/webassembly.rs`:
- Around line 316-331: Update extract_module_handle to validate the decoded
__wasmModulePtr with is_registered_module_ptr(...) before returning it. Return
None for null, non-finite, non-positive, or unregistered pointers, ensuring
perry_wasm_host_module_* callers only receive registered WasmModuleHandle
addresses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e068f724-dd20-46f6-8475-5cdff55d5333
📒 Files selected for processing (5)
changelog.d/7124-wasm-module-instanceof.mdcrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/global_this_webassembly.rscrates/perry-runtime/src/webassembly.rstest-parity/node-suite/globals/webassembly-module-metadata.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/perry-runtime/src/object/global_this.rs
…d modules
Under the wasm-host (wasmi) runtime, `new WebAssembly.Module(bytes)` (and
`WebAssembly.compile(bytes)`) returned a plain heap object whose `[[Prototype]]`
does not reach `WebAssembly.Module.prototype`, so `mod instanceof
WebAssembly.Module` walked the prototype chain and answered `false`.
This is the same shape problem the namespace error constructors already work
around (`webassembly_error_ctor_instanceof`): their instances have no prototype
chain reaching the namespace ctor's `.prototype`. Add a sibling
`webassembly_value_ctor_instanceof`, consulted from `js_instanceof_dynamic`
right after the error-ctor hook. It identifies the RHS by its constructor thunk
`func_ptr` (GC-move-safe, not forgeable by a same-named user function) and
short-circuits only on a positive match, so non-matching values still fall
through to the ordinary prototype walk (how `WebAssembly.Memory` instances
resolve, and how a foreign object answers `false`).
The instance brand is an UNFORGEABLE live host-module pointer, not the
enumerable, user-writable `__wasmKind` string. A string tag alone is forgeable:
`{ __wasmKind: "module" }` (or `{ __wasmKind: "module", __wasmModulePtr: 1 }`)
would otherwise answer `true` while Node answers `false`. Each real module's
host pointer is recorded at construction (`make_module_object`) in a side
registry, and the brand check requires the candidate's `__wasmModulePtr` to
name a registered live module -- mirroring the `is_registered_buffer`/`_map`/
`_set` registries the other builtin `instanceof` probes use. A user object
carries a pointer this runtime never handed out, so it is rejected. The
membership test compares by value and never dereferences the candidate pointer,
so a forged number is safe; the registry lives in the always-compiled namespace
module so the check builds with the engine off (no engine -> nothing registered
-> never matches).
Covered constructors: Module (fixed via the registry brand). Memory already
resolves through the prototype walk; Instance/Table/Global throw on construction
in this baseline so no instances exist to brand.
Tests: `value_ctor_instanceof_brands_module_by_registered_ptr` unit test
(genuine registered wrapper positive, plus forgery-rejection cases: tag-only,
and both-fields-but-unregistered), and instanceof assertions in the
`webassembly-module-metadata` parity fixture including a forged-object
negative (verified byte-identical to Node).
a701cea to
4cd1848
Compare
1 participant
A WebAssembly module built by Perry does not recognize itself as a
WebAssembly.Module. Any code that type-checks a value before using it, which is the ordinary way to write a guard or an overloaded function, takes the wrong branch:WebAssembly.compile(bytes)produces the same wrapper object and had the same defect. This affects programs running under the wasm-host (wasmi) runtime, which is selected by--enable-wasm-runtimeand linked automatically when a program uses anything underWebAssembly.*.After this change a genuine module answers
true, aWebAssembly.Memoryinstance still answers correctly for its own constructor, and an object that merely imitates a module answersfalse, matching Node in every case.Why the check returns false
instanceofin JavaScript is defined byOrdinaryHasInstance, which walks the value's prototype chain looking for the constructor's.prototypeobject. If the walk reaches the end without finding it, the answer isfalse.The module wrapper never joins that chain.
crate::webassembly::make_module_objectreturns a plain heap object stamped with two properties,__wasmKindand__wasmModulePtr, but it never links the object's[[Prototype]]toWebAssembly.Module.prototype.The right-hand side of the comparison,
WebAssembly.Module, is a member expression rather than a plain identifier, so the comparison lowers tojs_instanceof_dynamic, which ultimately falls back to that sameOrdinaryHasInstancewalk. The walk cannot reach the namespace constructor's.prototype, so it returnsfalse.This is the same shape problem the WebAssembly namespace's error constructors already have. Their
ErrorHeader-backed instances also lack a prototype chain reaching their constructor's.prototype, and they work around it withwebassembly_error_ctor_instanceof.The fix, and why the obvious version of it is wrong
A sibling function,
webassembly_value_ctor_instanceof, is added inglobal_this_webassembly.rsand consulted fromjs_instanceof_dynamicdirectly after the existing error-constructor hook.It first has to identify what is on the right-hand side. It does that by the constructor thunk's
func_ptr, which is stable across garbage-collector moves and cannot be forged by a user-defined function that merely shares the nameModule.It then has to decide whether the left-hand value is a genuine module, and this is where the obvious approach fails. Both
__wasmKindand__wasmModulePtrare ordinary enumerable, user-writable properties, so checking the string tag alone is not a brand check at all. A plain object literal such as{ __wasmKind: "module", __wasmModulePtr: 1 }would pass it, and Node answersfalsefor that object.The unforgeable signal is that
__wasmModulePtrnames a host allocation this runtime actually produced. Every module pointer is recorded in a side registry at construction time, insidemake_module_object, and the brand check requires membership in that registry. A copied field carries a pointer the runtime never handed out, so it is rejected. This mirrors theis_registered_buffer,_map, and_setside registries the other builtininstanceofprobes already rely on, and it fails closed: a null pointer or a poisoned lock yieldsfalse.The hook short-circuits only on a positive match and returns
Noneotherwise, so any non-matching value still falls through to the ordinary prototype walk. That keeps the change additive and non-regressive.Which constructors this covers
Moduleis the one fixed here. It is the only value constructor in this runtime that produces real instances, so it is the only one that needs a brand.Memoryalready resolves correctly through the ordinary prototype walk, because the dynamic construct path pre-linksMemory.prototypeonto the receiver. This is verified in the parity fixture:memory instanceof WebAssembly.Memoryistrueandmodule instanceof WebAssembly.Memoryisfalse.Instance,Table, andGlobalthrow on construction in this baseline, so no instances of them exist to brand.Test runs
A new Rust unit test,
value_ctor_instanceof_brands_module_by_registered_ptringlobal_this_webassembly.rs, covers a genuine module matching, a cross-brand miss returningNone, a forged object carrying only the__wasmKindtag returningNone, a forged object carrying both fields but an unregistered pointer returningNone, a foreign object returningNone, and a non-constructor right-hand side returningNone.instanceofassertions are added to thetest-parity/node-suite/globals/webassembly-module-metadata.tsfixture, which exercises the wasm-hostnew WebAssembly.Module(...)path. Its output was verified byte-identical to Node's.Before the change the fixture printed
module instanceof Module: false. After it,module instanceof Module: trueandcompiled instanceof Module: true. All 8global_this_webassemblyunit tests pass, and the default no-engine graceful-failure path intests/test_webassembly_graceful_fail.shstill passes.