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

Index: remoting/host/win/launch_process_with_token.cc
diff --git a/remoting/host/win/launch_process_with_token.cc b/remoting/host/win/launch_process_with_token.cc
index 1320e2a3dd911fa932d1c3fafc93f62109e045f1..87b097d5dbf8c7a442f3e913ab5ae83f197b8130 100644
--- a/remoting/host/win/launch_process_with_token.cc
+++ b/remoting/host/win/launch_process_with_token.cc
@@ -206,7 +206,9 @@ bool CreateRemoteSessionProcess(
   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);
@@ -264,6 +266,7 @@ bool CreateRemoteSessionProcess(
 
   // 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
@@ -288,14 +291,61 @@ bool CreateRemoteSessionProcess(
                     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