[wasm] Do an additional IsWasmModuleObject check during instantiation

gahaas · Commit Bot · commit 441e6d4a3ca4 · 2018-04-27T17:34:05.000Z
When WebAssembly.instantiate or WebAssembly.instantiateStreaming is called in JavaScript, internally we transfrom it into WebAssembly.compile(buffer).then(WebAssembly.instantiate). However, modifying the prototype of WebAssembly.Module can change the result of WebAssembly.compile(buffer). With this CL we make sure that even if the result of WebAssembly.compile is modified, there is still no type confusion. In the long term we have to do a refactoring and remove this internal transformation. R=mstarzinger@chromium.org Bug: chromium:837417 Change-Id: I376068b8b8b01b991ec450162da6a62ae7030c62 Reviewed-on: https://chromium-review.googlesource.com/1032392 Commit-Queue: Andreas Haas <ahaas@chromium.org> Reviewed-by: Michael Starzinger <mstarzinger@chromium.org> Cr-Commit-Position: refs/heads/master@{#52859}
diff --git a/src/wasm/wasm-js.cc b/src/wasm/wasm-js.cc
@@ -330,16 +330,22 @@ MaybeLocal<Value> WebAssemblyInstantiateImpl(Isolate* isolate,
   i::MaybeHandle<i::Object> instance_object;
   {
     ScheduledErrorThrower thrower(i_isolate, "WebAssembly Instantiation");
+
+    // TODO(ahaas): These checks on the module should not be necessary here They
+    // are just a workaround for https://crbug.com/837417.
+    i::Handle<i::Object> module_obj = Utils::OpenHandle(*module);
+    if (!module_obj->IsWasmModuleObject()) {
+      thrower.TypeError("Argument 0 must be a WebAssembly.Module object");
+      return {};
+    }
+
     i::MaybeHandle<i::JSReceiver> maybe_imports =
         GetValueAsImports(ffi, &thrower);
     if (thrower.error()) return {};
 
-    i::Handle<i::WasmModuleObject> module_obj =
-        i::Handle<i::WasmModuleObject>::cast(
-            Utils::OpenHandle(Object::Cast(*module)));
     instance_object = i_isolate->wasm_engine()->SyncInstantiate(
-        i_isolate, &thrower, module_obj, maybe_imports,
-        i::MaybeHandle<i::JSArrayBuffer>());
+        i_isolate, &thrower, i::Handle<i::WasmModuleObject>::cast(module_obj),
+        maybe_imports, i::MaybeHandle<i::JSArrayBuffer>());
   }
 
   DCHECK_EQ(instance_object.is_null(), i_isolate->has_scheduled_exception());
diff --git a/test/mjsunit/regress/wasm/regress-836141.js b/test/mjsunit/regress/wasm/regress-836141.js
@@ -11,7 +11,6 @@ builder.addFunction("test", kSig_i_v).addBody([
   kExprI32Const, 12,         // i32.const 0
 ]);
 
-let bla = 0;
 let module = new WebAssembly.Module(builder.toBuffer());
 module.then = () => {
   // Use setTimeout to get out of the promise chain.
diff --git a/test/mjsunit/regress/wasm/regress-837417.js b/test/mjsunit/regress/wasm/regress-837417.js
@@ -0,0 +1,23 @@
+// Copyright 2018 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+load('test/mjsunit/wasm/wasm-constants.js');
+load('test/mjsunit/wasm/wasm-module-builder.js');
+
+const builder = new WasmModuleBuilder();
+builder.addMemory(16, 32);
+builder.addFunction("test", kSig_i_v).addBody([
+  kExprI32Const, 12,         // i32.const 0
+]);
+
+WebAssembly.Module.prototype.then = resolve => resolve(
+    String.fromCharCode(null, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41));
+
+// WebAssembly.instantiate should not actually throw a TypeError in this case.
+// However, this is a workaround for
+assertPromiseResult(
+    WebAssembly.instantiate(builder.toBuffer()), assertUnreachable,
+    exception => {
+      assertInstanceof(exception, TypeError);
+    });
