ARC: upgrade protected buffers to new shared memory api

This change wraps the FD-based protected buffers in a
base::subtle::PlatformSharedMemoryRegion rather than the legacy
SharedMemoryHandle. Aside from a couple of new validation checks,
functionality is unchanged.

Bug: 849207

Change-Id: I2df619dc58c6313393e41eef7e4807edf340875e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1697183
Commit-Queue: Matthew Cary (CET) <mattcary@chromium.org>
Reviewed-by: Matthew Cary (CET) <mattcary@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#686169}

components/arc/mojom/protected_buffer_manager.mojom

@@ -8,8 +8,8 @@
 module arc.mojom;
 
 // This interface is exposed by the GPU process for translating dummy handles
-// for secure buffers into a usable shared memory handle. The output of a
-// decryption operation can then be written to that shared memory and will be
+// for secure buffers into a usable shared memory buffer handle. The output of
+// a decryption operation can then be written to that shared memory and will be
 // consumed by the video decoder in the GPU.
 // NOTE: This does not use a shared memory handle for the return type
 // because of incompatibilities between Chrome and Chrome OS mojo versions

components/arc/video_accelerator/gpu_arc_video_decode_accelerator.cc

@@ -358,12 +358,12 @@ void GpuArcVideoDecodeAccelerator::Decode(
   }
   DVLOGF(4) << "fd=" << handle_fd.get();
 
