ScopedProcessInformation protects against process/thread handle leaks from CreateProcess calls.

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..fc5f55f79eb6cb1ae7aeedf1d1e80f8f277bd60a 100644
--- a/sandbox/src/target_process.cc
+++ b/sandbox/src/target_process.cc
@@ -7,6 +7,7 @@
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/win/pe_image.h"
+#include "base/win/scoped_process_information.h"
 #include "sandbox/src/crosscall_server.h"
 #include "sandbox/src/crosscall_client.h"
 #include "sandbox/src/policy_low_level.h"
@@ -15,12 +16,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;
@@ -157,7 +152,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,43 +164,46 @@ 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;
+  sandbox_process_ = process_info.process_handle();
+  sandbox_thread_ = process_info.thread_handle();
+  sandbox_process_id_ = process_info.process_id();
 
 #if defined(_WIN64)
   void* entry_point = reinterpret_cast<void*>(context.Rcx);
@@ -217,7 +215,7 @@ DWORD TargetProcess::Create(const wchar_t* exe_path,
 #pragma warning(pop)
 #endif  // _WIN64
   base_address_ = GetBaseAddress(exe_path, entry_point);
-  *target_info = process_info;
+  *target_info = process_info.Take();

[alexeypa (please no reviews), 2012/03/29 04:51:35]

This is an example where Swap semantic could be clearer express what is
happening (assuming that target_info would also be ScopedProcessInformation).

[erikwright (departed), 2012/03/30 17:30:27]

See
http://codereview.chromium.org/9959018/diff/2001/sandbox/src/target_process.c...

   return win_result;
 }