Skip to content

Commit

Permalink
wasi: relax WebAssembly.Instance type check
Browse files Browse the repository at this point in the history
Instances coming from different VM contexts don't pass `instanceof`
type checks because each context has its own copy of the built-in
globals.

After review of the relevant code it seems like it should be safe to
relax the type check and that is what this commit does: `wasi.start()`
now accepts any input that walks and quacks like a WebAssembly.Instance
or WebAssembly.Memory instance.

Fixes: #33415

PR-URL: #33431
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
  • Loading branch information
bnoordhuis authored and codebytere committed Jul 8, 2020
1 parent 78734c2 commit 383c5b3
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 9 deletions.
21 changes: 14 additions & 7 deletions lib/wasi.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
'use strict';
/* global WebAssembly */
const {
ArrayPrototypeMap,
ArrayPrototypePush,
Expand All @@ -12,6 +11,7 @@ const {
ERR_WASI_ALREADY_STARTED
} = require('internal/errors').codes;
const { emitExperimentalWarning } = require('internal/util');
const { isArrayBuffer } = require('internal/util/types');
const {
validateArray,
validateBoolean,
Expand Down Expand Up @@ -70,10 +70,7 @@ class WASI {
}

start(instance) {
if (!(instance instanceof WebAssembly.Instance)) {
throw new ERR_INVALID_ARG_TYPE(
'instance', 'WebAssembly.Instance', instance);
}
validateObject(instance, 'instance');

const exports = instance.exports;

Expand All @@ -91,9 +88,19 @@ class WASI {
'instance.exports._initialize', 'undefined', _initialize);
}

if (!(memory instanceof WebAssembly.Memory)) {
// WASI::_SetMemory() in src/node_wasi.cc only expects that |memory| is
// an object. It will try to look up the .buffer property when needed
// and fail with UVWASI_EINVAL when the property is missing or is not
// an ArrayBuffer. Long story short, we don't need much validation here
// but we type-check anyway because it helps catch bugs in the user's
// code early.
validateObject(memory, 'instance.exports.memory');

if (!isArrayBuffer(memory.buffer)) {
throw new ERR_INVALID_ARG_TYPE(
'instance.exports.memory', 'WebAssembly.Memory', memory);
'instance.exports.memory.buffer',
['WebAssembly.Memory'],
memory.buffer);
}

if (this[kStarted]) {
Expand Down
76 changes: 74 additions & 2 deletions test/wasi/test-wasi-start-validation.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

const common = require('../common');
const assert = require('assert');
const vm = require('vm');
const { WASI } = require('wasi');

const fixtures = require('../common/fixtures');
Expand All @@ -15,7 +16,10 @@ const bufferSource = fixtures.readSync('simple.wasm');

assert.throws(
() => { wasi.start(); },
{ code: 'ERR_INVALID_ARG_TYPE', message: /\bWebAssembly\.Instance\b/ }
{
code: 'ERR_INVALID_ARG_TYPE',
message: /"instance" argument must be of type object/
}
);
}

Expand Down Expand Up @@ -87,11 +91,79 @@ const bufferSource = fixtures.readSync('simple.wasm');
() => { wasi.start(instance); },
{
code: 'ERR_INVALID_ARG_TYPE',
message: /"instance\.exports\.memory" property .+ WebAssembly\.Memory/
message: /"instance\.exports\.memory" property must be of type object/
}
);
}

{
// Verify that a non-ArrayBuffer memory.buffer is rejected.
const wasi = new WASI({});
const wasm = await WebAssembly.compile(bufferSource);
const instance = await WebAssembly.instantiate(wasm);

Object.defineProperty(instance, 'exports', {
get() {
return {
_start() {},
memory: {},
};
}
});
// The error message is a little white lie because any object
// with a .buffer property of type ArrayBuffer is accepted,
// but 99% of the time a WebAssembly.Memory object is used.
assert.throws(
() => { wasi.start(instance); },
{
code: 'ERR_INVALID_ARG_TYPE',
message: /"instance\.exports\.memory\.buffer" property must be an WebAssembly\.Memory/
}
);
}

{
// Verify that an argument that duck-types as a WebAssembly.Instance
// is accepted.
const wasi = new WASI({});
const wasm = await WebAssembly.compile(bufferSource);
const instance = await WebAssembly.instantiate(wasm);

Object.defineProperty(instance, 'exports', {
get() {
return {
_start() {},
memory: { buffer: new ArrayBuffer(0) },
};
}
});
wasi.start(instance);
}

{
// Verify that a WebAssembly.Instance from another VM context is accepted.
const wasi = new WASI({});
const instance = await vm.runInNewContext(`
(async () => {
const wasm = await WebAssembly.compile(bufferSource);
const instance = await WebAssembly.instantiate(wasm);
Object.defineProperty(instance, 'exports', {
get() {
return {
_start() {},
memory: new WebAssembly.Memory({ initial: 1 })
};
}
});
return instance;
})()
`, { bufferSource });

wasi.start(instance);
}

{
// Verify that start() can only be called once.
const wasi = new WASI({});
Expand Down

0 comments on commit 383c5b3

Please sign in to comment.