Chromoting service now launches the host process in the session the is currently attached to the ph…

remoting/host/wts_session_process_launcher_win.cc

Index: remoting/host/wts_session_process_launcher_win.cc
diff --git a/remoting/host/wts_session_process_launcher_win.cc b/remoting/host/wts_session_process_launcher_win.cc
index 119b564f553494610d7376f3453e61ec6e65c8bb..e14159bfaa7540c58ec86c163eebf754af5c2760 100644
--- a/remoting/host/wts_session_process_launcher_win.cc
+++ b/remoting/host/wts_session_process_launcher_win.cc
@@ -10,26 +10,299 @@
 #include <windows.h>
 
 #include "base/logging.h"
+#include "base/utf_string_conversions.h"
+#include "base/win/scoped_handle.h"
 
 #include "remoting/host/wts_console_monitor_win.h"
 
+using base::win::ScopedHandle;
+using base::TimeDelta;
+
+namespace {
+
+// The minimum and maximum delays between attempts to inject host process into
+// a session.
+const int kMaxLaunchDelaySeconds = 60;
+const int kMinLaunchDelaySeconds = 1;
+
+// Name of the default session desktop.
+const char kDefaultDesktopName[] = "winsta0\\default";
+
+// Takes the process token and makes a copy of it. The returned handle will have
+// |desired_access| rights.
+bool CopyProcessToken(DWORD desired_access,
+                      scoped_ptr<ScopedHandle>* token_out) {

[Wez, 2012/02/28 00:22:07]

Why does this need wrapping in a scoped_ptr<>?

[alexeypa (please no reviews), 2012/02/28 01:14:04]

ScopedHandle does not provide either copy or ownership semantic. So scoped_ptr
provides the latter. 

I agree it makes code a little bit messier. I can give up the strict ownership
semantic and use naked ScopedHandle instead. It is just that I'll have to be
more careful. So you think it worth a shot?

+
+  HANDLE handle;
+  if (!OpenProcessToken(GetCurrentProcess(),
+                        TOKEN_DUPLICATE | desired_access,
+                        &handle)) {
+    LOG_GETLASTERROR(ERROR) << "Failed to open process token";
+    return false;
+  }
+
+  scoped_ptr<ScopedHandle> process_token;
+  process_token.reset(new base::win::ScopedHandle(handle));

[Wez, 2012/02/28 00:22:07]

This could become:

ScopedHandle process_token(handle);

surely?

[alexeypa (please no reviews), 2012/02/28 01:14:04]

Yes.

+
+  if (!DuplicateTokenEx(*process_token,
+                        desired_access,
+                        NULL,
+                        SecurityImpersonation,
+                        TokenPrimary,
+                        &handle)) {
+    LOG_GETLASTERROR(ERROR) << "Failed to duplicate the process token";
+    return false;
+  }
+
+  token_out->reset(new ScopedHandle(handle));

[Wez, 2012/02/28 00:22:07]

And this would become:

token_out->Set(process_token.Take());

[alexeypa (please no reviews), 2012/02/28 01:14:04]

Yes.

+  return true;
+}
+
+// Creates a copy of the current process with SE_TCB_NAME privilege enabled.
+bool CreatePrivilegedToken(scoped_ptr<ScopedHandle>* token_out) {

[Wez, 2012/02/28 00:22:07]

Similarly, why wrap in a scoped_ptr<>?

[alexeypa (please no reviews), 2012/02/28 01:14:04]

Again, to underline ownership.

+  scoped_ptr<ScopedHandle> privileged_token;
+  DWORD desired_access = TOKEN_ADJUST_PRIVILEGES | TOKEN_IMPERSONATE |
+                         TOKEN_DUPLICATE | TOKEN_QUERY;
+  if (!CopyProcessToken(desired_access, &privileged_token)) {
+    return false;
+  }
+
+  // Get the LUID for the SE_TCB_NAME privilege.
+  TOKEN_PRIVILEGES state;
+  state.PrivilegeCount = 1;
+  state.Privileges[0].Attributes = SE_PRIVILEGE_ENABLED;
+  if (!LookupPrivilegeValue(NULL, SE_TCB_NAME, &state.Privileges[0].Luid)) {
+    LOG_GETLASTERROR(ERROR) <<
+        "Failed to lookup the LUID for the SE_TCB_NAME privilege";
+    return false;
+  }
+
+  // Enable the SE_TCB_NAME privilege.
+  if (!AdjustTokenPrivileges(*privileged_token, FALSE, &state, 0, NULL, 0)) {
+    LOG_GETLASTERROR(ERROR) <<
+        "Failed to enable SE_TCB_NAME privilege in a token";
+    return false;
+  }
+
+  *token_out = privileged_token.Pass();
+  return true;
+}
+
+// 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,
+                        scoped_ptr<ScopedHandle>* token_out) {
+
+  scoped_ptr<ScopedHandle> session_token;
+  DWORD desired_access = TOKEN_ADJUST_DEFAULT | TOKEN_ADJUST_SESSIONID |
+                         TOKEN_ASSIGN_PRIMARY | TOKEN_QUERY;
+  if (!CopyProcessToken(desired_access, &session_token)) {
+    return false;
+  }
+
+  // Change the session ID of the token.
+  DWORD new_session_id = session_id;
+  if (!SetTokenInformation(session_token->Get(),
+                           TokenSessionId,
+                           &new_session_id,
+                           sizeof(new_session_id))) {
+    LOG_GETLASTERROR(ERROR) <<
+        "Failed to change session ID of a token";
+    return false;
+  }
+
+  *token_out = session_token.Pass();
+  return true;
+}
+
+// Launches |binary| in the security context of the user represented by

