[Chromoting] Adding uiAccess='true' to the remoting_me2me_host.exe's manifest as it is required to …

remoting/host/win/wts_session_process_launcher.cc

Index: remoting/host/win/wts_session_process_launcher.cc
diff --git a/remoting/host/win/wts_session_process_launcher.cc b/remoting/host/win/wts_session_process_launcher.cc
index e57d37c93e03cd573d8db8ddb0e28543c9353256..f6d7be62849ed7c288bc421ec020c276bda05976 100644
--- a/remoting/host/win/wts_session_process_launcher.cc
+++ b/remoting/host/win/wts_session_process_launcher.cc
@@ -47,9 +47,14 @@ const int kMinLaunchDelaySeconds = 1;
 const FilePath::CharType kMe2meHostBinaryName[] =
     FILE_PATH_LITERAL("remoting_me2me_host.exe");
 
+const FilePath::CharType kMe2meServiceBinaryName[] =
+    FILE_PATH_LITERAL("remoting_service.exe");
+
 // The IPC channel name is passed to the host in the command line.
 const char kChromotingIpcSwitchName[] = "chromoting-ipc";
 
+const char kElevateSwitchName[] = "elevate";
+
 // The command line parameters that should be copied from the service's command
 // line to the host process.
 const char* kCopiedSwitchNames[] = {
@@ -72,8 +77,17 @@ WtsSessionProcessLauncher::WtsSessionProcessLauncher(
       attached_(false),
       main_message_loop_(main_message_loop),
       ipc_message_loop_(ipc_message_loop),
-      monitor_(monitor) {
+      monitor_(monitor),
+      job_state_(kJobUninitialized) {
   monitor_->AddWtsConsoleObserver(this);
+
+  process_exit_event_.Set(CreateEvent(NULL, TRUE, FALSE, NULL));
+  CHECK(process_exit_event_.IsValid());
+
+  // Job object initalization has to be performed on the I/O thread.

[Wez, 2012/08/14 01:08:59]

nit: Why? :) e.g. because an I/O thread is needed to handle job notifications
via a completion port?

[alexeypa (please no reviews), 2012/08/14 05:43:22]

Done.

+  ipc_message_loop_->PostTask(FROM_HERE, base::Bind(
+      &WtsSessionProcessLauncher::InitializeJob,
+      base::Unretained(this)));
 }
 
 WtsSessionProcessLauncher::~WtsSessionProcessLauncher() {
@@ -103,19 +117,22 @@ void WtsSessionProcessLauncher::LaunchProcess() {
 void WtsSessionProcessLauncher::OnLauncherStopped() {
   DCHECK(main_message_loop_->BelongsToCurrentThread());
 
-  DWORD exit_code;
-  if (!::GetExitCodeProcess(worker_process_, &exit_code)) {
-    LOG_GETLASTERROR(INFO)
-        << "Failed to query the exit code of the worker process";
-    exit_code = CONTROL_C_EXIT;
+  DWORD exit_code = CONTROL_C_EXIT;
+  if (worker_process_.IsValid()) {
+    if (!::GetExitCodeProcess(worker_process_, &exit_code)) {
+      LOG_GETLASTERROR(INFO)
+          << "Failed to query the exit code of the worker process";
+      exit_code = CONTROL_C_EXIT;
+    }
+
+    worker_process_.Close();
   }
 
   launcher_.reset(NULL);
-  worker_process_.Close();
 
   // Do not relaunch the worker process if the caller has asked us to stop.
   if (stoppable_state() != Stoppable::kRunning) {
-    CompleteStopping();
+    Stop();

[Wez, 2012/08/14 01:08:59]

I'm confused; isn't Stop() what the caller called to get us here?

[alexeypa (please no reviews), 2012/08/14 05:43:22]

Yes, but it does not mean that Stop() is still on the stack. Stop() could post
tasks that eventually resulted in OnLauncherStopped() to be called. At this
point we need to call Stop() again to resume shutting down from where we left
it.

     return;
   }
 
@@ -148,12 +165,34 @@ void WtsSessionProcessLauncher::OnLauncherStopped() {
   }
 }
 
+void WtsSessionProcessLauncher::OnIOCompleted(
+    base::MessagePumpForIO::IOContext* context,
+    DWORD bytes_transfered,

[Wez, 2012/08/14 01:08:59]

typo: bytes_transferred

[alexeypa (please no reviews), 2012/08/14 05:43:22]

Done.

+    DWORD error) {
+  DCHECK(ipc_message_loop_->BelongsToCurrentThread());
+
+  // |bytes_transfered| is repurposed by the job object notifications to be
+  // the message ID.

[Wez, 2012/08/14 01:08:59]

nit: Suggest "... used in job notifications to supply the message ID."

[alexeypa (please no reviews), 2012/08/14 05:43:22]

Done.

+  BOOL result;
+  switch (bytes_transfered) {
+    case JOB_OBJECT_MSG_ACTIVE_PROCESS_ZERO:
+      result = SetEvent(process_exit_event_);
+      CHECK(result);
+      break;
+  }
+}
+
 bool WtsSessionProcessLauncher::DoLaunchProcess(
     const std::string& channel_name,
     ScopedHandle* process_exit_event_out) {
   DCHECK(main_message_loop_->BelongsToCurrentThread());
   DCHECK(!worker_process_.IsValid());
 
+  // Wait until the sandbox is ready.

[Wez, 2012/08/14 01:08:59]

nit: You mean that we can't launch until the sandbox is ready? Will we re-try
the operation once it's ready?

[alexeypa (please no reviews), 2012/08/14 05:43:22]

Yes.

+  if (!job_.IsValid()) {
+    return false;
+  }
+
   // Construct the host binary name.
   FilePath dir_path;
   if (!PathService::Get(base::DIR_EXE, &dir_path)) {
@@ -161,28 +200,49 @@ bool WtsSessionProcessLauncher::DoLaunchProcess(
     return false;
   }
   FilePath host_binary = dir_path.Append(kMe2meHostBinaryName);
+  FilePath service_binary = dir_path.Append(kMe2meServiceBinaryName);
 
   // Create the host process command line passing the name of the IPC channel
   // to use and copying known switches from the service's command line.
-  CommandLine command_line(host_binary);
+  CommandLine command_line(service_binary);
+  command_line.AppendSwitchPath(kElevateSwitchName, host_binary);
   command_line.AppendSwitchNative(kChromotingIpcSwitchName,
                                   UTF8ToWide(channel_name));
   command_line.CopySwitchesFrom(*CommandLine::ForCurrentProcess(),
                                 kCopiedSwitchNames,
                                 _countof(kCopiedSwitchNames));
 
+  BOOL result = ResetEvent(process_exit_event_);
+  CHECK(result);

[Wez, 2012/08/14 01:08:59]

nit: Do you need to create a local variable for this rather than
CHECK(ResetEvent)?

[alexeypa (please no reviews), 2012/08/14 05:43:22]

Done.

+
   // Try to launch the process and attach an object watcher to the returned
   // handle so that we get notified when the process terminates.
-  if (!LaunchProcessWithToken(host_binary,
+  base::win::ScopedHandle worker_process;
+  base::win::ScopedHandle worker_thread;
+  if (!LaunchProcessWithToken(service_binary,
                               command_line.GetCommandLineString(),
                               session_token_,
-                              &worker_process_)) {
+                              CREATE_SUSPENDED,
+                              &worker_process,
+                              &worker_thread)) {
+    return false;
+  }
+
+  if (!AssignProcessToJobObject(job_, worker_process)) {
+    LOG_GETLASTERROR(ERROR) << "Failed to assign the worker to the job object";
+    TerminateProcess(worker_process, CONTROL_C_EXIT);

[Wez, 2012/08/14 01:08:59]

nit: ::TerminateProcess()?

[alexeypa (please no reviews), 2012/08/14 05:43:22]

'::' is not necessary, less common in Chromium code and frowned at by many
reviewer. 

+    return false;
+  }
+
+  if (!ResumeThread(worker_thread)) {
+    LOG_GETLASTERROR(ERROR) << "Failed to resume the worker thread";
+    DoKillProcess(CONTROL_C_EXIT);
     return false;
   }
 
   ScopedHandle process_exit_event;
   if (!DuplicateHandle(GetCurrentProcess(),
-                       worker_process_,
+                       process_exit_event_,
                        GetCurrentProcess(),
                        process_exit_event.Receive(),
                        SYNCHRONIZE,
@@ -200,21 +260,21 @@ bool WtsSessionProcessLauncher::DoLaunchProcess(
 void WtsSessionProcessLauncher::DoKillProcess(DWORD exit_code) {
   DCHECK(main_message_loop_->BelongsToCurrentThread());
 
-  if (worker_process_.IsValid()) {
-    TerminateProcess(worker_process_, exit_code);
+  if (job_.IsValid()) {
+    TerminateJobObject(job_, exit_code);
   }
 }
 
 void WtsSessionProcessLauncher::OnChannelConnected(DWORD peer_pid) {
   DCHECK(main_message_loop_->BelongsToCurrentThread());
 
-  DWORD expected_pid = GetProcessId(worker_process_);
-  if (peer_pid != expected_pid) {
-    LOG(ERROR)
-        << "Unexpected client connected: expected=" << expected_pid
-        << ", actual=" << peer_pid;
-    Stop();
-  }
+  // Nothing to validate. Only processes running as SYSTEM can connect to
+  // the pipe, so if it was a malicious process the game would be over already.
+
+  // Get a handle to the peer process so we could query its exit code later.
+  // Ignore the error. If the processes cannot be open the error code is generic

[Wez, 2012/08/14 01:08:59]

typo: processes

[alexeypa (please no reviews), 2012/08/14 05:43:22]

Done.

+  // |CONTROL_C_EXIT|.
+  worker_process_.Set(OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, peer_pid));
 }
 
 bool WtsSessionProcessLauncher::OnMessageReceived(const IPC::Message& message) {
@@ -284,8 +344,104 @@ void WtsSessionProcessLauncher::DoStop() {
     OnSessionDetached();
   }
 
-  if (launcher_.get() == NULL) {
-    CompleteStopping();
+  job_.Close();
+
+  // Drain the completion queue to make sure all job object notification have
+  // been received.
+  if (job_state_ > kJobUninitialized) {

[Wez, 2012/08/14 01:08:59]

Ick. Do you really need to use more-than here?

[alexeypa (please no reviews), 2012/08/14 05:43:22]

No, that is wrong. It should list specific states.

+    DCHECK_EQ(job_state_, kJobRunning);
+    job_state_ = kJobStopping;
+    ipc_message_loop_->PostTask(FROM_HERE, base::Bind(
+        &WtsSessionProcessLauncher::DrainJobNotifications,
+        base::Unretained(this)));
+  }
+
+  // Wait for |launcher_| to be completely stopped.

[Wez, 2012/08/14 01:08:59]

nit: You're not waiting for anything here; you mean don't complete shutdown if
the |launcher_| isn't stopped yet?

[alexeypa (please no reviews), 2012/08/14 05:43:22]

Done.

+  if (launcher_.get() != NULL) {
+    return;
+  }
+
+  // Wait for the completion queue to be drained.

[Wez, 2012/08/14 01:08:59]

nit: Similarly, you mean don't complete shutdown until the completion queue is
drained?

[alexeypa (please no reviews), 2012/08/14 05:43:22]

Done.

+  if (job_state_ != kJobUninitialized && job_state_ != kJobStopped) {
+    return;
+  }
+
+  CompleteStopping();
+}
+
+void WtsSessionProcessLauncher::DrainJobNotifications() {

[Wez, 2012/08/14 01:08:59]

nit: This method just posts a notification back to the calling thread; it
doesn't actually drain anything. Tricky to think of a suitably descriptive name.
Perhaps OnJobShutdownCompleteOnIpcThread?

[alexeypa (please no reviews), 2012/08/14 05:43:22]

|DrainJobNotifications| describes what happens, but does not document how.
|OnJobShutdownCompleteOnIpcThread| documents when something happens, but does
not say why and what. I think the former is a better name.

+  DCHECK(ipc_message_loop_->BelongsToCurrentThread());
+
+  // DrainJobNotifications() is posted after the job object is destroyed, so
+  // by this time all notifications from the job object have been processed
+  // already. Let the main thread know that the queue has been drained.

[Wez, 2012/08/14 01:08:59]

This assumes that the notifications are queued to the IPC thread before the
PostTask()? Is that guaranteed by Windows, or would it be legal for it to defer
those notifications in some situations?

[alexeypa (please no reviews), 2012/08/14 05:43:22]

Yes, no notifications can be posted after the job object has been destroyed.

+  main_message_loop_->PostTask(FROM_HERE, base::Bind(
+      &WtsSessionProcessLauncher::DrainJobNotificationsCompleted,
+      base::Unretained(this)));
+}
+
+void WtsSessionProcessLauncher::DrainJobNotificationsCompleted() {
+  DCHECK(main_message_loop_->BelongsToCurrentThread());
+  DCHECK_EQ(job_state_, kJobStopping);
+
+  job_state_ = kJobStopped;
+  Stop();
+}
+
+void WtsSessionProcessLauncher::InitializeJob() {
+  DCHECK(ipc_message_loop_->BelongsToCurrentThread());
+
+  ScopedHandle job;
+  job.Set(CreateJobObject(NULL, NULL));
+  if (!job.IsValid()) {
+    LOG_GETLASTERROR(ERROR) << "Failed to create a job object";
+    return;
+  }
+
+  // Limit number of active processes in the job to two (the process performing
+  // elevation and the host) and make sure that all processes will be killed
+  // once the job object is destroyed.
+  JOBOBJECT_EXTENDED_LIMIT_INFORMATION info;
+  memset(&info, 0, sizeof(info));
+  info.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_ACTIVE_PROCESS |
+      JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE;
+  info.BasicLimitInformation.ActiveProcessLimit = 2;
+  if (!SetInformationJobObject(job,
+                               JobObjectExtendedLimitInformation,
+                               &info,
+                               sizeof(info))) {
+    LOG_GETLASTERROR(ERROR) << "Failed to set limits on the job object";
+    return;
+  }
+
+  // Register the job object with the completion port in the I/O thread to
+  // receive job notifications.
+  if (!MessageLoopForIO::current()->RegisterJobObject(job, this)) {
+    LOG_GETLASTERROR(ERROR)
+        << "Failed to associate the job object with a completion port";
+    return;
+  }
+
+  // ScopedHandle is not compatible with base::Passed, so we wrap it to a scoped
+  // pointer.
+  scoped_ptr<ScopedHandle> job_wrapper(new ScopedHandle());
+  *job_wrapper = job.Pass();
+
+  // Let the main thread know that initialization is complete.
+  main_message_loop_->PostTask(FROM_HERE, base::Bind(
+      &WtsSessionProcessLauncher::InitializeJobCompleted,
+      base::Unretained(this), base::Passed(&job_wrapper)));
+}
+
+void WtsSessionProcessLauncher::InitializeJobCompleted(
+    scoped_ptr<ScopedHandle> job) {
+  DCHECK(main_message_loop_->BelongsToCurrentThread());
+  DCHECK(!job_.IsValid());
+  DCHECK_EQ(job_state_, kJobUninitialized);
+
+  if (stoppable_state() == Stoppable::kRunning) {
+    job_ = job->Pass();
+    job_state_ = kJobRunning;
   }
 }