From 6b20e8442fa10b5967e36f21c964bf435dd594dc Mon Sep 17 00:00:00 2001
From: Ben Noordhuis <info@bnoordhuis.nl>
Date: Sat, 16 May 2020 13:19:54 +0200
Subject: [PATCH] wasi: relax WebAssembly.Instance type check

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: https://github.com/nodejs/node/issues/33415

PR-URL: https://github.com/nodejs/node/pull/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>
---
 lib/wasi.js                             | 21 ++++---
 test/wasi/test-wasi-start-validation.js | 76 ++++++++++++++++++++++++-
 2 files changed, 88 insertions(+), 9 deletions(-)

diff --git a/lib/wasi.js b/lib/wasi.js
index 2de467ff1ac066..d3125ca7abd9d2 100644
--- a/lib/wasi.js
+++ b/lib/wasi.js
@@ -1,5 +1,4 @@
 'use strict';
-/* global WebAssembly */
 const {
   ArrayPrototypeMap,
   ArrayPrototypePush,
@@ -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,
@@ -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;
 
@@ -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');
+
+    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]) {
diff --git a/test/wasi/test-wasi-start-validation.js b/test/wasi/test-wasi-start-validation.js
index 617738442ee64a..3134514d704595 100644
--- a/test/wasi/test-wasi-start-validation.js
+++ b/test/wasi/test-wasi-start-validation.js
@@ -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');
@@ -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/
+      }
     );
   }
 
@@ -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({});