[shared-struct] Be lenient deserializing shared objects

ValueDeserializer should be lenient and not assume correct usage when
deserializing shared objects. This CL makes ValueDeserializer throw if
a shared object conveyor id or shared object id are not found.

Bug: v8:12547, chromium:1359227
Change-Id: I429a37dfadd95e42edca5d4870eb5188cb013bc7
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3872549Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Adam Klein <adamk@chromium.org>
Commit-Queue: Shu-yu Guo <syg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#83004}

src/execution/isolate.h

@@ -2002,7 +2002,10 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
 
   SharedObjectConveyors* GetSharedObjectConveyors() const {
     if (is_shared()) return shared_object_conveyors_.get();
-    return shared_isolate()->shared_object_conveyors_.get();
+    if (shared_isolate()) {
+      return shared_isolate()->shared_object_conveyors_.get();
+    }
+    return nullptr;
   }
 
   bool log_object_relocation() const { return log_object_relocation_; }

src/handles/shared-object-conveyors.cc

@@ -21,7 +21,7 @@ uint32_t SharedObjectConveyorHandles::Persist(HeapObject shared_object) {
 }
 
 HeapObject SharedObjectConveyorHandles::GetPersisted(uint32_t object_id) {
-  DCHECK_LT(object_id, shared_objects_.size());
+  DCHECK(HasPersisted(object_id));
   return *shared_objects_[object_id];
 }
 
@@ -55,23 +55,23 @@ SharedObjectConveyorHandles* SharedObjectConveyors::NewConveyor() {
   return conveyors_[id].get();
 }
 
