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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 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
Bug
Under the wasm-host (wasmi) runtime (
--enable-wasm-runtime/ auto-linked when a program usesWebAssembly.*):WebAssembly.compile(bytes)produces the same wrapper and had the same defect.Root cause
crate::webassembly::make_module_objectreturns a plain heap object stamped with__wasmKind/__wasmModulePtrbut never links its[[Prototype]]toWebAssembly.Module.prototype. The RHSWebAssembly.Moduleis a member expression, sox instanceof WebAssembly.Modulelowers tojs_instanceof_dynamic, which ultimately falls back to the ECMAScriptOrdinaryHasInstanceprototype-chain walk. Because the wrapper's chain never reaches the namespace constructor's.prototype, the walk returnedfalse. This is the exact shape problem the WebAssembly namespace error constructors already work around viawebassembly_error_ctor_instanceof(theirErrorHeader-backed instances also have no prototype chain reaching the ctor's.prototype).Fix
Add a sibling
webassembly_value_ctor_instanceofinglobal_this_webassembly.rsand consult it fromjs_instanceof_dynamicright after the error-ctor hook. It identifies the RHS by its constructor thunkfunc_ptr(GC-move-safe, not forgeable by a same-named user function) and brand-checks the module wrapper by its__wasmKind == "module"tag. It short-circuits only on a positive match and otherwise returnsNone, so non-matching values still fall through to the prototype walk. This keeps the behavior additive and non-regressive.Constructors covered
__wasmKindbrand tag (the only value constructor that produces real instances in this runtime).Memory.prototypeonto the receiver. Verified in the parity fixture (memory instanceof WebAssembly.Memory→true,module instanceof WebAssembly.Memory→false).Tests
value_ctor_instanceof_brands_module_by_kindinglobal_this_webassembly.rs(positive match, cross-brand miss →None, foreign object →None, non-ctor RHS →None).instanceofassertions added to thetest-parity/node-suite/globals/webassembly-module-metadata.tsfixture (the wasm-hostnew WebAssembly.Module(...)path). Output verified byte-identical to Node.Verification run
Before:
module instanceof Module: false. After:module instanceof Module: true/compiled instanceof Module: true. All 8global_this_webassemblyunit tests pass; the default no-engine graceful-fail path (tests/test_webassembly_graceful_fail.sh) still passes.Summary by CodeRabbit
Bug Fixes
instanceofchecks forWebAssembly.Moduleunder the Wasmi runtime.WebAssembly.Memorychecks.Documentation
instanceoffix.