NaCl: Use RAII for sockets in NaClProcessHost.

components/nacl/browser/nacl_process_host.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/nacl/browser/nacl_process_host.h"
 
 #include <algorithm>
 #include <string>
 #include <vector>
 
@@ skipping 58 matching lines @@
 #include "components/nacl/browser/nacl_broker_service_win.h"
 #include "components/nacl/common/nacl_debug_exception_handler_win.h"
 #include "content/public/common/sandbox_init.h"
 #endif
 
 using content::BrowserThread;
 using content::ChildProcessData;
 using content::ChildProcessHost;
 using ppapi::proxy::SerializedHandle;
 
+namespace nacl {
+namespace {
+
 #if defined(OS_WIN)
 
-namespace {
-
 // Looks for the largest contiguous unallocated region of address
 // space and returns it via |*out_addr| and |*out_size|.
 void FindAddressSpace(base::ProcessHandle process,
                       char** out_addr, size_t* out_size) {
   *out_addr = NULL;
   *out_size = 0;
   char* addr = 0;
   while (true) {
     MEMORY_BASIC_INFORMATION info;
     size_t result = VirtualQueryEx(process, static_cast<void*>(addr),
@@ skipping 19 matching lines @@
     if (*i == dir)
       return true;
   }
   return false;
 }
 
 #endif  // _DLL
 
 }  // 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) {
   char* addr;
   size_t avail_size;
   FindAddressSpace(process, &addr, &avail_size);
   if (avail_size < size)
     return NULL;
   size_t offset = base::RandGenerator(avail_size - size);
   const int kPageSize = 0x10000;
   void* request_addr =
       reinterpret_cast<void*>(reinterpret_cast<uint64>(addr + offset)
                               & ~(kPageSize - 1));
   return VirtualAllocEx(process, request_addr, 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
     : public content::SandboxedProcessLauncherDelegate {
  public:
   NaClSandboxedProcessLauncherDelegate(ChildProcessHost* host)
 #if defined(OS_POSIX)
       : ipc_fd_(host->TakeClientFileDescriptor())
 #endif
   {}
@@ skipping 63 matching lines @@
   nacl::FileDescriptor channel;
   channel.fd = sourceh;
   channel.auto_close = close_source;
   handles_for_sel_ldr->push_back(channel);
 #endif
   return true;
 }
 
 }  // 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;
 
 NaClProcessHost::NaClProcessHost(const GURL& manifest_url,
                                  base::File nexe_file,
                                  const NaClFileToken& nexe_token,
                                  ppapi::PpapiPermissions permissions,
                                  int render_view_id,
                                  uint32 permission_bits,
                                  bool uses_irt,
                                  bool uses_nonsfi_mode,
                                  bool enable_dyncode_syscalls,
                                  bool enable_exception_handling,
                                  bool enable_crash_throttling,
                                  bool off_the_record,
                                  const base::FilePath& profile_directory)
     : manifest_url_(manifest_url),
       nexe_file_(nexe_file.Pass()),
       nexe_token_(nexe_token),
       permissions_(permissions),
 #if defined(OS_WIN)
       process_launched_by_broker_(false),
 #endif
       reply_msg_(NULL),
 #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),
       enable_dyncode_syscalls_(enable_dyncode_syscalls),
       enable_exception_handling_(enable_exception_handling),
       enable_crash_throttling_(enable_crash_throttling),
       off_the_record_(off_the_record),
       profile_directory_(profile_directory),
       render_view_id_(render_view_id),
       weak_factory_(this) {
@@ skipping 19 matching lines @@
         base::StringPrintf("NaCl process exited with status %i (0x%x)",
                            exit_code, exit_code);
     if (exit_code == 0) {
       VLOG(1) << message;
     } else {
       LOG(ERROR) << message;
     }
     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.
     reply_msg_->set_reply_error();
     nacl_host_message_filter_->Send(reply_msg_);
   }
 #if defined(OS_WIN)
   if (process_launched_by_broker_) {
     NaClBrokerService::GetInstance()->OnLoaderDied();
   }
@@ skipping 123 matching lines @@
     // This means the sandboxed renderer cannot send handles to the
     // browser process.
 
     NaClHandle pair[2];
     // Create a connected socket
     if (NaClSocketPair(pair) == -1) {
       SendErrorToRenderer("NaClSocketPair() failed");
       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]);
   }
 
   // Create a shared memory region that the renderer and plugin share for
   // reporting crash information.
   crash_info_shmem_.CreateAnonymous(kNaClCrashInfoShmemSize);
 
   // Launch the process
   if (!LaunchSelLdr()) {
@@ skipping 231 matching lines @@
       return false;
     }
   }
 #endif
 
   FileDescriptor imc_handle_for_renderer;
 #if defined(OS_WIN)
   // 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.
                        FALSE,
                        DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS)) {
     SendErrorToRenderer("DuplicateHandle() failed");
     return false;
   }
   imc_handle_for_renderer = reinterpret_cast<FileDescriptor>(
       handle_in_renderer);
 #else
   // 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
 
   const ChildProcessData& data = process_->GetData();
   base::SharedMemoryHandle crash_info_shmem_renderer_handle;
   if (!crash_info_shmem_.ShareToProcess(nacl_host_message_filter_->PeerHandle(),
                                         &crash_info_shmem_renderer_handle)) {
     SendErrorToRenderer("ShareToProcess() failed");
     return false;
   }
 
   SendMessageToRenderer(
       NaClLaunchResult(imc_handle_for_renderer,
                        ppapi_channel_handle,
                        trusted_channel_handle,
                        manifest_service_channel_handle,
                        base::GetProcId(data.handle),
                        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.
   crash_info_shmem_.Close();
   return true;
 }
 
 void NaClProcessHost::SendErrorToRenderer(const std::string& error_message) {
   LOG(ERROR) << "NaCl process launch failed: " << error_message;
   SendMessageToRenderer(NaClLaunchResult(), error_message);
@@ skipping 64 matching lines @@
 
   NaClStartParams params;
 
   // Enable PPAPI proxy channel creation only for renderer processes.
   params.enable_ipc_proxy = enable_ppapi_proxy();
   if (uses_nonsfi_mode_) {
     // Currently, non-SFI mode is supported only on Linux.
 #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();
     params.validation_cache_key = nacl_browser->GetValidationCacheKey();
     params.version = NaClBrowser::GetDelegate()->GetVersionString();
     params.enable_exception_handling = enable_exception_handling_;
     params.enable_debug_stub = enable_debug_stub_ &&
         NaClBrowser::GetDelegate()->URLMatchesDebugPatterns(manifest_url_);
     params.uses_irt = uses_irt_;
     params.enable_dyncode_syscalls = enable_dyncode_syscalls_;
 
     // TODO(teravest): Resolve the file tokens right now instead of making the
     // loader send IPC to resolve them later.
     params.nexe_token_lo = nexe_token_.lo;
     params.nexe_token_hi = nexe_token_.hi;
 
     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;
     }
 
     if (params.uses_irt) {
       const base::File& irt_file = nacl_browser->IrtFile();
       CHECK(irt_file.IsValid());
       // Send over the IRT file handle.  We don't close our own copy!
       if (!ShareHandleToSelLdr(data.handle, irt_file.GetPlatformFile(), false,
                                &params.handles)) {
@@ skipping 28 matching lines @@
     if (params.enable_debug_stub) {
       net::SocketDescriptor server_bound_socket = GetDebugStubSocketHandle();
       if (server_bound_socket != net::kInvalidSocket) {
         params.debug_stub_server_bound_socket =
             FileDescriptor(server_bound_socket, true);
       }
     }
 #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,
                                         &params.crash_info_shmem_handle)) {
     DLOG(ERROR) << "Failed to ShareToProcess() a shared memory buffer";
     return false;
   }
 
   process_->Send(new NaClProcessMsg_Start(params));
   return true;
@@ skipping 298 matching lines @@
         process_handle.Take(), info,
         base::MessageLoopProxy::current(),
         base::Bind(&NaClProcessHost::OnDebugExceptionHandlerLaunchedByBroker,
                    weak_factory_.GetWeakPtr()));
     return true;
   }
 }
 #endif
 
 }  // namespace nacl