[wasm] Improve copying behavior for SyncCompile and SyncValidate

This fixes a long-standing TODO to only make a copy of a module's
wire bytes if the input is a SharedArrayBuffer and also fixes the
concurrent-modification bug for synchronous validation.

R=clemensh@chromium.org
BUG=chromium:794091

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: I8d2f20a9aeedbc306434853f8f6cfc070a24cf97
Reviewed-on: https://chromium-review.googlesource.com/856559
Commit-Queue: Ben Titzer <titzer@chromium.org>
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50472}

src/api.cc

@@ -7666,7 +7666,8 @@ void WasmModuleObjectBuilderStreaming::Finish() {
   // will be resolved when we move to true streaming compilation.
   i::wasm::AsyncCompile(reinterpret_cast<i::Isolate*>(isolate_),
                         Utils::OpenHandle(*promise_.Get(isolate_)),
-                        {wire_bytes.get(), wire_bytes.get() + total_size_});
+                        {wire_bytes.get(), wire_bytes.get() + total_size_},
+                        false);
 }
 
 void WasmModuleObjectBuilderStreaming::Abort(Local<Value> exception) {

src/wasm/module-compiler.cc

@@ -562,13 +562,8 @@ MaybeHandle<WasmModuleObject> SyncCompileTranslatedAsmJs(
 MaybeHandle<WasmModuleObject> SyncCompile(Isolate* isolate,
                                           ErrorThrower* thrower,
                                           const ModuleWireBytes& bytes) {
-  // TODO(titzer): only make a copy of the bytes if SharedArrayBuffer
-  std::unique_ptr<byte[]> copy(new byte[bytes.length()]);
-  memcpy(copy.get(), bytes.start(), bytes.length());
-  ModuleWireBytes bytes_copy(copy.get(), copy.get() + bytes.length());
-
-  ModuleResult result = SyncDecodeWasmModule(
-      isolate, bytes_copy.start(), bytes_copy.end(), false, kWasmOrigin);
+  ModuleResult result = SyncDecodeWasmModule(isolate, bytes.start(),
+                                             bytes.end(), false, kWasmOrigin);
   if (result.failed()) {
     thrower->CompileFailed("Wasm decoding failed", result);
     return {};
@@ -577,7 +572,7 @@ MaybeHandle<WasmModuleObject> SyncCompile(Isolate* isolate,
   // Transfer ownership of the WasmModule to the {WasmModuleWrapper} generated
   // in {CompileToModuleObject}.
   return ModuleCompiler::CompileToModuleObject(
-      isolate, thrower, std::move(result.val), bytes_copy, Handle<Script>(),
+      isolate, thrower, std::move(result.val), bytes, Handle<Script>(),
       Vector<const byte>());
 }
 
@@ -634,12 +629,22 @@ void AsyncInstantiate(Isolate* isolate, Handle<JSPromise> promise,
 }
 
 void AsyncCompile(Isolate* isolate, Handle<JSPromise> promise,
-                  const ModuleWireBytes& bytes) {
+                  const ModuleWireBytes& bytes, bool is_shared) {
   if (!FLAG_wasm_async_compilation) {
+    // Asynchronous compilation disabled; fall back on synchronous compilation.
     ErrorThrower thrower(isolate, "WasmCompile");
-    // Compile the module.
-    MaybeHandle<WasmModuleObject> module_object =
-        SyncCompile(isolate, &thrower, bytes);
+    MaybeHandle<WasmModuleObject> module_object;
+    if (is_shared) {
+      // Make a copy of the wire bytes to avoid concurrent modification.
+      std::unique_ptr<uint8_t[]> copy(new uint8_t[bytes.length()]);
+      memcpy(copy.get(), bytes.start(), bytes.length());
+      i::wasm::ModuleWireBytes bytes_copy(copy.get(),
+                                          copy.get() + bytes.length());
+      module_object = SyncCompile(isolate, &thrower, bytes_copy);
+    } else {
+      // The wire bytes are not shared, OK to use them directly.
+      module_object = SyncCompile(isolate, &thrower, bytes);
+    }
     if (thrower.error()) {
       RejectPromise(isolate, handle(isolate->context()), thrower, promise);
       return;

src/wasm/module-compiler.h

@@ -43,7 +43,8 @@ V8_EXPORT_PRIVATE MaybeHandle<WasmInstanceObject> SyncCompileAndInstantiate(
     MaybeHandle<JSReceiver> imports, MaybeHandle<JSArrayBuffer> memory);
 
 V8_EXPORT_PRIVATE void AsyncCompile(Isolate* isolate, Handle<JSPromise> promise,
-                                    const ModuleWireBytes& bytes);
+                                    const ModuleWireBytes& bytes,
+                                    bool is_shared);
 
 V8_EXPORT_PRIVATE void AsyncInstantiate(Isolate* isolate,
                                         Handle<JSPromise> promise,

src/wasm/wasm-js.cc

@@ -63,7 +63,8 @@ i::MaybeHandle<i::WasmModuleObject> GetFirstArgumentAsModule(
 }
 
 i::wasm::ModuleWireBytes GetFirstArgumentAsBytes(
-    const v8::FunctionCallbackInfo<v8::Value>& args, ErrorThrower* thrower) {
+    const v8::FunctionCallbackInfo<v8::Value>& args, ErrorThrower* thrower,
+    bool* is_shared) {
   const uint8_t* start = nullptr;
   size_t length = 0;
   v8::Local<v8::Value> source = args[0];
@@ -74,6 +75,7 @@ i::wasm::ModuleWireBytes GetFirstArgumentAsBytes(
 
     start = reinterpret_cast<const uint8_t*>(contents.Data());
     length = contents.ByteLength();
+    *is_shared = buffer->IsSharedArrayBuffer();
   } else if (source->IsTypedArray()) {
     // A TypedArray was passed.
     Local<TypedArray> array = Local<TypedArray>::Cast(source);
@@ -84,6 +86,7 @@ i::wasm::ModuleWireBytes GetFirstArgumentAsBytes(
     start =
         reinterpret_cast<const uint8_t*>(contents.Data()) + array->ByteOffset();
     length = array->ByteLength();
+    *is_shared = buffer->IsSharedArrayBuffer();
   } else {
     thrower->TypeError("Argument 0 must be a buffer source");
   }
@@ -154,7 +157,8 @@ void WebAssemblyCompile(const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::ReturnValue<v8::Value> return_value = args.GetReturnValue();
   return_value.Set(resolver->GetPromise());
 
-  auto bytes = GetFirstArgumentAsBytes(args, &thrower);
+  bool is_shared = false;
+  auto bytes = GetFirstArgumentAsBytes(args, &thrower, &is_shared);
   if (thrower.error()) {
     auto maybe = resolver->Reject(context, Utils::ToLocal(thrower.Reify()));
     CHECK_IMPLIES(!maybe.FromMaybe(false),
@@ -162,7 +166,8 @@ void WebAssemblyCompile(const v8::FunctionCallbackInfo<v8::Value>& args) {
     return;
   }
   i::Handle<i::JSPromise> promise = Utils::OpenHandle(*resolver->GetPromise());
-  i::wasm::AsyncCompile(i_isolate, promise, bytes);
+  // Asynchronous compilation handles copying wire bytes if necessary.
+  i::wasm::AsyncCompile(i_isolate, promise, bytes, is_shared);
 }
 
 // WebAssembly.validate(bytes) -> bool
@@ -172,16 +177,31 @@ void WebAssemblyValidate(const v8::FunctionCallbackInfo<v8::Value>& args) {
   HandleScope scope(isolate);
   i::wasm::ScheduledErrorThrower thrower(i_isolate, "WebAssembly.validate()");
 
-  auto bytes = GetFirstArgumentAsBytes(args, &thrower);
+  bool is_shared = false;
+  auto bytes = GetFirstArgumentAsBytes(args, &thrower, &is_shared);
 
   v8::ReturnValue<v8::Value> return_value = args.GetReturnValue();
-  if (!thrower.error() &&
-      i::wasm::SyncValidate(reinterpret_cast<i::Isolate*>(isolate), bytes)) {
-    return_value.Set(v8::True(isolate));
-  } else {
+
+  if (thrower.error()) {
     if (thrower.wasm_error()) thrower.Reset();  // Clear error.
     return_value.Set(v8::False(isolate));
+    return;
+  }
+
+  bool validated = false;
+  if (is_shared) {
+    // Make a copy of the wire bytes to avoid concurrent modification.
+    std::unique_ptr<uint8_t[]> copy(new uint8_t[bytes.length()]);
+    memcpy(copy.get(), bytes.start(), bytes.length());
+    i::wasm::ModuleWireBytes bytes_copy(copy.get(),
+                                        copy.get() + bytes.length());
+    validated = i::wasm::SyncValidate(i_isolate, bytes_copy);
+  } else {
+    // The wire bytes are not shared, OK to use them directly.
+    validated = i::wasm::SyncValidate(i_isolate, bytes);
   }
+
+  return_value.Set(Boolean::New(isolate, validated));
 }
 
 // new WebAssembly.Module(bytes) -> WebAssembly.Module
@@ -202,13 +222,25 @@ void WebAssemblyModule(const v8::FunctionCallbackInfo<v8::Value>& args) {
     return;
   }
 
-  auto bytes = GetFirstArgumentAsBytes(args, &thrower);
+  bool is_shared = false;
+  auto bytes = GetFirstArgumentAsBytes(args, &thrower, &is_shared);
 
   if (thrower.error()) {
     return;
   }
-  i::MaybeHandle<i::Object> module_obj =
-      i::wasm::SyncCompile(i_isolate, &thrower, bytes);
+  i::MaybeHandle<i::Object> module_obj;
+  if (is_shared) {
+    // Make a copy of the wire bytes to avoid concurrent modification.
+    std::unique_ptr<uint8_t[]> copy(new uint8_t[bytes.length()]);
+    memcpy(copy.get(), bytes.start(), bytes.length());
+    i::wasm::ModuleWireBytes bytes_copy(copy.get(),
+                                        copy.get() + bytes.length());
+    module_obj = i::wasm::SyncCompile(i_isolate, &thrower, bytes_copy);
+  } else {
+    // The wire bytes are not shared, OK to use them directly.
+    module_obj = i::wasm::SyncCompile(i_isolate, &thrower, bytes);
+  }
+
   if (module_obj.is_null()) return;
 
   v8::ReturnValue<v8::Value> return_value = args.GetReturnValue();

test/fuzzer/wasm-async.cc

@@ -94,7 +94,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
   Local<Promise> promise = resolver->GetPromise();
 
   AsyncCompile(i_isolate, Utils::OpenHandle(*promise),
-               ModuleWireBytes(data, data + size));
+               ModuleWireBytes(data, data + size), false);
 
   ASSIGN(Function, instantiate_impl,
          Function::New(support->GetContext(), &InstantiateCallback,