From 6ca6db105d8db40af538e275f39612b569b9c141 Mon Sep 17 00:00:00 2001
From: cjihrig <cjihrig@gmail.com>
Date: Sun, 26 Apr 2020 01:20:10 -0400
Subject: [PATCH] wasi: update start() behavior to match spec

_start() and _initialize() shouldn't be called from the same
function, as they have different behavior. Furthermore, Node
should throw if both are provided. This commit updates the
implementation, docs, and tests accordingly.

PR-URL: https://github.com/nodejs/node/pull/33073
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
---
 doc/api/wasi.md                         |   9 +-
 lib/wasi.js                             |  17 ++-
 test/wasi/test-wasi-start-validation.js | 164 ++++++++++++++----------
 3 files changed, 112 insertions(+), 78 deletions(-)

diff --git a/doc/api/wasi.md b/doc/api/wasi.md
index 09ea12e811f..ebaf15525d0 100644
--- a/doc/api/wasi.md
+++ b/doc/api/wasi.md
@@ -76,14 +76,15 @@ added:
 
 * `instance` {WebAssembly.Instance}
 
-Attempt to begin execution of `instance` by invoking its `_start()` export.
-If `instance` does not contain a `_start()` export, then `start()` attempts to
-invoke the `_initialize()` export. If neither of those exports is present on
-`instance`, then `start()` does nothing.
+Attempt to begin execution of `instance` as a WASI command by invoking its
+`_start()` export. If `instance` does not contain a `_start()` export, or if
+`instance` contains an `_initialize()` export, then an exception is thrown.
 
 `start()` requires that `instance` exports a [`WebAssembly.Memory`][] named
 `memory`. If `instance` does not have a `memory` export an exception is thrown.
 
+If `start()` is called more than once, an exception is thrown.
+
 ### `wasi.wasiImport`
 <!-- YAML
 added:
