Disallow passing a SharedArrayBuffer in the transfer list.

src/value-serializer.cc

 // Copyright 2016 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.
 
 #include "src/value-serializer.h"
 
 #include <type_traits>
 
 #include "src/base/logging.h"
 #include "src/conversions.h"
@@ skipping 424 matching lines @@
     case JS_VALUE_TYPE:
       return WriteJSValue(Handle<JSValue>::cast(receiver));
     case JS_REGEXP_TYPE:
       WriteJSRegExp(JSRegExp::cast(*receiver));
       return Just(true);
     case JS_MAP_TYPE:
       return WriteJSMap(Handle<JSMap>::cast(receiver));
     case JS_SET_TYPE:
       return WriteJSSet(Handle<JSSet>::cast(receiver));
     case JS_ARRAY_BUFFER_TYPE:
-      return WriteJSArrayBuffer(JSArrayBuffer::cast(*receiver));
+      return WriteJSArrayBuffer(Handle<JSArrayBuffer>::cast(receiver));
     case JS_TYPED_ARRAY_TYPE:
     case JS_DATA_VIEW_TYPE:
       return WriteJSArrayBufferView(JSArrayBufferView::cast(*receiver));
     default:
       ThrowDataCloneError(MessageTemplate::kDataCloneError, receiver);
       return Nothing<bool>();
   }
   return Nothing<bool>();
 }
 
@@ skipping 261 matching lines @@
   for (int i = 0; i < length; i++) {
     if (!WriteObject(handle(entries->get(i), isolate_)).FromMaybe(false)) {
       return Nothing<bool>();
     }
   }
   WriteTag(SerializationTag::kEndJSSet);
   WriteVarint<uint32_t>(length);
   return Just(true);
 }
 
-Maybe<bool> ValueSerializer::WriteJSArrayBuffer(JSArrayBuffer* array_buffer) {
+Maybe<bool> ValueSerializer::WriteJSArrayBuffer(
+    Handle<JSArrayBuffer> array_buffer) {
   uint32_t* transfer_entry = array_buffer_transfer_map_.Find(array_buffer);
-  if (transfer_entry) {
-    WriteTag(array_buffer->is_shared()
-                 ? SerializationTag::kSharedArrayBufferTransfer
-                 : SerializationTag::kArrayBufferTransfer);
-    WriteVarint(*transfer_entry);
+  if (array_buffer->is_shared()) {
+    if (!delegate_) {
+      ThrowDataCloneError(MessageTemplate::kDataCloneError, array_buffer);
+      return Nothing<bool>();
+    }
+
+    if (transfer_entry) {
+      // SharedArrayBuffer must not be in the transfer list.
+      ThrowDataCloneError(

[Jakob Kummerow, 2016/12/13 02:34:58]

Would it make more sense to enforce this with a
DCHECK(!array_buffer->is_shared()) in ValueSerializer::TransferArrayBuffer()
(and/or a DCHECK here)? Then you could move the
array_buffer_transfer_map_.Find() call into the branch below where
|transfer_entry| is actually required.

[jbroman, 2016/12/13 20:56:00]

+1

[binji, 2016/12/14 23:58:30]

Done.

+          MessageTemplate::kDataCloneErrorSharedArrayBufferTransferred);
+      return Nothing<bool>();
+    }
+
+    v8::Isolate* v8_isolate = reinterpret_cast<v8::Isolate*>(isolate_);
+    Maybe<uint32_t> index = delegate_->TransferSharedArrayBuffer(
+        v8_isolate, Utils::ToLocal(array_buffer));
+    RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate_, Nothing<bool>());
+    DCHECK(!index.IsNothing());
+
+    WriteTag(SerializationTag::kSharedArrayBufferTransfer);

[jbroman, 2016/12/13 20:56:00]

nit: Should this be renamed to just kSharedArrayBuffer, since it's not a
transfer anymore?

[binji, 2016/12/14 23:58:30]

Done.

+    WriteVarint(index.FromJust());
     return Just(true);
-  }
-
-  if (array_buffer->is_shared()) {
-    ThrowDataCloneError(
-        MessageTemplate::kDataCloneErrorSharedArrayBufferNotTransferred);
-    return Nothing<bool>();
+  } else {

[jbroman, 2016/12/13 20:56:00]

nit: From my perspective, having to match this else with the corresponding if
makes this harder to read. Since the other branch always returns anyhow, could
this be formatted as:

if (array_buffer->is_shared()) {
  // shared array buffers
  return Just(true);
}

uint32_t* transfer_entry = array_buffer_transfer_map_.Find(array_buffer);
if (transfer_entry) {
  // array buffer transfer
  return Just(true);
}

// remaining code

[binji, 2016/12/14 23:58:30]

Done.

+    if (transfer_entry) {
+      WriteTag(SerializationTag::kArrayBufferTransfer);
+      WriteVarint(*transfer_entry);
+      return Just(true);
+    }
   }
   if (array_buffer->was_neutered()) {
     ThrowDataCloneError(MessageTemplate::kDataCloneErrorNeuteredArrayBuffer);
     return Nothing<bool>();
   }
   double byte_length = array_buffer->byte_length()->Number();
   if (byte_length > std::numeric_limits<uint32_t>::max()) {
-    ThrowDataCloneError(MessageTemplate::kDataCloneError, handle(array_buffer));
+    ThrowDataCloneError(MessageTemplate::kDataCloneError, array_buffer);
     return Nothing<bool>();
   }
   WriteTag(SerializationTag::kArrayBuffer);
   WriteVarint<uint32_t>(byte_length);
   WriteRawBytes(array_buffer->backing_store(), byte_length);
   return Just(true);
 }
 
 Maybe<bool> ValueSerializer::WriteJSArrayBufferView(JSArrayBufferView* view) {
   WriteTag(SerializationTag::kArrayBufferView);
@@ skipping 1101 matching lines @@
   if (stack.size() != 1) {
     isolate_->Throw(*isolate_->factory()->NewError(
         MessageTemplate::kDataCloneDeserializationError));
     return MaybeHandle<Object>();
   }
   return scope.CloseAndEscape(stack[0]);
 }
 
 }  // namespace internal
 }  // namespace v8