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

sandbox/src/target_process.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 "sandbox/src/target_process.h"
 
 #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"
 #include "sandbox/src/sandbox_types.h"
 #include "sandbox/src/sharedmem_ipc_server.h"
 
 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;
   memcpy(dest, source, size);
   sandbox::PolicyGlobal* policy =
       reinterpret_cast<sandbox::PolicyGlobal*>(dest);
 
   size_t offset = reinterpret_cast<size_t>(source);
 
   for (size_t i = 0; i < sandbox::kMaxServiceCount; i++) {
@@ skipping 100 matching lines @@
   if (shared_section_)
     ::CloseHandle(shared_section_);
   free(exe_name_);
 }
 
 // Creates the target (child) process suspended and assigns it to the job
 // object.
 DWORD TargetProcess::Create(const wchar_t* exe_path,
                             const wchar_t* command_line,
                             const wchar_t* desktop,
                             PROCESS_INFORMATION* target_info) {

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

target_info can also be converted to ScopedProcessInformation*.

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

Extracted to a new CL, see
http://codereview.chromium.org/9959018/diff/2001/sandbox/src/target_process.c...

   exe_name_ = _wcsdup(exe_path);
 
   // the command line needs to be writable by CreateProcess().
   scoped_ptr_malloc<wchar_t> cmd_line(_wcsdup(command_line));
   scoped_ptr_malloc<wchar_t> desktop_name(desktop ? _wcsdup(desktop) : NULL);
 
   // Start the target process suspended.
   const DWORD flags = CREATE_SUSPENDED | CREATE_BREAKAWAY_FROM_JOB |
                       CREATE_UNICODE_ENVIRONMENT | DETACHED_PROCESS;
 
   STARTUPINFO startup_info = {sizeof(STARTUPINFO)};
   if (desktop) {
     startup_info.lpDesktop = desktop_name.get();
   }
 
-  PROCESS_INFORMATION process_info = {0};
+  base::win::ScopedProcessInformation process_info;
 
   if (!::CreateProcessAsUserW(lockdown_token_,
                               exe_path,
                               cmd_line.get(),
                               NULL,   // No security attribute.
                               NULL,   // No thread attribute.
                               FALSE,  // Do not inherit handles.
                               flags,
                               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);
 #else
 #pragma warning(push)
 #pragma warning(disable: 4312)
   // This cast generates a warning because it is 32 bit specific.
   void* entry_point = reinterpret_cast<void*>(context.Eax);
 #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;
 }
 
 ResultCode TargetProcess::TransferVariable(char* name, void* address,
                                            size_t size) {
   if (NULL == sandbox_process_)
     return SBOX_ERROR_UNEXPECTED_CALL;
 
   void* child_var = address;
 
@@ skipping 112 matching lines @@
 
 
 TargetProcess* MakeTestTargetProcess(HANDLE process, HMODULE base_address) {
   TargetProcess* target = new TargetProcess(NULL, NULL, NULL, NULL);
   target->sandbox_process_ = process;
   target->base_address_ = base_address;
   return target;
 }
 
 }  // namespace sandbox