Return the thread handle when launching a process in a different session via the execution server o…

remoting/host/win/launch_process_with_token.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 "remoting/host/win/launch_process_with_token.h"
 
 #include <windows.h>
 #include <winternl.h>
 
 #include "base/logging.h"
@@ skipping 188 matching lines @@
       (application_name.size() + command_line.size() + desktop_name.size() + 3);
   scoped_array<char> buffer(new char[size]);
   memset(buffer.get(), 0, size);
 
   // Marshal the input parameters.
   CreateProcessRequest* request =
       reinterpret_cast<CreateProcessRequest*>(buffer.get());
   request->size = size;
   request->process_id = GetCurrentProcessId();
   request->use_default_token = TRUE;
-  request->creation_flags = creation_flags;
+  // Always pass CREATE_SUSPENDED to avoid a race between the created process
+  // exition too soon and OpenProcess() call below.

[simonmorris, 2012/10/04 23:32:44]

exition -> exiting

[alexeypa (please no reviews), 2012/10/05 18:30:00]

Done.

+  request->creation_flags = creation_flags | CREATE_SUSPENDED;
   request->startup_info.cb = sizeof(request->startup_info);
 
   size_t buffer_offset = sizeof(CreateProcessRequest);
 
   request->application_name = reinterpret_cast<LPWSTR>(buffer_offset);
   std::copy(application_name.begin(),
             application_name.end(),
             reinterpret_cast<wchar_t*>(buffer.get() + buffer_offset));
   buffer_offset += (application_name.size() + 1) * sizeof(wchar_t);
 
@@ skipping 37 matching lines @@
     return false;
   }
 
   if (!response.success) {
     SetLastError(response.last_error);
     return false;
   }
 
   // The execution server does not return handles to the created process and
   // thread.
+  bool success = true;
   if (response.process_information.hProcess == NULL) {

[simonmorris, 2012/10/04 23:32:44]

Maybe change this to !response..., for consistency.

[alexeypa (please no reviews), 2012/10/05 18:30:00]

Done.

     // N.B. PROCESS_ALL_ACCESS is different in XP and Vista+ versions of
     // the SDK. |desired_access| below is effectively PROCESS_ALL_ACCESS from
     // the XP version of the SDK.
     DWORD desired_access =
         STANDARD_RIGHTS_REQUIRED |
         SYNCHRONIZE |
         PROCESS_TERMINATE |
         PROCESS_CREATE_THREAD |
         PROCESS_SET_SESSIONID |
         PROCESS_VM_OPERATION |
         PROCESS_VM_READ |
         PROCESS_VM_WRITE |
         PROCESS_DUP_HANDLE |
         PROCESS_CREATE_PROCESS |
         PROCESS_SET_QUOTA |
         PROCESS_SET_INFORMATION |
         PROCESS_QUERY_INFORMATION |
         PROCESS_SUSPEND_RESUME;
     response.process_information.hProcess =
         OpenProcess(desired_access,
                     FALSE,
                     response.process_information.dwProcessId);
     if (!response.process_information.hProcess) {
-      LOG_GETLASTERROR(ERROR) << "Failed to open process "
+      LOG_GETLASTERROR(ERROR) << "Failed to open the process "
                               << response.process_information.dwProcessId;
-      return false;
+      success = false;
     }
   }
 
-  *process_information_out = response.process_information;
-  return true;
+  if (success && response.process_information.hThread == NULL) {
+    // N.B. THREAD_ALL_ACCESS is different in XP and Vista+ versions of
+    // the SDK. |desired_access| below is effectively THREAD_ALL_ACCESS from
+    // the XP version of the SDK.
+    DWORD desired_access =
+        STANDARD_RIGHTS_REQUIRED |
+        SYNCHRONIZE |
+        THREAD_TERMINATE |
+        THREAD_SUSPEND_RESUME |
+        THREAD_GET_CONTEXT |
+        THREAD_SET_CONTEXT |
+        THREAD_QUERY_INFORMATION |
+        THREAD_SET_INFORMATION |
+        THREAD_SET_THREAD_TOKEN |
+        THREAD_IMPERSONATE |
+        THREAD_DIRECT_IMPERSONATION;
+    response.process_information.hThread =
+        OpenThread(desired_access,
+                   FALSE,
+                   response.process_information.dwThreadId);
+    if (!response.process_information.hThread) {
+      LOG_GETLASTERROR(ERROR) << "Failed to open the thread "
+                              << response.process_information.dwThreadId;
+      success = false;
+    }
+  }
+
+  // Resume the thread if the caller didn't want to suspend the process.
+  if (success && (creation_flags & CREATE_SUSPENDED) == 0) {
+    if (!ResumeThread(response.process_information.hThread)) {
+      LOG_GETLASTERROR(ERROR) << "Failed to resume the thread "
+                              << response.process_information.dwThreadId;
+      success = false;
+    }
+  }
+
+  if (success) {
+    *process_information_out = response.process_information;
+    return true;
+  } else {
+    if (response.process_information.hThread != NULL)
+      CloseHandle(response.process_information.hThread);
+
+    if (response.process_information.hProcess != NULL) {
+      TerminateProcess(response.process_information.hProcess, CONTROL_C_EXIT);
+      CloseHandle(response.process_information.hProcess);
+    }

[simonmorris, 2012/10/04 23:32:44]

Consider moving the preceding 7 lines to a separate method, and replacing each
"success = false;" by "CloseHandles(response); return false;". Then you don't
need |success|, and I think the logic is easier to follow.

[alexeypa (please no reviews), 2012/10/05 18:30:00]

Done. I've split CreateRemoteSessionProcess() into several functions. 

+    return false;
+  }
 }
 
 } // namespace
 
 namespace remoting {
 
 // Creates a copy of the current process token for the given |session_id| so
 // it can be used to launch a process in that session.
 bool CreateSessionToken(uint32 session_id, ScopedHandle* token_out) {
   ScopedHandle session_token;
@@ skipping 99 matching lines @@
     return false;
   }
 
   CHECK(process_info.IsValid());
   process_out->Set(process_info.TakeProcessHandle());
   thread_out->Set(process_info.TakeThreadHandle());
   return true;
 }
 
 } // namespace remoting