[Wez, 2012/02/28 00:22:07]

nit: ... of the supplied |user_token|.

[alexeypa (please no reviews), 2012/02/28 01:14:04]

Done.

+// |user_token|.
+bool LaunchProcessAsUser(const FilePath& binary,
+                         const scoped_ptr<ScopedHandle>& user_token,
+                         base::Process* process_out) {

[Wez, 2012/02/28 00:22:07]

Why not return a scoped_ptr<base::Process>, rather than having to have an
out-parameter?

[alexeypa (please no reviews), 2012/02/28 01:14:04]

This makes the caller's code look like all other code that calls Win32 APIs. So
it looks more uniform.

But, I could wrap base::Process into scoped_ptr to make the point about the
ownership. This however should done only if ScopedHandle instances above remain
wrapped in scoped_ptr. So I'll wait changing the code until we settle on
something.

+  string16 command_line = binary.value();
+  string16 desktop = ASCIIToUTF16(kDefaultDesktopName);
+
+  PROCESS_INFORMATION process_info;
+  STARTUPINFOW startup_info;
+
+  memset(&startup_info, 0, sizeof(startup_info));
+  startup_info.cb = sizeof(startup_info);
+  startup_info.lpDesktop = const_cast<LPWSTR>(desktop.c_str());
+
+  if (!CreateProcessAsUserW(*user_token,
+                            command_line.c_str(),
+                            const_cast<LPWSTR>(command_line.c_str()),
+                            NULL,
+                            NULL,
+                            FALSE,
+                            0,
+                            NULL,
+                            NULL,
+                            &startup_info,
+                            &process_info)) {
+    LOG_GETLASTERROR(ERROR) <<
+        "Failed to launch a process with a user token";
+    return false;
+  }
+
+  CloseHandle(process_info.hThread);
+  process_out->set_handle(process_info.hProcess);
+  return true;
+}
+
+} // namespace
+
 namespace remoting {
 
 WtsSessionProcessLauncher::WtsSessionProcessLauncher(
-    WtsConsoleMonitor* monitor) : monitor_(monitor) {
+    WtsConsoleMonitor* monitor,
+    const FilePath& host_binary)
+    : host_binary_(host_binary),
+      monitor_(monitor),
+      state_(StateDetached) {
   monitor_->AddWtsConsoleObserver(this);
 }
 
 WtsSessionProcessLauncher::~WtsSessionProcessLauncher() {
+  DCHECK(state_ == StateDetached);
+  DCHECK(!timer_.IsRunning());
+  DCHECK(process_.handle() == NULL);
+  DCHECK(process_watcher_.GetWatchedObject() == NULL);
+
   monitor_->RemoveWtsConsoleObserver(this);
 }
 
+void WtsSessionProcessLauncher::LaunchProcess() {
+  DCHECK(state_ == StateStarting);
+  DCHECK(!timer_.IsRunning());
+  DCHECK(process_.handle() == NULL);
+  DCHECK(process_watcher_.GetWatchedObject() == NULL);
+
+  // Try to launch the process and attach an object watcher to the returned
+  // handle so that we get notified when the process terminates.
+  launch_time_ = base::Time::Now();
+  if (LaunchProcessAsUser(host_binary_, session_token_, &process_)) {
+    if (process_watcher_.StartWatching(process_.handle(), this)) {
+      state_ = StateAttached;
+      return;
+    } else {

[Wez, 2012/02/28 00:22:07]

nit: Is there anything meaningful to log in this case, or does StartWatching()
log something on failure?

[alexeypa (please no reviews), 2012/02/28 01:14:04]

StartWatching() will print details to the log. I think it is still worth logging
this error from our code.

+      process_.Terminate(0);
+      process_.Close();
+    }
+  }
+
+  // Something went wrong. Try to launch the host again later. The attempts rate
+  // is limited by exponential backoff.
+  launch_backoff_ = std::max(launch_backoff_ * 2,
+                             TimeDelta::FromSeconds(kMinLaunchDelaySeconds));
+  launch_backoff_ = std::min(launch_backoff_,
+                             TimeDelta::FromSeconds(kMaxLaunchDelaySeconds));
+  timer_.Start(FROM_HERE, launch_backoff_,
+               this, &WtsSessionProcessLauncher::LaunchProcess);
+}
+
+void WtsSessionProcessLauncher::OnObjectSignaled(HANDLE object) {
+  DCHECK(state_ == StateAttached);
+  DCHECK(!timer_.IsRunning());
+  DCHECK(process_.handle() != NULL);
+  DCHECK(process_watcher_.GetWatchedObject() == NULL);
+
+  // The host process has been terminated for some reason. The handle can now be
+  // closed.
+  process_.Close();
+
+  // Expand the backoff interval if the process has died quickly or reset it if
+  // it was up longer than the maximum backoff delay.
+  base::TimeDelta delta = base::Time::Now() - launch_time_;
+  if (delta < base::TimeDelta() ||
+      delta >= base::TimeDelta::FromSeconds(kMaxLaunchDelaySeconds)) {
+    launch_backoff_ = base::TimeDelta();
+  } else {
+    launch_backoff_ = std::max(launch_backoff_ * 2,
+                               TimeDelta::FromSeconds(kMinLaunchDelaySeconds));
+    launch_backoff_ = std::min(launch_backoff_,
+                               TimeDelta::FromSeconds(kMaxLaunchDelaySeconds));
+  }
+
+  // Try to restart the host.
+  state_ = StateStarting;
+  timer_.Start(FROM_HERE, launch_backoff_,
+               this, &WtsSessionProcessLauncher::LaunchProcess);
+}
+
 void WtsSessionProcessLauncher::OnSessionAttached(uint32 session_id) {
-  // TODO(alexeypa): The code injecting the host process to a session goes here.
+  DCHECK(state_ == StateDetached);

[Wez, 2012/02/28 00:22:07]

This should probably be a CHECK, since OnSessionAttached is called based on
notifications from the OS and so whether it gets called in the way we expect is
not 100% in our control.

[alexeypa (please no reviews), 2012/02/28 01:14:04]

It is actually in our control. WtsConsoleObserver's contract  is that Attached
and Detached calls are properly ordered. We actually make sure that this happens
even if the OS does not give us proper ordering.

+  DCHECK(!timer_.IsRunning());
+  DCHECK(process_.handle() == NULL);
+  DCHECK(process_watcher_.GetWatchedObject() == NULL);
+
+  // Temporary enable the SE_TCB_NAME priviledge by impersonating a privileged

[Wez, 2012/02/28 00:22:07]

typo: Temporarily
typo: privilege

[Wez, 2012/02/28 00:22:07]

Suggest rewording: ... by impersonating a copy of our process token with the
privilege enabled.

[alexeypa (please no reviews), 2012/02/28 01:14:04]

Done.

[alexeypa (please no reviews), 2012/02/28 01:14:04]

Done.

+  // token. The privileged token is created as needed and kept for later reuse.
+  if (privileged_token_.get() == NULL) {
+    if (!CreatePrivilegedToken(&privileged_token_)) {
+      return;
+    }
+  }
+
+  if (!ImpersonateLoggedOnUser(*privileged_token_)) {
+    LOG_GETLASTERROR(ERROR) <<
+        "Failed to impersonate the privileged token";
+    return;
+  }
+
+  // While the SE_TCB_NAME progolege is enabled, create a session token for
+  // the launched process.
+  BOOL result = CreateSessionToken(session_id, &session_token_);

[Wez, 2012/02/28 00:22:07]

nit: CreateSessionToken returns bool, not BOOL. :)

[alexeypa (please no reviews), 2012/02/28 01:14:04]

Indeed. :-)

