Don't try to map invalid shm handle.

If we don't have a valid shared memory handle return early and don't try
to map the memory. Also move check for duplicated ids earlier in the
function.

Bug: none
Change-Id: I53b113cc3da2149d15fa70afa8bc8d52bc362e8d
Reviewed-on: https://chromium-review.googlesource.com/c/1326842
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606904}

components/viz/service/display_embedder/server_shared_bitmap_manager.cc

@@ -11,6 +11,7 @@
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/trace_event/process_memory_dump.h"
@@ -98,11 +99,19 @@ bool ServerSharedBitmapManager::ChildAllocatedSharedBitmap(
     const SharedBitmapId& id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
+  // Duplicate ids are not allowed.
+  if (base::ContainsKey(handle_map_, id))
+    return false;
+
   base::SharedMemoryHandle memory_handle;
   size_t buffer_size;
   MojoResult result = mojo::UnwrapSharedMemoryHandle(
       std::move(buffer), &memory_handle, &buffer_size, nullptr);
-  DCHECK_EQ(result, MOJO_RESULT_OK);
+
+  // This function handles public API requests, so verify we unwrapped a shared
+  // memory handle before trying to use the handle.
+  if (result != MOJO_RESULT_OK)
+    return false;
 
   auto data = base::MakeRefCounted<BitmapData>(buffer_size);
   data->memory = std::make_unique<base::SharedMemory>(memory_handle, false);
@@ -112,8 +121,6 @@ bool ServerSharedBitmapManager::ChildAllocatedSharedBitmap(
   data->memory->Map(data->buffer_size);
   data->memory->Close();
 
-  if (handle_map_.find(id) != handle_map_.end())
-    return false;
   handle_map_[id] = std::move(data);
   return true;
 }

components/viz/service/display_embedder/server_shared_bitmap_manager_unittest.cc

@@ -148,5 +148,17 @@ TEST_F(ServerSharedBitmapManagerTest, SharedMemoryHandle) {
   manager()->ChildDeletedSharedBitmap(id);
 }
 
+TEST_F(ServerSharedBitmapManagerTest, InvalidScopedSharedBufferHandle) {
+  SharedBitmapId id = SharedBitmap::GenerateId();
+  mojo::ScopedSharedBufferHandle invalid_handle(
+      mojo::SharedBufferHandle(0x1234567));
+  EXPECT_FALSE(
+      manager()->ChildAllocatedSharedBitmap(std::move(invalid_handle), id));
+
+  // The client could still send an IPC to say it deleted the shared bitmap,
+  // even though it wasn't valid, which should be ignored.
+  manager()->ChildDeletedSharedBitmap(id);
+}
+
 }  // namespace
 }  // namespace viz