Use safer types for mojo::Wrap/UnwrapPlatformFile

This changes mojo::Wrap/UnwrapPlatformFile to use
base::ScopedPlatformFile instead of raw PlatformFile values.

In the process, this fixes potential file descriptor leaks in
chromeos_camera code.

Fixed: 710376
Change-Id: Id86db91695bf3a6e87e210c2226739dbc9d8fa01
Tbr: lamzin@google.com
Tbr: drubery@chromium.org
Tbr: rakeshsoma@google.com
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2491545Reviewed-by: Shik Chen <shik@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#820304}

chrome/browser/chromeos/arc/print_spooler/arc_print_spooler_util.cc

@@ -22,14 +22,14 @@ void DeletePrintDocument(const base::FilePath& file_path) {
 }
 
 base::FilePath SavePrintDocument(mojo::ScopedHandle scoped_handle) {
-  base::PlatformFile platform_file = base::kInvalidPlatformFile;
+  base::ScopedPlatformFile platform_file;
   if (mojo::UnwrapPlatformFile(std::move(scoped_handle), &platform_file) !=
       MOJO_RESULT_OK) {
     PLOG(ERROR) << "UnwrapPlatformFile failed.";
     return base::FilePath();
   }
 
-  base::File src_file(platform_file);
+  base::File src_file(std::move(platform_file));
   if (!src_file.IsValid()) {
     PLOG(ERROR) << "Source file is invalid.";
     return base::FilePath();

chrome/browser/chromeos/arc/print_spooler/print_session_impl.cc

@@ -147,13 +147,13 @@ mojom::PrintDocumentRequestPtr PrintDocumentRequestFromJobSettings(
 base::ReadOnlySharedMemoryRegion ReadPreviewDocument(
     mojo::ScopedHandle preview_document,
     size_t data_size) {
-  base::PlatformFile platform_file;
+  base::ScopedPlatformFile platform_file;
   if (mojo::UnwrapPlatformFile(std::move(preview_document), &platform_file) !=
       MOJO_RESULT_OK) {
     return base::ReadOnlySharedMemoryRegion();
   }
 
-  base::File src_file(platform_file);
+  base::File src_file(std::move(platform_file));
   if (!src_file.IsValid()) {
     DPLOG(ERROR) << "Source file is invalid.";
     return base::ReadOnlySharedMemoryRegion();

chrome/browser/chromeos/arc/screen_capture/arc_screen_capture_session.cc

@@ -198,7 +198,7 @@ void ArcScreenCaptureSession::SetOutputBuffer(
   handle.type = gfx::NATIVE_PIXMAP;
   // Dummy modifier.
   handle.native_pixmap_handle.modifier = 0;
-  base::PlatformFile platform_file;
+  base::ScopedPlatformFile platform_file;
   MojoResult mojo_result =
       mojo::UnwrapPlatformFile(std::move(graphics_buffer), &platform_file);
   if (mojo_result != MOJO_RESULT_OK) {
@@ -208,7 +208,7 @@ void ArcScreenCaptureSession::SetOutputBuffer(
   }
   handle.native_pixmap_handle.planes.emplace_back(
       stride * kBytesPerPixel, 0, stride * kBytesPerPixel * size_.height(),
-      base::ScopedFD(platform_file));
+      std::move(platform_file));
   std::unique_ptr<gfx::GpuMemoryBuffer> gpu_memory_buffer =
       gpu::GpuMemoryBufferImplNativePixmap::CreateFromHandle(
           client_native_pixmap_factory_.get(), std::move(handle), size_,

chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc

@@ -375,7 +375,7 @@ void ArcTracingBridge::StartTracing(const std::string& config,
   }
 
   tracing_instance->StartTracing(
-      selected_categories, mojo::WrapPlatformFile(write_fd.release()),
+      selected_categories, mojo::WrapPlatformFile(std::move(write_fd)),
       base::BindOnce(&ArcTracingBridge::OnArcTracingStarted,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 

chrome/browser/chromeos/wilco_dtc_supportd/mojo_utils.cc

@@ -23,12 +23,12 @@ base::StringPiece MojoUtils::GetStringPieceFromMojoHandle(
     base::ReadOnlySharedMemoryMapping* shared_memory) {
   DCHECK(shared_memory);
 
-  base::PlatformFile platform_file;
+  base::ScopedPlatformFile platform_file;
   auto result = mojo::UnwrapPlatformFile(std::move(handle), &platform_file);
   if (result != MOJO_RESULT_OK)
     return base::StringPiece();
 
-  base::File file(platform_file);
+  base::File file(std::move(platform_file));
   size_t file_size = 0;
   {
     // TODO(b/146119375): Remove blocking operation from production code.
@@ -69,8 +69,7 @@ mojo::ScopedHandle MojoUtils::CreateReadOnlySharedMemoryMojoHandle(
   base::subtle::PlatformSharedMemoryRegion platform_region =
       base::ReadOnlySharedMemoryRegion::TakeHandleForSerialization(
           std::move(shm.region));
-  return mojo::WrapPlatformFile(
-      platform_region.PassPlatformHandle().fd.release());
+  return mojo::WrapPlatformFile(platform_region.PassPlatformHandle().fd);
 }
 
 }  // namespace chromeos

chrome/chrome_cleaner/mojom/typemaps/windows_handle_mojom_traits.cc

@@ -4,6 +4,8 @@
 
 #include "chrome/chrome_cleaner/mojom/typemaps/windows_handle_mojom_traits.h"
 
+#include <windows.h>
+
 namespace mojo {
 
 using chrome_cleaner::mojom::PredefinedHandle;

chrome/chrome_cleaner/mojom/typemaps/windows_handle_mojom_traits.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_CHROME_CLEANER_MOJOM_TYPEMAPS_WINDOWS_HANDLE_MOJOM_TRAITS_H_
 #define CHROME_CHROME_CLEANER_MOJOM_TYPEMAPS_WINDOWS_HANDLE_MOJOM_TRAITS_H_
 
+#include "base/win/windows_types.h"
 #include "chrome/chrome_cleaner/mojom/windows_handle.mojom-shared.h"
 #include "mojo/public/cpp/bindings/union_traits.h"
 

chrome/credential_provider/gaiacp/credential_provider_broker_win.cc

@@ -7,9 +7,11 @@
 #define INITGUID
 
 // clang-format off
+#include <windows.h>
 #include <hidclass.h>
 #include <hidsdi.h>
 #include <hidpi.h>
+#include <setupapi.h>
 // clang-format on
 
 #include "base/memory/free_deleter.h"

chrome/credential_provider/gaiacp/credential_provider_broker_win.h

@@ -7,8 +7,6 @@
 
 #include "chrome/credential_provider/gaiacp/mojom/gaia_credential_provider_win_hid.mojom.h"
 
-#include <setupapi.h>
-
 #include "base/strings/string16.h"
 #include "base/win/scoped_handle.h"
 

components/arc/timer/arc_timer_bridge.cc

@@ -40,13 +40,12 @@ void OnStartTimer(mojom::TimerHost::StartTimerCallback callback, bool result) {
 
 // Unwraps a mojo handle to a file descriptor on the system.
 base::ScopedFD UnwrapScopedHandle(mojo::ScopedHandle handle) {
-  base::PlatformFile platform_file;
+  base::ScopedPlatformFile platform_file;
   if (mojo::UnwrapPlatformFile(std::move(handle), &platform_file) !=
       MOJO_RESULT_OK) {
     LOG(ERROR) << "Failed to unwrap mojo handle";
-    return base::ScopedFD();
   }
-  return base::ScopedFD(platform_file);
+  return platform_file;
 }
 
 // Returns true iff |arc_timer_requests| contains duplicate clock id values.

components/arc/timer/arc_timer_bridge_unittest.cc

@@ -38,7 +38,7 @@ namespace {
 // Converts a system file descriptor to a mojo handle that can be sent to the
 // host.
 mojo::ScopedHandle WrapPlatformFd(base::ScopedFD scoped_fd) {
-  mojo::ScopedHandle handle = mojo::WrapPlatformFile(scoped_fd.release());
+  mojo::ScopedHandle handle = mojo::WrapPlatformFile(std::move(scoped_fd));
   if (!handle.is_valid()) {
     LOG(ERROR) << "Failed to wrap platform handle";
     return mojo::ScopedHandle();

components/arc/video_accelerator/arc_video_accelerator_util.cc

@@ -21,15 +21,12 @@ base::ScopedFD UnwrapFdFromMojoHandle(mojo::ScopedHandle handle) {
     return base::ScopedFD();
   }
 
-  base::PlatformFile platform_file;
+  base::ScopedPlatformFile platform_file;
   MojoResult mojo_result =
       mojo::UnwrapPlatformFile(std::move(handle), &platform_file);
-  if (mojo_result != MOJO_RESULT_OK) {
+  if (mojo_result != MOJO_RESULT_OK)
     VLOGF(1) << "UnwrapPlatformFile failed: " << mojo_result;
-    return base::ScopedFD();
-  }
-
-  return base::ScopedFD(platform_file);
+  return platform_file;
 }
 
 std::vector<base::ScopedFD> DuplicateFD(base::ScopedFD fd, size_t num_fds) {

components/arc/video_accelerator/protected_buffer_manager_proxy.cc

@@ -28,7 +28,7 @@ void GpuArcProtectedBufferManagerProxy::GetProtectedSharedMemoryFromHandle(
       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()));
+  std::move(callback).Run(mojo::WrapPlatformFile(std::move(fd_pair.fd)));
 }
 
 }  // namespace arc

components/chromeos_camera/mojo_jpeg_encode_accelerator_service.cc

@@ -127,9 +127,9 @@ void MojoJpegEncodeAcceleratorService::EncodeWithFD(
     uint32_t output_buffer_size,
     EncodeWithFDCallback callback) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  base::PlatformFile input_fd;
-  base::PlatformFile exif_fd;
-  base::PlatformFile output_fd;
+  base::ScopedPlatformFile input_fd;
+  base::ScopedPlatformFile exif_fd;
+  base::ScopedPlatformFile output_fd;
   MojoResult result;
 
   if (coded_size_width <= 0 || coded_size_height <= 0) {
@@ -166,13 +166,13 @@ void MojoJpegEncodeAcceleratorService::EncodeWithFD(
   base::UnsafeSharedMemoryRegion input_region =
       base::UnsafeSharedMemoryRegion::Deserialize(
           base::subtle::PlatformSharedMemoryRegion::Take(
-              base::ScopedFD(input_fd),
+              std::move(input_fd),
               base::subtle::PlatformSharedMemoryRegion::Mode::kUnsafe,
               input_buffer_size, base::UnguessableToken::Create()));
 
   base::subtle::PlatformSharedMemoryRegion output_shm_region =
       base::subtle::PlatformSharedMemoryRegion::Take(
-          base::ScopedFD(output_fd),
+          std::move(output_fd),
           base::subtle::PlatformSharedMemoryRegion::Mode::kUnsafe,
           output_buffer_size, base::UnguessableToken::Create());
 
@@ -182,8 +182,7 @@ void MojoJpegEncodeAcceleratorService::EncodeWithFD(
   if (exif_buffer_size > 0) {
     base::subtle::PlatformSharedMemoryRegion exif_shm_region =
         base::subtle::PlatformSharedMemoryRegion::Take(
-            base::subtle::ScopedFDPair(base::ScopedFD(exif_fd),
-                                       base::ScopedFD()),
+            base::subtle::ScopedFDPair(std::move(exif_fd), base::ScopedFD()),
             base::subtle::PlatformSharedMemoryRegion::Mode::kUnsafe,
             exif_buffer_size, base::UnguessableToken::Create());
     exif_buffer = std::make_unique<media::BitstreamBuffer>(
@@ -261,7 +260,7 @@ void MojoJpegEncodeAcceleratorService::EncodeWithDmaBuf(
     return;
   }
 
-  base::PlatformFile exif_fd;
+  base::ScopedPlatformFile exif_fd;
   auto result = mojo::UnwrapPlatformFile(std::move(exif_handle), &exif_fd);
   if (result != MOJO_RESULT_OK) {
     std::move(callback).Run(
@@ -289,8 +288,7 @@ void MojoJpegEncodeAcceleratorService::EncodeWithDmaBuf(
     // the encode task process from both Chrome OS and Chrome side.
     base::subtle::PlatformSharedMemoryRegion exif_shm_region =
         base::subtle::PlatformSharedMemoryRegion::Take(
-            base::subtle::ScopedFDPair(base::ScopedFD(exif_fd),
-                                       base::ScopedFD()),
+            base::subtle::ScopedFDPair(std::move(exif_fd), base::ScopedFD()),
             base::subtle::PlatformSharedMemoryRegion::Mode::kUnsafe,
             exif_buffer_size, base::UnguessableToken::Create());
     exif_buffer = std::make_unique<media::BitstreamBuffer>(

gpu/ipc/common/luid_mojom_traits.h

@@ -5,6 +5,8 @@
 #ifndef GPU_IPC_COMMON_LUID_MOJOM_TRAITS_H_
 #define GPU_IPC_COMMON_LUID_MOJOM_TRAITS_H_
 
+#include <windows.h>
+
 #include "gpu/ipc/common/luid.mojom-shared.h"
 
 namespace mojo {

ipc/ipc_message_attachment.cc

@@ -61,7 +61,7 @@ mojo::ScopedHandle MessageAttachment::TakeMojoHandle() {
         DPLOG(WARNING) << "Failed to dup FD to transmit.";
         return mojo::ScopedHandle();
       }
-      return mojo::WrapPlatformFile(file.release());
+      return mojo::WrapPlatformFile(std::move(file));
     }
 #endif  // defined(OS_POSIX) || defined(OS_FUCHSIA)
 
@@ -94,8 +94,8 @@ mojo::ScopedHandle MessageAttachment::TakeMojoHandle() {
     }
 #elif defined(OS_WIN)
     case Type::WIN_HANDLE:
-      return mojo::WrapPlatformFile(
-          static_cast<internal::HandleAttachmentWin*>(this)->Take());
+      return mojo::WrapPlatformFile(base::win::ScopedHandle(
+          static_cast<internal::HandleAttachmentWin*>(this)->Take()));
 #endif
     default:
       break;

mojo/public/cpp/system/platform_handle.cc

@@ -14,19 +14,20 @@ namespace mojo {
 
 namespace {
 
-uint64_t PlatformHandleValueFromPlatformFile(base::PlatformFile file) {
+uint64_t ReleasePlatformHandleValueFromPlatformFile(
+    base::ScopedPlatformFile file) {
 #if defined(OS_WIN)
-  return reinterpret_cast<uint64_t>(file);
+  return reinterpret_cast<uint64_t>(file.Take());
 #else
-  return static_cast<uint64_t>(file);
+  return static_cast<uint64_t>(file.release());
 #endif
 }
 
-base::PlatformFile PlatformFileFromPlatformHandleValue(uint64_t value) {
+base::ScopedPlatformFile PlatformFileFromPlatformHandleValue(uint64_t value) {
 #if defined(OS_WIN)
-  return reinterpret_cast<base::PlatformFile>(value);
+  return base::ScopedPlatformFile(reinterpret_cast<base::PlatformFile>(value));
 #else
-  return static_cast<base::PlatformFile>(value);
+  return base::ScopedPlatformFile(static_cast<base::PlatformFile>(value));
 #endif
 }
 
@@ -198,12 +199,12 @@ PlatformHandle UnwrapPlatformHandle(ScopedHandle handle) {
   return PlatformHandle::FromMojoPlatformHandle(&platform_handle);
 }
 
-// Wraps a PlatformFile as a Mojo handle. Takes ownership of the file object.
-ScopedHandle WrapPlatformFile(base::PlatformFile platform_file) {
+ScopedHandle WrapPlatformFile(base::ScopedPlatformFile platform_file) {
   MojoPlatformHandle platform_handle;
   platform_handle.struct_size = sizeof(MojoPlatformHandle);
   platform_handle.type = kPlatformFileHandleType;
-  platform_handle.value = PlatformHandleValueFromPlatformFile(platform_file);
+  platform_handle.value =
+      ReleasePlatformHandleValueFromPlatformFile(std::move(platform_file));
 
   MojoHandle mojo_handle;
   MojoResult result =
@@ -213,7 +214,8 @@ ScopedHandle WrapPlatformFile(base::PlatformFile platform_file) {
   return ScopedHandle(Handle(mojo_handle));
 }
 
-MojoResult UnwrapPlatformFile(ScopedHandle handle, base::PlatformFile* file) {
+MojoResult UnwrapPlatformFile(ScopedHandle handle,
+                              base::ScopedPlatformFile* file) {
   MojoPlatformHandle platform_handle;
   platform_handle.struct_size = sizeof(MojoPlatformHandle);
   MojoResult result = MojoUnwrapPlatformHandle(handle.release().value(),
@@ -222,7 +224,7 @@ MojoResult UnwrapPlatformFile(ScopedHandle handle, base::PlatformFile* file) {
     return result;
 
   if (platform_handle.type == MOJO_PLATFORM_HANDLE_TYPE_INVALID) {
-    *file = base::kInvalidPlatformFile;
+    *file = base::ScopedPlatformFile();
   } else {
     CHECK_EQ(platform_handle.type, kPlatformFileHandleType);
     *file = PlatformFileFromPlatformHandleValue(platform_handle.value);

mojo/public/cpp/system/platform_handle.h

@@ -13,7 +13,6 @@
 
 #include <stdint.h>
 
-#include "base/compiler_specific.h"
 #include "base/files/platform_file.h"
 #include "base/macros.h"
 #include "base/memory/read_only_shared_memory_region.h"
@@ -27,53 +26,16 @@
 #include "mojo/public/cpp/system/handle.h"
 #include "mojo/public/cpp/system/system_export.h"
 
-#if defined(OS_WIN)
-#include <windows.h>
-#endif
-
 namespace mojo {
 
 #if defined(OS_WIN)
 const MojoPlatformHandleType kPlatformFileHandleType =
     MOJO_PLATFORM_HANDLE_TYPE_WINDOWS_HANDLE;
-
-const MojoPlatformHandleType kPlatformSharedBufferHandleType =
-    MOJO_PLATFORM_HANDLE_TYPE_WINDOWS_HANDLE;
-
-#elif defined(OS_FUCHSIA)
-const MojoPlatformHandleType kPlatformFileHandleType =
-    MOJO_PLATFORM_HANDLE_TYPE_FILE_DESCRIPTOR;
-const MojoPlatformHandleType kPlatformSharedBufferHandleType =
-    MOJO_PLATFORM_HANDLE_TYPE_FUCHSIA_HANDLE;
-
-#elif defined(OS_POSIX)
-const MojoPlatformHandleType kPlatformFileHandleType =
-    MOJO_PLATFORM_HANDLE_TYPE_FILE_DESCRIPTOR;
-
-#if defined(OS_MAC)
-const MojoPlatformHandleType kPlatformSharedBufferHandleType =
-    MOJO_PLATFORM_HANDLE_TYPE_MACH_PORT;
 #else
-const MojoPlatformHandleType kPlatformSharedBufferHandleType =
+const MojoPlatformHandleType kPlatformFileHandleType =
     MOJO_PLATFORM_HANDLE_TYPE_FILE_DESCRIPTOR;
-#endif  // defined(OS_MAC)
-
 #endif  // defined(OS_WIN)
 
-// Used to specify the protection status of a base::SharedMemoryHandle memory
-// handle wrapped or unwrapped by mojo::WrapSharedMemoryHandle or
-// mojo::UnwrapSharedMemoryHandle below. See those functions for additional
-// details.
-enum class UnwrappedSharedMemoryHandleProtection {
-  // Indicates that the base::SharedMemoryHandle supports being mapped to
-  // writable memory regions.
-  kReadWrite,
-
-  // Indicates that the base::SharedMemoryHandle supports being mapped only to
-  // read-only memory regions.
-  kReadOnly,
-};
-
 // Wraps and unwraps base::subtle::PlatformSharedMemoryRegions. This should be
 // used only while transitioning from the legacy shared memory API. In new code
 // only base::*SharedMemoryRegion should be used instead.
@@ -90,14 +52,15 @@ MOJO_CPP_SYSTEM_EXPORT ScopedHandle WrapPlatformHandle(PlatformHandle handle);
 // support library.
 MOJO_CPP_SYSTEM_EXPORT PlatformHandle UnwrapPlatformHandle(ScopedHandle handle);
 
-// Wraps a PlatformFile as a Mojo handle. Takes ownership of the file object.
-// If |platform_file| is valid, this will return a valid handle.
+// Wraps a ScopedPlatformFile as a Mojo handle. Takes ownership of the file
+// object. If |platform_file| is valid, this will return a valid handle.
 MOJO_CPP_SYSTEM_EXPORT
-ScopedHandle WrapPlatformFile(base::PlatformFile platform_file);
+ScopedHandle WrapPlatformFile(base::ScopedPlatformFile platform_file);
 
 // Unwraps a PlatformFile from a Mojo handle.
 MOJO_CPP_SYSTEM_EXPORT
-MojoResult UnwrapPlatformFile(ScopedHandle handle, base::PlatformFile* file);
+MojoResult UnwrapPlatformFile(ScopedHandle handle,
+                              base::ScopedPlatformFile* file);
 
 // Helpers for wrapping and unwrapping new base shared memory API primitives.
 // If the input |region| is valid for the Wrap* functions, they will always