Use ScopedProcessInformation and other RAII types in sandbox.

sandbox/src/target_process.cc

Index: sandbox/src/target_process.cc
diff --git a/sandbox/src/target_process.cc b/sandbox/src/target_process.cc
index 2710dc000be47fe0804d54be9ae7b69c7170c409..65e472fb55d4ea19df3cd11df80928b5465b5456 100644
--- a/sandbox/src/target_process.cc
+++ b/sandbox/src/target_process.cc
@@ -15,12 +15,6 @@
 
 namespace {
 
-void TerminateTarget(PROCESS_INFORMATION* pi) {
-  ::CloseHandle(pi->hThread);
-  ::TerminateProcess(pi->hProcess, 0);
-  ::CloseHandle(pi->hProcess);
-}
-
 void CopyPolicyToTarget(const void* source, size_t size, void* dest) {
   if (!source || !size)
     return;
@@ -97,14 +91,8 @@ TargetProcess::TargetProcess(HANDLE initial_token, HANDLE lockdown_token,
     : lockdown_token_(lockdown_token),
       initial_token_(initial_token),
       job_(job),
-      shared_section_(NULL),
-      ipc_server_(NULL),
       thread_pool_(thread_pool),
-      base_address_(NULL),
-      exe_name_(NULL),
-      sandbox_process_(NULL),
-      sandbox_thread_(NULL),
-      sandbox_process_id_(0) {
+      base_address_(NULL) {
 }
 
 TargetProcess::~TargetProcess() {
@@ -117,23 +105,18 @@ TargetProcess::~TargetProcess() {
   // it. http://b/893891
   // For now, this wait is there only to do a best effort to prevent some leaks
   // from showing up in purify.
-  ::WaitForSingleObject(sandbox_process_, 50);
-  if (!::GetExitCodeProcess(sandbox_process_, &exit_code) ||
-      (STILL_ACTIVE == exit_code)) {
+  ::WaitForSingleObject(sandbox_process_info_.process_handle(), 50);
+  if (!::GetExitCodeProcess(sandbox_process_info_.process_handle(),
+                            &exit_code) || (STILL_ACTIVE == exit_code)) {
     // It is an error to destroy this object while the target process is still
     // alive because we need to destroy the IPC subsystem and cannot risk to
     // have an IPC reach us after this point.
     return;
   }
 
-  delete ipc_server_;
-
-  ::CloseHandle(lockdown_token_);
-  ::CloseHandle(initial_token_);
-  ::CloseHandle(sandbox_process_);
-  if (shared_section_)
-    ::CloseHandle(shared_section_);
-  free(exe_name_);
+  // ipc_server_ references our process handle, so make sure the former is shut
+  // down before the latter is closed (by ScopedProcessInformation).
+  ipc_server_.reset();
 }
 
 // Creates the target (child) process suspended and assigns it to the job
@@ -141,8 +124,8 @@ TargetProcess::~TargetProcess() {
 DWORD TargetProcess::Create(const wchar_t* exe_path,
                             const wchar_t* command_line,
                             const wchar_t* desktop,
-                            PROCESS_INFORMATION* target_info) {
-  exe_name_ = _wcsdup(exe_path);
+                            base::win::ScopedProcessInformation* target_info) {
+  exe_name_.reset(_wcsdup(exe_path));
 
   // the command line needs to be writable by CreateProcess().
   scoped_ptr_malloc<wchar_t> cmd_line(_wcsdup(command_line));
@@ -157,7 +140,7 @@ DWORD TargetProcess::Create(const wchar_t* exe_path,
     startup_info.lpDesktop = desktop_name.get();
   }
 
-  PROCESS_INFORMATION process_info = {0};
+  base::win::ScopedProcessInformation process_info;
 
   if (!::CreateProcessAsUserW(lockdown_token_,
                               exe_path,
@@ -169,44 +152,43 @@ DWORD TargetProcess::Create(const wchar_t* exe_path,
                               NULL,   // Use the environment of the caller.
                               NULL,   // Use current directory of the caller.
                               &startup_info,
-                              &process_info)) {
+                              process_info.Receive())) {
     return ::GetLastError();
   }
 
-  PoisonLowerAddressRange(process_info.hProcess);
+  PoisonLowerAddressRange(process_info.process_handle());
 
   DWORD win_result = ERROR_SUCCESS;
 
   // Assign the suspended target to the windows job object
-  if (!::AssignProcessToJobObject(job_, process_info.hProcess)) {
+  if (!::AssignProcessToJobObject(job_, process_info.process_handle())) {
     win_result = ::GetLastError();
     // It might be a security breach if we let the target run outside the job
     // so kill it before it causes damage
-    TerminateTarget(&process_info);
+    ::TerminateProcess(process_info.process_handle(), 0);
     return win_result;
   }
 
   // Change the token of the main thread of the new process for the
   // impersonation token with more rights. This allows the target to start;
   // otherwise it will crash too early for us to help.
-  if (!SetThreadToken(&process_info.hThread, initial_token_)) {
-    win_result = ::GetLastError();
-    TerminateTarget(&process_info);
-    return win_result;
+  {
+    HANDLE temp_thread = process_info.thread_handle();
+    if (!::SetThreadToken(&temp_thread, initial_token_)) {
+      win_result = ::GetLastError();
+      ::TerminateProcess(process_info.process_handle(), 0);
+      return win_result;
+    }
   }
 
   CONTEXT context;
   context.ContextFlags = CONTEXT_ALL;
-  if (!::GetThreadContext(process_info.hThread, &context)) {
+  if (!::GetThreadContext(process_info.thread_handle(), &context)) {
     win_result = ::GetLastError();
-    TerminateTarget(&process_info);
+    ::TerminateProcess(process_info.process_handle(), 0);
     return win_result;
   }
 
-  sandbox_process_ = process_info.hProcess;
-  sandbox_thread_ = process_info.hThread;
-  sandbox_process_id_ = process_info.dwProcessId;
-
 #if defined(_WIN64)
   void* entry_point = reinterpret_cast<void*>(context.Rcx);
 #else
@@ -216,20 +198,26 @@ DWORD TargetProcess::Create(const wchar_t* exe_path,
   void* entry_point = reinterpret_cast<void*>(context.Eax);
 #pragma warning(pop)
 #endif  // _WIN64
+
+  if (!target_info->DuplicateFrom(process_info)) {

[erikwright (departed), 2012/03/30 16:29:31]

Previously the caller (broker_services.cc) duplicated the process handle, but
not the thread handle.

content/common/sandbox_policy.cc assumed that it owned the thread handle and
closed it.

So implicitly, this method was returning a PROCESS_INFORMATION, transferring
ownership of hThread but not of hProcess.

Furthermore, in Init, hThread is referenced, so you have to be sure that Init is
called before the Close in sandbox_policy.

While it might not be strictly necessary to duplicate both hProcess and hThread,
it seems to remove some ambiguity.

+    ::TerminateProcess(process_info.process_handle(), 0);
+    return ERROR_INVALID_FUNCTION;
+  }
+
   base_address_ = GetBaseAddress(exe_path, entry_point);
-  *target_info = process_info;
+  sandbox_process_info_.Swap(&process_info);
   return win_result;
 }
 
 ResultCode TargetProcess::TransferVariable(char* name, void* address,
                                            size_t size) {
-  if (NULL == sandbox_process_)
+  if (!sandbox_process_info_.IsValid())
     return SBOX_ERROR_UNEXPECTED_CALL;
 
   void* child_var = address;
 
 #if SANDBOX_EXPORTS
-  HMODULE module = ::LoadLibrary(exe_name_);
+  HMODULE module = ::LoadLibrary(exe_name_.get());
   if (NULL == module)
     return SBOX_ERROR_GENERIC;
 
@@ -247,8 +235,8 @@ ResultCode TargetProcess::TransferVariable(char* name, void* address,
 #endif
 
   SIZE_T written;
-  if (!::WriteProcessMemory(sandbox_process_, child_var, address, size,
-                            &written))
+  if (!::WriteProcessMemory(sandbox_process_info_.process_handle(),
+                            child_var, address, size, &written))
     return SBOX_ERROR_GENERIC;
 
   if (written != size)
@@ -270,18 +258,18 @@ DWORD TargetProcess::Init(Dispatcher* ipc_dispatcher, void* policy,
   // We use this single memory pool for IPC and for policy.
   DWORD shared_mem_size = static_cast<DWORD>(shared_IPC_size +
                                              shared_policy_size);
-  shared_section_ = ::CreateFileMappingW(INVALID_HANDLE_VALUE, NULL,
-                                         PAGE_READWRITE | SEC_COMMIT,
-                                         0, shared_mem_size, NULL);
-  if (NULL == shared_section_) {
+  shared_section_.Set(::CreateFileMappingW(INVALID_HANDLE_VALUE, NULL,
+                                           PAGE_READWRITE | SEC_COMMIT,
+                                           0, shared_mem_size, NULL));
+  if (!shared_section_.IsValid()) {
     return ::GetLastError();
   }
 
   DWORD access = FILE_MAP_READ | FILE_MAP_WRITE;
   HANDLE target_shared_section = NULL;
   if (!::DuplicateHandle(::GetCurrentProcess(), shared_section_,
-                         sandbox_process_, &target_shared_section,
-                         access, FALSE, 0)) {
+                         sandbox_process_info_.process_handle(),
+                         &target_shared_section, access, FALSE, 0)) {
     return ::GetLastError();
   }
 
@@ -322,29 +310,31 @@ DWORD TargetProcess::Init(Dispatcher* ipc_dispatcher, void* policy,
            ::GetLastError() : ERROR_INVALID_FUNCTION;
   }
 
-  ipc_server_ = new SharedMemIPCServer(sandbox_process_, sandbox_process_id_,
-                                       job_, thread_pool_, ipc_dispatcher);
+  ipc_server_.reset(
+      new SharedMemIPCServer(sandbox_process_info_.process_handle(),
+                             sandbox_process_info_.process_id(),
+                             job_, thread_pool_, ipc_dispatcher));
 
   if (!ipc_server_->Init(shared_memory, shared_IPC_size, kIPCChannelSize))
     return ERROR_NOT_ENOUGH_MEMORY;
 
   // After this point we cannot use this handle anymore.

[erikwright (departed), 2012/03/30 16:29:31]

I assume that this referred to the fact that sandbox_policy.cc might close the
HANDLE. Since it's now duplicated, is it actually meaningful to close it here,
or should we just let that happen in the destructor?

-  sandbox_thread_ = NULL;
+  ::CloseHandle(sandbox_process_info_.TakeThreadHandle());
 
   return ERROR_SUCCESS;
 }
 
 void TargetProcess::Terminate() {
-  if (NULL == sandbox_process_)
+  if (!sandbox_process_info_.IsValid())
     return;
 
-  ::TerminateProcess(sandbox_process_, 0);
+  ::TerminateProcess(sandbox_process_info_.process_handle(), 0);
 }
 
 
 TargetProcess* MakeTestTargetProcess(HANDLE process, HMODULE base_address) {
   TargetProcess* target = new TargetProcess(NULL, NULL, NULL, NULL);
-  target->sandbox_process_ = process;
+  target->sandbox_process_info_.Receive()->hProcess = process;
   target->base_address_ = base_address;
   return target;
 }