Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wasi: relax WebAssembly.Instance type check #33431

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -13,6 +12,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 @@ -71,10 +71,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 @@ -92,9 +89,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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking observation: one additional check we could do here is to check that memory.constructor.name === 'Memory'.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t see why anybody would ever subclass WebAssembly.Memory, but then again I don’t really see why we would ever want to forbid that. I think this can stay as-is.


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