NaCl: Use RAII for sockets in NaClProcessHost.

components/nacl/browser/nacl_process_host.cc

Index: components/nacl/browser/nacl_process_host.cc
diff --git a/components/nacl/browser/nacl_process_host.cc b/components/nacl/browser/nacl_process_host.cc
index c461d3e4bf817416de8d9a2f16fd90f2cd47c3ec..4bf53086340ee39b9772dd6019c125215d4aef09 100644
--- a/components/nacl/browser/nacl_process_host.cc
+++ b/components/nacl/browser/nacl_process_host.cc
@@ -76,10 +76,11 @@ using content::ChildProcessData;
 using content::ChildProcessHost;
 using ppapi::proxy::SerializedHandle;
 
-#if defined(OS_WIN)
-
+namespace nacl {
 namespace {
 
+#if defined(OS_WIN)
+
 // Looks for the largest contiguous unallocated region of address
 // space and returns it via |*out_addr| and |*out_size|.
 void FindAddressSpace(base::ProcessHandle process,
@@ -119,8 +120,6 @@ bool IsInPath(const std::string& path_env_var, const std::string& dir) {
 
 }  // namespace
 
-namespace nacl {
-
 // Allocates |size| bytes of address space in the given process at a
 // randomised address.
 void* AllocateAddressSpaceASLR(base::ProcessHandle process, size_t size) {
@@ -138,18 +137,14 @@ void* AllocateAddressSpaceASLR(base::ProcessHandle process, size_t size) {
                         MEM_RESERVE, PAGE_NOACCESS);
 }
 
-}  // namespace nacl
-
-#endif  // defined(OS_WIN)
-
 namespace {
 
-#if defined(OS_WIN)
 bool RunningOnWOW64() {
   return (base::win::OSInfo::GetInstance()->wow64_status() ==
           base::win::OSInfo::WOW64_ENABLED);
 }
-#endif
+
+#endif  // defined(OS_WIN)

[Mark Seaborn, 2014/09/11 20:43:26]

Having #if/#endif nesting not nest with namespace{} is quite confusing.

 
 // NOTE: changes to this class need to be reviewed by the security team.
 class NaClSandboxedProcessLauncherDelegate
@@ -233,19 +228,6 @@ bool ShareHandleToSelLdr(
 
 }  // namespace
 
-namespace nacl {
-
-struct NaClProcessHost::NaClInternal {
-  NaClHandle socket_for_renderer;
-  NaClHandle socket_for_sel_ldr;
-
-  NaClInternal()
-    : socket_for_renderer(NACL_INVALID_HANDLE),
-      socket_for_sel_ldr(NACL_INVALID_HANDLE) { }
-};
-
-// -----------------------------------------------------------------------------
-
 unsigned NaClProcessHost::keepalive_throttle_interval_milliseconds_ =
     ppapi::kKeepaliveThrottleIntervalDefaultMilliseconds;
 
@@ -273,7 +255,6 @@ NaClProcessHost::NaClProcessHost(const GURL& manifest_url,
 #if defined(OS_WIN)
       debug_exception_handler_requested_(false),
 #endif
-      internal_(new NaClInternal()),
       uses_irt_(uses_irt),
       uses_nonsfi_mode_(uses_nonsfi_mode),
       enable_debug_stub_(false),
@@ -313,18 +294,6 @@ NaClProcessHost::~NaClProcessHost() {
     NaClBrowser::GetInstance()->OnProcessEnd(process_->GetData().id);
   }
 
-  if (internal_->socket_for_renderer != NACL_INVALID_HANDLE) {
-    if (NaClClose(internal_->socket_for_renderer) != 0) {
-      NOTREACHED() << "NaClClose() failed";
-    }
-  }
-
-  if (internal_->socket_for_sel_ldr != NACL_INVALID_HANDLE) {
-    if (NaClClose(internal_->socket_for_sel_ldr) != 0) {
-      NOTREACHED() << "NaClClose() failed";
-    }
-  }
-
   if (reply_msg_) {
     // The process failed to launch for some reason.
     // Don't keep the renderer hanging.
@@ -468,8 +437,8 @@ void NaClProcessHost::Launch(
       delete this;
       return;
     }
-    internal_->socket_for_renderer = pair[0];
-    internal_->socket_for_sel_ldr = pair[1];
+    socket_for_renderer_ = base::File(pair[0]);
+    socket_for_sel_ldr_ = base::File(pair[1]);
     SetCloseOnExec(pair[0]);
     SetCloseOnExec(pair[1]);
   }
@@ -721,8 +690,7 @@ bool NaClProcessHost::ReplyToRenderer(
   // Copy the handle into the renderer process.
   HANDLE handle_in_renderer;
   if (!DuplicateHandle(base::GetCurrentProcessHandle(),
-                       reinterpret_cast<HANDLE>(
-                           internal_->socket_for_renderer),
+                       socket_for_renderer_.TakePlatformFile(),
                        nacl_host_message_filter_->PeerHandle(),
                        &handle_in_renderer,
                        0,  // Unused given DUPLICATE_SAME_ACCESS.
@@ -737,7 +705,7 @@ bool NaClProcessHost::ReplyToRenderer(
   // No need to dup the imc_handle - we don't pass it anywhere else so
   // it cannot be closed.
   FileDescriptor imc_handle;
-  imc_handle.fd = internal_->socket_for_renderer;
+  imc_handle.fd = socket_for_renderer_.TakePlatformFile();
   imc_handle.auto_close = true;
   imc_handle_for_renderer = imc_handle;
 #endif
@@ -759,7 +727,6 @@ bool NaClProcessHost::ReplyToRenderer(
                        data.id,
                        crash_info_shmem_renderer_handle),
       std::string() /* error_message */);
-  internal_->socket_for_renderer = NACL_INVALID_HANDLE;
 
   // Now that the crash information shmem handles have been shared with the
   // plugin and the renderer, the browser can close its handle.
@@ -844,7 +811,7 @@ bool NaClProcessHost::StartNaClExecution() {
 #if defined(OS_LINUX)
     // In non-SFI mode, we do not use SRPC. Make sure that the socketpair is
     // not created.
-    DCHECK_EQ(internal_->socket_for_sel_ldr, NACL_INVALID_HANDLE);
+    DCHECK(!socket_for_sel_ldr_.IsValid());
 #endif
   } else {
     params.validation_cache_enabled = nacl_browser->ValidationCacheIsEnabled();
@@ -863,7 +830,8 @@ bool NaClProcessHost::StartNaClExecution() {
 
     const ChildProcessData& data = process_->GetData();
     if (!ShareHandleToSelLdr(data.handle,
-                             internal_->socket_for_sel_ldr, true,
+                             socket_for_sel_ldr_.TakePlatformFile(),
+                             true,
                              &params.handles)) {
       return false;
     }
@@ -912,10 +880,6 @@ bool NaClProcessHost::StartNaClExecution() {
 #endif
   }
 
-  if (!uses_nonsfi_mode_) {
-    internal_->socket_for_sel_ldr = NACL_INVALID_HANDLE;
-  }
-
   params.nexe_file = IPC::TakeFileHandleForProcess(nexe_file_.Pass(),
                                                    process_->GetData().handle);
   if (!crash_info_shmem_.ShareToProcess(process_->GetData().handle,