-SharedObjectConveyorHandles* SharedObjectConveyors::GetConveyor(
+SharedObjectConveyorHandles* SharedObjectConveyors::MaybeGetConveyor(
     uint32_t conveyor_id) {
   base::MutexGuard guard(&conveyors_mutex_);
-  DcheckIsValidConveyorId(conveyor_id);
-  return conveyors_[conveyor_id].get();
+  if (HasConveyor(conveyor_id)) return conveyors_[conveyor_id].get();
+  return nullptr;
 }
 
 void SharedObjectConveyors::DeleteConveyor(uint32_t conveyor_id) {
   base::MutexGuard guard(&conveyors_mutex_);
-  DcheckIsValidConveyorId(conveyor_id);
+  DCHECK(HasConveyor(conveyor_id));
   conveyors_[conveyor_id].reset(nullptr);
 }
 
-void SharedObjectConveyors::DcheckIsValidConveyorId(uint32_t conveyor_id) {
-  DCHECK_LT(conveyor_id, conveyors_.size());
-  DCHECK_NOT_NULL(conveyors_[conveyor_id].get());
-  DCHECK_EQ(conveyors_[conveyor_id]->id, conveyor_id);
+bool SharedObjectConveyors::HasConveyor(uint32_t conveyor_id) const {
+  return conveyor_id < conveyors_.size() &&
+         conveyors_[conveyor_id] != nullptr &&
+         conveyors_[conveyor_id]->id == conveyor_id;
 }
 
 }  // namespace internal

src/handles/shared-object-conveyors.h

@@ -37,11 +37,14 @@ class SharedObjectConveyorHandles {
   SharedObjectConveyorHandles& operator=(const SharedObjectConveyorHandles&) =
       delete;
 
-  // Persist and GetPersisted are not threadsafe. A particular conveyor is used
-  // by a single thread at a time, either during sending a message or receiving
-  // a message.
+  // Persist, GetPersisted, and HasPersisted are not threadsafe. A particular
+  // conveyor is used by a single thread at a time, either during sending a
+  // message or receiving a message.
   uint32_t Persist(HeapObject shared_object);
   HeapObject GetPersisted(uint32_t object_id);
+  bool HasPersisted(uint32_t object_id) const {
+    return object_id < shared_objects_.size();
+  }
 
   // Deleting conveyors is threadsafe and may be called from multiple threads.
   void Delete();
@@ -60,15 +63,14 @@ class SharedObjectConveyors {
   explicit SharedObjectConveyors(Isolate* isolate) : isolate_(isolate) {}
 
   SharedObjectConveyorHandles* NewConveyor();
-  SharedObjectConveyorHandles* GetConveyor(uint32_t conveyor_id);
+  SharedObjectConveyorHandles* MaybeGetConveyor(uint32_t conveyor_id);
 
  private:
   friend class SharedObjectConveyorHandles;
 
+  bool HasConveyor(uint32_t conveyor_id) const;
   void DeleteConveyor(uint32_t conveyor_id);
 
-  void DcheckIsValidConveyorId(uint32_t conveyor_id);
-
   Isolate* isolate_;
   base::Mutex conveyors_mutex_;
   std::vector<std::unique_ptr<SharedObjectConveyorHandles>> conveyors_;

src/objects/value-serializer.cc

@@ -2256,6 +2256,20 @@ MaybeHandle<WasmMemoryObject> ValueDeserializer::ReadWasmMemory() {
 }
 #endif  // V8_ENABLE_WEBASSEMBLY
 
+namespace {
+
+// Throws a generic "deserialization failed" exception by default, unless a more
+// specific exception has already been thrown.
+void ThrowDeserializationExceptionIfNonePending(Isolate* isolate) {
+  if (!isolate->has_pending_exception()) {
+    isolate->Throw(*isolate->factory()->NewError(
+        MessageTemplate::kDataCloneDeserializationError));
+  }
+  DCHECK(isolate->has_pending_exception());
+}
+
+}  // namespace
+
 MaybeHandle<HeapObject> ValueDeserializer::ReadSharedObject() {
   STACK_CHECK(isolate_, MaybeHandle<HeapObject>());
   DCHECK_GE(version_, 15);
@@ -2265,9 +2279,15 @@ MaybeHandle<HeapObject> ValueDeserializer::ReadSharedObject() {
     RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate_, HeapObject);
     return MaybeHandle<HeapObject>();
   }
+
   // The conveyor must have already been gotten via the kSharedObjectConveyor
   // tag.
-  DCHECK_NOT_NULL(shared_object_conveyor_);
+  if (shared_object_conveyor_ == nullptr ||
+      !shared_object_conveyor_->HasPersisted(shared_object_id)) {
+    ThrowDeserializationExceptionIfNonePending(isolate_);
+    return MaybeHandle<HeapObject>();
+  }
+
   Handle<HeapObject> shared_object(
       shared_object_conveyor_->GetPersisted(shared_object_id), isolate_);
   DCHECK(shared_object->IsShared());
@@ -2277,15 +2297,27 @@ MaybeHandle<HeapObject> ValueDeserializer::ReadSharedObject() {
 bool ValueDeserializer::ReadSharedObjectConveyor() {
   STACK_CHECK(isolate_, false);
   DCHECK_GE(version_, 15);
-  // This tag appears at most once per deserialization data.
-  DCHECK_NULL(shared_object_conveyor_);
+
   uint32_t conveyor_id;
   if (!ReadVarint<uint32_t>().To(&conveyor_id)) {
     RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate_, false);
     return false;
   }
+
+  // This tag appears at most once per deserialization data.
+  if (shared_object_conveyor_ != nullptr ||
+      isolate_->GetSharedObjectConveyors() == nullptr) {
+    ThrowDeserializationExceptionIfNonePending(isolate_);
+    return false;
+  }
+
   shared_object_conveyor_ =
-      isolate_->GetSharedObjectConveyors()->GetConveyor(conveyor_id);
+      isolate_->GetSharedObjectConveyors()->MaybeGetConveyor(conveyor_id);
+  if (shared_object_conveyor_ == nullptr) {
+    ThrowDeserializationExceptionIfNonePending(isolate_);
+    return false;
+  }
+
   return true;
 }
 
@@ -2521,20 +2553,6 @@ static Maybe<bool> SetPropertiesFromKeyValuePairs(Isolate* isolate,
   return Just(true);
 }
 
-namespace {
-
-// Throws a generic "deserialization failed" exception by default, unless a more
-// specific exception has already been thrown.
-void ThrowDeserializationExceptionIfNonePending(Isolate* isolate) {
-  if (!isolate->has_pending_exception()) {
-    isolate->Throw(*isolate->factory()->NewError(
-        MessageTemplate::kDataCloneDeserializationError));
-  }
-  DCHECK(isolate->has_pending_exception());
-}
-
-}  // namespace
-
 MaybeHandle<Object>
 ValueDeserializer::ReadObjectUsingEntireBufferForLegacyFormat() {
   DCHECK_EQ(version_, 0u);

test/mjsunit/shared-memory/value-serializer.js

+// Copyright 2022 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.
+
+// The value deserializer should be lenient of malformed or malicious data and
+// should throw instead of crash on non-existent shared objects in serialized
+// data.
+
+(function Conveyor() {
+  // Conveyor is 'q', ASCII 113.
+  const data = new Uint8Array([255, 15, 113, 0]);
+  assertThrows(() => { d8.serializer.deserialize(data.buffer); });
+})();
+
+(function SharedObjectNoConveyor() {
+  // Shared object is 'p', ASCII 112.
+  const data = new Uint8Array([255, 15, 112, 0]);
+  assertThrows(() => { d8.serializer.deserialize(data.buffer); });
+})();
+
+(function SharedObjectAndConveyor() {
+  const data = new Uint8Array([255, 15, 113, 0, 112, 0]);
+  assertThrows(() => { d8.serializer.deserialize(data.buffer); });
+})();