diff --git a/lib/wasi.js b/lib/wasi.js
index 4283216afc7..2de467ff1ac 100644
--- a/lib/wasi.js
+++ b/lib/wasi.js
@@ -80,7 +80,17 @@ class WASI {
 
     validateObject(exports, 'instance.exports');
 
-    const { memory } = exports;
+    const { _initialize, _start, memory } = exports;
+
+    if (typeof _start !== 'function') {
+      throw new ERR_INVALID_ARG_TYPE(
+        'instance.exports._start', 'function', _start);
+    }
+
+    if (_initialize !== undefined) {
+      throw new ERR_INVALID_ARG_TYPE(
+        'instance.exports._initialize', 'undefined', _initialize);
+    }
 
     if (!(memory instanceof WebAssembly.Memory)) {
       throw new ERR_INVALID_ARG_TYPE(
@@ -95,10 +105,7 @@ class WASI {
     this[kSetMemory](memory);
 
     try {
-      if (exports._start)
-        exports._start();
-      else if (exports._initialize)
-        exports._initialize();
+      exports._start();
     } catch (err) {
       if (err !== kExitCode) {
         throw err;
diff --git a/test/wasi/test-wasi-start-validation.js b/test/wasi/test-wasi-start-validation.js
index 94d31f2e125..617738442ee 100644
--- a/test/wasi/test-wasi-start-validation.js
+++ b/test/wasi/test-wasi-start-validation.js
@@ -8,85 +8,111 @@ const { WASI } = require('wasi');
 const fixtures = require('../common/fixtures');
 const bufferSource = fixtures.readSync('simple.wasm');
 
-{
-  const wasi = new WASI();
-  assert.throws(
-    () => {
-      wasi.start();
-    },
-    { code: 'ERR_INVALID_ARG_TYPE', message: /\bWebAssembly\.Instance\b/ }
-  );
-}
-
 (async () => {
-  const wasi = new WASI({});
-  const wasm = await WebAssembly.compile(bufferSource);
-  const instance = await WebAssembly.instantiate(wasm);
+  {
+    // Verify that a WebAssembly.Instance is passed in.
+    const wasi = new WASI();
 
-  assert.throws(
-    () => { wasi.start(instance); },
-    { code: 'ERR_INVALID_ARG_TYPE', message: /\bWebAssembly\.Memory\b/ }
-  );
-})();
+    assert.throws(
+      () => { wasi.start(); },
+      { code: 'ERR_INVALID_ARG_TYPE', message: /\bWebAssembly\.Instance\b/ }
+    );
+  }
 
-(async () => {
-  const wasi = new WASI();
-  const wasm = await WebAssembly.compile(bufferSource);
-  const instance = await WebAssembly.instantiate(wasm);
-  const values = [undefined, null, 'foo', 42, true, false, () => {}];
-  let cnt = 0;
+  {
+    // Verify that the passed instance has an exports objects.
+    const wasi = new WASI({});
+    const wasm = await WebAssembly.compile(bufferSource);
+    const instance = await WebAssembly.instantiate(wasm);
 
-  // Mock instance.exports to trigger start() validation.
-  Object.defineProperty(instance, 'exports', {
-    get() { return values[cnt++]; }
-  });
-
-  values.forEach((val) => {
+    Object.defineProperty(instance, 'exports', { get() { return null; } });
     assert.throws(
       () => { wasi.start(instance); },
-      { code: 'ERR_INVALID_ARG_TYPE', message: /\binstance\.exports\b/ }
+      {
+        code: 'ERR_INVALID_ARG_TYPE',
+        message: /"instance\.exports" property must be of type object/
+      }
     );
-  });
-})();
+  }
 
-(async () => {
-  const wasi = new WASI();
-  const wasm = await WebAssembly.compile(bufferSource);
-  const instance = await WebAssembly.instantiate(wasm);
+  {
+    // Verify that a _start() export was passed.
+    const wasi = new WASI({});
+    const wasm = await WebAssembly.compile(bufferSource);
+    const instance = await WebAssembly.instantiate(wasm);
 
-  // Mock instance.exports.memory to bypass start() validation.
-  Object.defineProperty(instance, 'exports', {
-    get() {
-      return {
-        memory: new WebAssembly.Memory({ initial: 1 })
-      };
-    }
-  });
+    Object.defineProperty(instance, 'exports', { get() { return {}; } });
+    assert.throws(
+      () => { wasi.start(instance); },
+      {
+        code: 'ERR_INVALID_ARG_TYPE',
+        message: /"instance\.exports\._start" property must be of type function/
+      }
+    );
+  }
 
-  wasi.start(instance);
-  assert.throws(
-    () => { wasi.start(instance); },
-    {
-      code: 'ERR_WASI_ALREADY_STARTED',
-      message: /^WASI instance has already started$/
-    }
-  );
-})();
+  {
+    // Verify that an _initialize export was not passed.
+    const wasi = new WASI({});
+    const wasm = await WebAssembly.compile(bufferSource);
+    const instance = await WebAssembly.instantiate(wasm);
 
-(async () => {
-  const wasi = new WASI();
-  const wasm = await WebAssembly.compile(bufferSource);
-  const instance = await WebAssembly.instantiate(wasm);
+    Object.defineProperty(instance, 'exports', {
+      get() {
+        return {
+          _start() {},
+          _initialize() {}
+        };
+      }
+    });
+    assert.throws(
+      () => { wasi.start(instance); },
+      {
+        code: 'ERR_INVALID_ARG_TYPE',
+        message: /"instance\.exports\._initialize" property must be undefined/
+      }
+    );
+  }
 
-  // Mock instance.exports to bypass start() validation.
-  Object.defineProperty(instance, 'exports', {
-    get() {
-      return {
-        memory: new WebAssembly.Memory({ initial: 1 }),
-        _initialize: common.mustCall()
-      };
-    }
-  });
+  {
+    // Verify that a memory export was passed.
+    const wasi = new WASI({});
+    const wasm = await WebAssembly.compile(bufferSource);
+    const instance = await WebAssembly.instantiate(wasm);
 
-  wasi.start(instance);
-})();
+    Object.defineProperty(instance, 'exports', {
+      get() { return { _start() {} }; }
+    });
+    assert.throws(
+      () => { wasi.start(instance); },
+      {
+        code: 'ERR_INVALID_ARG_TYPE',
+        message: /"instance\.exports\.memory" property .+ WebAssembly\.Memory/
+      }
+    );
+  }
+
+  {
+    // Verify that start() can only be called once.
+    const wasi = new WASI({});
+    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 })
+        };
+      }
+    });
+    wasi.start(instance);
+    assert.throws(
+      () => { wasi.start(instance); },
+      {
+        code: 'ERR_WASI_ALREADY_STARTED',
+        message: /^WASI instance has already started$/
+      }
+    );
+  }
+})().then(common.mustCall());