-  base::SharedMemoryHandle shm_handle;
+  base::subtle::PlatformSharedMemoryRegion shm_region;
   if (secure_mode_) {
     // Use protected shared memory associated with the given file descriptor.
-    shm_handle = protected_buffer_manager_->GetProtectedSharedMemoryHandleFor(
+    shm_region = protected_buffer_manager_->GetProtectedSharedMemoryRegionFor(
         std::move(handle_fd));
-    if (!shm_handle.IsValid()) {
+    if (!shm_region.IsValid()) {
       VLOGF(1) << "No protected shared memory found for handle";
       client_->NotifyError(
           mojom::VideoDecodeAccelerator::Result::INVALID_ARGUMENT);
@@ -376,9 +376,16 @@ void GpuArcVideoDecodeAccelerator::Decode(
           mojom::VideoDecodeAccelerator::Result::INVALID_ARGUMENT);
       return;
     }
-    shm_handle = base::SharedMemoryHandle(
-        base::FileDescriptor(handle_fd.release(), true), handle_size,
+    shm_region = base::subtle::PlatformSharedMemoryRegion::Take(
+        std::move(handle_fd),
+        base::subtle::PlatformSharedMemoryRegion::Mode::kUnsafe, handle_size,
         base::UnguessableToken::Create());
+    if (!shm_region.IsValid()) {
+      VLOGF(1) << "Cannot take file descriptor based shared memory";
+      client_->NotifyError(
+          mojom::VideoDecodeAccelerator::Result::INVALID_ARGUMENT);
+      return;
+    }
   }
 
   // Use Unretained(this) is safe, this callback will be executed in
@@ -387,19 +394,12 @@ void GpuArcVideoDecodeAccelerator::Decode(
   // All the callbacks thus should be called or deleted before |this| is
   // invalidated.
   ExecuteRequest(
-      {base::BindOnce(&GpuArcVideoDecodeAccelerator::DecodeRequest,
-                      base::Unretained(this),
-                      media::BitstreamBuffer(bitstream_buffer->bitstream_id,
-                                             shm_handle, false /* read_only */,
-                                             bitstream_buffer->bytes_used,
-                                             bitstream_buffer->offset)),
+      {base::BindOnce(
+           &GpuArcVideoDecodeAccelerator::DecodeRequest, base::Unretained(this),
+           media::BitstreamBuffer(
+               bitstream_buffer->bitstream_id, std::move(shm_region),
+               bitstream_buffer->bytes_used, bitstream_buffer->offset)),
        PendingCallback()});
-
-  // Close |shm_handle| because it is actually duplicated on the ctor of
-  // media::BitstreamBuffer and it will not close itself on the dtor.
-  if (shm_handle.IsValid()) {
-    shm_handle.Close();
-  }
 }
 
 void GpuArcVideoDecodeAccelerator::AssignPictureBuffers(uint32_t count) {

components/arc/video_accelerator/gpu_arc_video_encode_accelerator.cc

@@ -263,18 +263,16 @@ void GpuArcVideoEncodeAccelerator::UseBitstreamBuffer(
   // rather than pulling out the fd. https://crbug.com/713763.
   // TODO(rockot): Pass through a real size rather than |0|.
   base::UnguessableToken guid = base::UnguessableToken::Create();
-  base::SharedMemoryHandle shm_handle(base::FileDescriptor(fd.release(), true),
-                                      shmem_size, guid);
-  use_bitstream_cbs_.emplace(bitstream_buffer_serial_, std::move(callback));
-  accelerator_->UseOutputBitstreamBuffer(
-      media::BitstreamBuffer(bitstream_buffer_serial_, shm_handle,
-                             false /* read_only */, size, offset));
-
-  // Close |shm_handle| because it is actually duplicated on the ctor of
-  // media::BitstreamBuffer and it will not close itself on the dtor.
-  if (shm_handle.IsValid()) {
-    shm_handle.Close();
+  auto shm_region = base::subtle::PlatformSharedMemoryRegion::Take(
+      std::move(fd), base::subtle::PlatformSharedMemoryRegion::Mode::kUnsafe,
+      shmem_size, guid);
+  if (!shm_region.IsValid()) {
+    client_->NotifyError(Error::kInvalidArgumentError);
+    return;
   }
+  use_bitstream_cbs_.emplace(bitstream_buffer_serial_, std::move(callback));
+  accelerator_->UseOutputBitstreamBuffer(media::BitstreamBuffer(
+      bitstream_buffer_serial_, std::move(shm_region), size, offset));
 
   // Mask against 30 bits to avoid (undefined) wraparound on signed integer.
   bitstream_buffer_serial_ = (bitstream_buffer_serial_ + 1) & 0x3FFFFFFF;

components/arc/video_accelerator/protected_buffer_manager.cc

@@ -9,7 +9,6 @@
 #include "base/bind.h"
 #include "base/bits.h"
 #include "base/logging.h"
-#include "base/memory/shared_memory.h"
 #include "base/system/sys_info.h"
 #include "base/threading/thread_checker.h"
 #include "components/arc/video_accelerator/protected_buffer_allocator.h"
@@ -43,8 +42,9 @@ class ProtectedBufferManager::ProtectedBuffer {
   // Downcasting methods to return duplicated handles to the underlying
   // protected buffers for each buffer type, or empty/null handles if not
   // applicable.
-  virtual base::SharedMemoryHandle DuplicateSharedMemoryHandle() const {
-    return base::SharedMemoryHandle();
+  virtual base::subtle::PlatformSharedMemoryRegion
+  DuplicatePlatformSharedMemoryRegion() const {
+    return {};
   }
   virtual gfx::NativePixmapHandle DuplicateNativePixmapHandle() const {
     return gfx::NativePixmapHandle();
@@ -76,14 +76,15 @@ class ProtectedBufferManager::ProtectedSharedMemory
       scoped_refptr<gfx::NativePixmap> dummy_handle,
       size_t size);
 
-  base::SharedMemoryHandle DuplicateSharedMemoryHandle() const override {
-    return base::SharedMemory::DuplicateHandle(shmem_->handle());
+  base::subtle::PlatformSharedMemoryRegion DuplicatePlatformSharedMemoryRegion()
+      const override {
+    return region_.Duplicate();
   }
 
  private:
   explicit ProtectedSharedMemory(scoped_refptr<gfx::NativePixmap> dummy_handle);
 
-  std::unique_ptr<base::SharedMemory> shmem_;
+  base::subtle::PlatformSharedMemoryRegion region_;
 };
 
 ProtectedBufferManager::ProtectedSharedMemory::ProtectedSharedMemory(
@@ -103,23 +104,12 @@ ProtectedBufferManager::ProtectedSharedMemory::Create(
   size_t aligned_size =
       base::bits::Align(size, base::SysInfo::VMAllocationGranularity());
 
-  mojo::ScopedSharedBufferHandle mojo_shared_buffer =
-      mojo::SharedBufferHandle::Create(aligned_size);
-  if (!mojo_shared_buffer->is_valid()) {
+  protected_shmem->region_ =
+      base::subtle::PlatformSharedMemoryRegion::CreateUnsafe(aligned_size);
+  if (!protected_shmem->region_.IsValid()) {
     VLOGF(1) << "Failed to allocate shared memory";
     return nullptr;
   }
-
-  base::SharedMemoryHandle shm_handle;
-  MojoResult mojo_result = mojo::UnwrapSharedMemoryHandle(
-      std::move(mojo_shared_buffer), &shm_handle, nullptr, nullptr);
-  if (mojo_result != MOJO_RESULT_OK) {
-    VLOGF(1) << "Failed to unwrap a mojo shared memory handle";
-    return nullptr;
-  }
-
-  protected_shmem->shmem_ =
-      std::make_unique<base::SharedMemory>(shm_handle, false);
   return protected_shmem;
 }
 
@@ -395,8 +385,8 @@ void ProtectedBufferManager::ReleaseAllProtectedBuffers(uint64_t allocator_id) {
   allocator_to_buffers_map_.erase(allocator_id);
 }
 
-base::SharedMemoryHandle
-ProtectedBufferManager::GetProtectedSharedMemoryHandleFor(
+base::subtle::PlatformSharedMemoryRegion
+ProtectedBufferManager::GetProtectedSharedMemoryRegionFor(
     base::ScopedFD dummy_fd) {
   uint32_t id = 0;
   auto pixmap = ImportDummyFd(std::move(dummy_fd), &id);
@@ -404,9 +394,9 @@ ProtectedBufferManager::GetProtectedSharedMemoryHandleFor(
   base::AutoLock lock(buffer_map_lock_);
   const auto& iter = buffer_map_.find(id);
   if (iter == buffer_map_.end())
-    return base::SharedMemoryHandle();
+    return {};
 
-  return iter->second->DuplicateSharedMemoryHandle();
+  return iter->second->DuplicatePlatformSharedMemoryRegion();
 }
 
 gfx::NativePixmapHandle

components/arc/video_accelerator/protected_buffer_manager.h

@@ -10,8 +10,8 @@
 #include <set>
 
 #include "base/files/scoped_file.h"
+#include "base/memory/platform_shared_memory_region.h"
 #include "base/memory/ref_counted.h"
-#include "base/memory/shared_memory.h"
 #include "base/memory/weak_ptr.h"
 #include "base/synchronization/lock.h"
 #include "base/thread_annotations.h"
@@ -35,10 +35,10 @@ class ProtectedBufferManager
   CreateProtectedBufferAllocator(
       scoped_refptr<ProtectedBufferManager> protected_buffer_manager);
 
-  // Return a duplicated SharedMemoryHandle associated with the |dummy_fd|,
-  // if one exists, or an invalid handle otherwise.
-  // The client is responsible for closing the handle after use.
-  base::SharedMemoryHandle GetProtectedSharedMemoryHandleFor(
+  // Return a duplicated PlatformSharedMemoryRegion associated with the
+  // |dummy_fd|, if one exists, or an invalid handle otherwise. The client is
+  // responsible for closing the handle after use.
+  base::subtle::PlatformSharedMemoryRegion GetProtectedSharedMemoryRegionFor(
       base::ScopedFD dummy_fd);
 
   // Return a duplicated NativePixmapHandle associated with the |dummy_fd|,

components/arc/video_accelerator/protected_buffer_manager_proxy.cc

@@ -24,12 +24,11 @@ void GpuArcProtectedBufferManagerProxy::GetProtectedSharedMemoryFromHandle(
     GetProtectedSharedMemoryFromHandleCallback callback) {
   base::ScopedFD unwrapped_fd = UnwrapFdFromMojoHandle(std::move(dummy_handle));
 
-  base::ScopedFD shmem_fd(
-      protected_buffer_manager_
-          ->GetProtectedSharedMemoryHandleFor(std::move(unwrapped_fd))
-          .Release());
-
-  std::move(callback).Run(mojo::WrapPlatformFile(shmem_fd.release()));
+  auto region = protected_buffer_manager_->GetProtectedSharedMemoryRegionFor(
+      std::move(unwrapped_fd));
+  // This ScopedFDPair dance is chromeos-specific.
+  base::subtle::ScopedFDPair fd_pair = region.PassPlatformHandle();
+  std::move(callback).Run(mojo::WrapPlatformFile(fd_pair.fd.release()));
 }
 
 }  // namespace arc