+
+  // Revert to the default token. The default token is sufficient to call
+  // CreateProcessAsUser() successfully.
+  if (!RevertToSelf()) {

[Wez, 2012/02/28 00:22:07]

The process should crash if this fails.

[alexeypa (please no reviews), 2012/02/28 01:14:04]

Done.

+    LOG_GETLASTERROR(ERROR) <<
+        "Failed to revert to the default process token";
+  }
+
+  if (!result)
+    return;
+
+  // Now try to launch the host.
+  state_ = StateStarting;
+  LaunchProcess();
 }
 
 void WtsSessionProcessLauncher::OnSessionDetached() {
-  // TODO(alexeypa): The code terminating the host process goes here.
+  DCHECK(state_ == StateDetached ||
+         state_ == StateStarting ||
+         state_ == StateAttached);
+
+  switch (state_) {
+    case StateDetached:

[Wez, 2012/02/28 00:22:07]

Why does OnSessionDetached cope with being called when already detached, but
OnSessionAttached DCHECK in the equivalent situation?

[alexeypa (please no reviews), 2012/02/28 01:14:04]

Because the caller makes sure that ordering in right. Token manipulations can
fail, so we can stay in StateDetached. The reason we don't retry if any of the
token operations fail is that it is very likely to be a permanent failure, so
there is not point to keep trying.

+      DCHECK(!timer_.IsRunning());
+      DCHECK(process_.handle() == NULL);
+      DCHECK(process_watcher_.GetWatchedObject() == NULL);
+      break;
+
+    case StateStarting:
+      DCHECK(timer_.IsRunning());
+      DCHECK(process_.handle() == NULL);
+      DCHECK(process_watcher_.GetWatchedObject() == NULL);
+
+      timer_.Stop();
+      launch_backoff_ = base::TimeDelta();
+      state_ = StateDetached;
+      break;
+
+    case StateAttached:
+      DCHECK(!timer_.IsRunning());
+      DCHECK(process_.handle() != NULL);
+      DCHECK(process_watcher_.GetWatchedObject() != NULL);
+
+      process_watcher_.StopWatching();
+      process_.Terminate(0);
+      process_.Close();
+      state_ = StateDetached;
+      break;
+  }
 }
 
 } // namespace remoting