From df9740a82a96fa627ee039fabcec20d823aabf95 Mon Sep 17 00:00:00 2001
From: Joe Downing <joedow@google.com>
Date: Fri, 21 Aug 2026 16:49:47 -0700
Subject: [PATCH] [remoting][win] Handle abnormal process exit in job notifications

WtsSessionProcessDelegate::Core::OnIOCompleted() only handled
JOB_OBJECT_MSG_EXIT_PROCESS. Windows delivers
JOB_OBJECT_MSG_ABNORMAL_EXIT_PROCESS instead when a process in the job
terminates due to an unhandled exception or crash, leaving
worker_process_pid_ stale. If the intermediate launcher then exited
without spawning a replacement worker, the stale PID was treated as a
successful launch and passed to OnProcessLaunchDetected().

This change:
1. Handles JOB_OBJECT_MSG_ABNORMAL_EXIT_PROCESS identically to
   JOB_OBJECT_MSG_EXIT_PROCESS in OnIOCompleted(), resetting
   worker_process_pid_.
2. Resets worker_process_pid_ in CloseChannel().
3. Adds defense-in-depth verification in OnProcessLaunchDetected() via
   ::IsProcessInJob() to ensure the opened process is a member of the
   delegate's job object before initiating IPC.
4. Adds unit tests covering normal and abnormal exit handling, launcher
   exit behavior, and job membership validation.

TAG=agy
CONV=2622fa76-8d29-4859-9d74-e7b26cd10bb8

Bug: 524423633
Change-Id: I71c315b964507b9abba8931bc18e847b0b7fe6fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8271670
Reviewed-by: Yuwei Huang <yuweih@chromium.org>
Commit-Queue: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1684314}
---

diff --git a/remoting/host/win/wts_session_process_delegate.cc b/remoting/host/win/wts_session_process_delegate.cc
index e38aeeb..02a7554 100644
--- a/remoting/host/win/wts_session_process_delegate.cc
+++ b/remoting/host/win/wts_session_process_delegate.cc
@@ -92,6 +92,15 @@
 
   bool AssignProcessToJobForTesting(base::ProcessHandle process);
   void SetCoreDeletedCallbackForTesting(base::OnceClosure callback);
+  void SimulateJobNotificationForTesting(DWORD event, base::ProcessId pid);
+  base::ProcessId GetWorkerProcessPidForTesting() const;
+  void SetElevatedLauncherPidForTesting(base::ProcessId pid);
+  void SetWorkerProcessPidForTesting(base::ProcessId pid);
+  void OnProcessLaunchDetectedForTesting(base::ProcessId pid);
+  void SetElevatedServerEndpointForTesting(
+      mojo::PlatformChannelServerEndpoint endpoint);
+  void SetFatalErrorCallbackForTesting(base::OnceClosure callback);
+  void SetProcessLaunchedCallbackForTesting(base::OnceClosure callback);
 
  private:
   friend class base::RefCountedThreadSafe<Core>;
@@ -198,6 +207,8 @@
   bool job_process_assigned_ = false;
 
   base::OnceClosure deleted_callback_for_testing_;
+  base::OnceClosure fatal_error_callback_for_testing_;
+  base::OnceClosure process_launched_callback_for_testing_;
 };
 
 WtsSessionProcessDelegate::Core::Core(
@@ -297,6 +308,7 @@
   channel_.reset();
   elevated_server_endpoint_.reset();
   elevated_launcher_pid_ = base::kNullProcessId;
+  worker_process_pid_ = base::kNullProcessId;
   mojo_invitation_ = {};
 }
 
@@ -345,6 +357,56 @@
   deleted_callback_for_testing_ = std::move(callback);
 }
 
+void WtsSessionProcessDelegate::Core::SimulateJobNotificationForTesting(
+    DWORD event,
+    base::ProcessId pid) {
+  if (!io_task_runner_->BelongsToCurrentThread()) {
+    io_task_runner_->PostTask(
+        FROM_HERE, base::BindOnce(&Core::SimulateJobNotificationForTesting,
+                                  this, event, pid));
+    return;
+  }
+  OnIOCompleted(reinterpret_cast<base::MessagePumpForIO::IOContext*>(
+                    static_cast<uintptr_t>(pid)),
+                event, 0);
+}
+
+base::ProcessId WtsSessionProcessDelegate::Core::GetWorkerProcessPidForTesting()
+    const {
+  return worker_process_pid_.load();
+}
+
+void WtsSessionProcessDelegate::Core::SetElevatedLauncherPidForTesting(
+    base::ProcessId pid) {
+  elevated_launcher_pid_ = pid;
+}
+
+void WtsSessionProcessDelegate::Core::SetWorkerProcessPidForTesting(
+    base::ProcessId pid) {
+  worker_process_pid_ = pid;
+}
+
+void WtsSessionProcessDelegate::Core::OnProcessLaunchDetectedForTesting(
+    base::ProcessId pid) {
+  DCHECK(caller_task_runner_->BelongsToCurrentThread());
+  OnProcessLaunchDetected(pid);
+}
+
+void WtsSessionProcessDelegate::Core::SetElevatedServerEndpointForTesting(
+    mojo::PlatformChannelServerEndpoint endpoint) {
+  elevated_server_endpoint_ = std::move(endpoint);
+}
+
+void WtsSessionProcessDelegate::Core::SetFatalErrorCallbackForTesting(
+    base::OnceClosure callback) {
+  fatal_error_callback_for_testing_ = std::move(callback);
+}
+
+void WtsSessionProcessDelegate::Core::SetProcessLaunchedCallbackForTesting(
+    base::OnceClosure callback) {
+  process_launched_callback_for_testing_ = std::move(callback);
+}
+
 WtsSessionProcessDelegate::Core::~Core() {
   DCHECK(!channel_);
   DCHECK(!event_handler_);
@@ -382,7 +444,8 @@
       }
       break;
     }
-    case JOB_OBJECT_MSG_EXIT_PROCESS: {
+    case JOB_OBJECT_MSG_EXIT_PROCESS:
+    case JOB_OBJECT_MSG_ABNORMAL_EXIT_PROCESS: {
       if (process_id == worker_process_pid_.load()) {
         // In official builds the first launch of a UiAccess enabled binary
         // will fail due to 'STATUS_ELEVATION_REQUIRED'.  This is an artifact of
@@ -654,6 +717,16 @@
     ReportFatalError();
     return;
   }
+
+  BOOL is_in_job = FALSE;
+  if (!job_.is_valid() ||
+      !::IsProcessInJob(worker_process.Get(), job_.Get(), &is_in_job) ||
+      !is_in_job) {
+    LOG(ERROR) << "Process " << pid << " is not in the expected job object.";
+    ReportFatalError();
+    return;
+  }
+
   elevated_launcher_pid_ = base::kNullProcessId;
   mojo::OutgoingInvitation::Send(std::move(mojo_invitation_),
                                  worker_process.Get(),
@@ -668,7 +741,12 @@
 
   WorkerProcessLauncher* event_handler = event_handler_;
   event_handler_ = nullptr;
-  event_handler->OnFatalError();
+  if (event_handler) {
+    event_handler->OnFatalError();
+  }
+  if (fatal_error_callback_for_testing_) {
+    std::move(fatal_error_callback_for_testing_).Run();
+  }
 }
 
 void WtsSessionProcessDelegate::Core::ReportProcessLaunched(
@@ -692,9 +770,12 @@
   }
   ScopedHandle limited_handle(temp_handle);
 
-  if (delegate_) {
+  if (delegate_ && event_handler_) {
     delegate_->WatchProcess(std::move(limited_handle));
   }
+  if (process_launched_callback_for_testing_) {
+    std::move(process_launched_callback_for_testing_).Run();
+  }
 }
 
 WtsSessionProcessDelegate::WtsSessionProcessDelegate(
@@ -738,14 +819,58 @@
   core_->KillProcess();
 }
 
-bool WtsSessionProcessDelegate::AssignProcessToJobForTesting(
+WtsSessionProcessDelegate::TestApi::TestApi(WtsSessionProcessDelegate* delegate)
+    : delegate_(delegate) {}
+
+bool WtsSessionProcessDelegate::TestApi::AssignProcessToJob(
     base::ProcessHandle process) {
-  return core_->AssignProcessToJobForTesting(process);  // IN-TEST
+  return delegate_->core_->AssignProcessToJobForTesting(process);
 }
 
-void WtsSessionProcessDelegate::SetCoreDeletedCallbackForTesting(
+void WtsSessionProcessDelegate::TestApi::SetCoreDeletedCallback(

diff --git a/remoting/host/win/wts_session_process_delegate_unittest.cc b/remoting/host/win/wts_session_process_delegate_unittest.cc
index 484ef86..6fd332a1 100644
--- a/remoting/host/win/wts_session_process_delegate_unittest.cc
+++ b/remoting/host/win/wts_session_process_delegate_unittest.cc
@@ -21,6 +21,7 @@
 #include "base/test/test_timeouts.h"
 #include "base/threading/thread.h"
 #include "base/win/scoped_handle.h"
+#include "mojo/public/cpp/platform/named_platform_channel.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/multiprocess_func_list.h"
 
@@ -79,14 +80,16 @@
       "WtsSessionProcessDelegateTestChild",
       base::GetMultiProcessTestChildBaseCommandLine(), {});
   ASSERT_TRUE(child.IsValid());
-  ASSERT_TRUE(delegate->AssignProcessToJobForTesting(child.Handle()));
+  ASSERT_TRUE(WtsSessionProcessDelegate::TestApi(delegate.get())
+                  .AssignProcessToJob(child.Handle()));
 
   bool core_deleted = false;
   base::RunLoop run_loop;
-  delegate->SetCoreDeletedCallbackForTesting(base::BindLambdaForTesting([&]() {
-    core_deleted = true;
-    run_loop.Quit();
-  }));
+  WtsSessionProcessDelegate::TestApi(delegate.get())
+      .SetCoreDeletedCallback(base::BindLambdaForTesting([&]() {
+        core_deleted = true;
+        run_loop.Quit();
+      }));
 
   // Destroying the delegate calls Stop(), which terminates the job. The Core
   // must remain alive until the I/O thread has delivered the final job
@@ -122,13 +125,15 @@
       "WtsSessionProcessDelegateTestChild",
       base::GetMultiProcessTestChildBaseCommandLine(), {});
   ASSERT_TRUE(child1.IsValid());
-  ASSERT_TRUE(delegate->AssignProcessToJobForTesting(child1.Handle()));
+  ASSERT_TRUE(WtsSessionProcessDelegate::TestApi(delegate.get())
+                  .AssignProcessToJob(child1.Handle()));
 
   base::Process child2 = base::SpawnMultiProcessTestChild(
       "WtsSessionProcessDelegateTestChild",
       base::GetMultiProcessTestChildBaseCommandLine(), {});
   ASSERT_TRUE(child2.IsValid());
-  ASSERT_TRUE(delegate->AssignProcessToJobForTesting(child2.Handle()));
+  ASSERT_TRUE(WtsSessionProcessDelegate::TestApi(delegate.get())
+                  .AssignProcessToJob(child2.Handle()));
 
   // Terminate child1 so a JOB_OBJECT_MSG_EXIT_PROCESS / ACTIVE_PROCESS_ZERO is
   // generated, but child2 remains active in the job object.
@@ -137,10 +142,11 @@
 
   bool core_deleted = false;
   base::RunLoop run_loop;
-  delegate->SetCoreDeletedCallbackForTesting(base::BindLambdaForTesting([&]() {
-    core_deleted = true;
-    run_loop.Quit();
-  }));
+  WtsSessionProcessDelegate::TestApi(delegate.get())
+      .SetCoreDeletedCallback(base::BindLambdaForTesting([&]() {
+        core_deleted = true;
+        run_loop.Quit();
+      }));
 
   // Destroying delegate stops the job. Since child2 is still in the job, Core
   // must stay alive until child2 is terminated.
@@ -155,6 +161,186 @@
                                             &exit_code));
 }
 
+TEST_F(WtsSessionProcessDelegateTest, AbnormalWorkerExitClearsWorkerPid) {
+  auto target_command =
+      std::make_unique<base::CommandLine>(base::CommandLine::NO_PROGRAM);
+  auto delegate = std::make_unique<WtsSessionProcessDelegate>(
+      io_thread_.task_runner(), std::move(target_command),
+      /*launch_elevated=*/true,
+      /*channel_security=*/std::string());
+
+  std::ignore = delegate->Initialize(WTSGetActiveConsoleSessionId());
+  FlushIoThread();
+
+  constexpr base::ProcessId kLauncherPid = 1000;
+  constexpr base::ProcessId kWorkerPid = 2000;
+
+  WtsSessionProcessDelegate::TestApi test_api(delegate.get());
+  test_api.SetElevatedLauncherPid(kLauncherPid);
+  test_api.SetWorkerProcessPid(kWorkerPid);
+  EXPECT_EQ(test_api.GetWorkerProcessPid(), kWorkerPid);
+
+  // Simulate JOB_OBJECT_MSG_ABNORMAL_EXIT_PROCESS for the worker process.
+  test_api.SimulateJobNotification(JOB_OBJECT_MSG_ABNORMAL_EXIT_PROCESS,
+                                   kWorkerPid);
+  FlushIoThread();
+
+  EXPECT_EQ(test_api.GetWorkerProcessPid(), base::kNullProcessId);
+}
+
+TEST_F(WtsSessionProcessDelegateTest, NormalWorkerExitClearsWorkerPid) {
+  auto target_command =
+      std::make_unique<base::CommandLine>(base::CommandLine::NO_PROGRAM);
+  auto delegate = std::make_unique<WtsSessionProcessDelegate>(
+      io_thread_.task_runner(), std::move(target_command),
+      /*launch_elevated=*/true,
+      /*channel_security=*/std::string());
+
+  std::ignore = delegate->Initialize(WTSGetActiveConsoleSessionId());
+  FlushIoThread();
+
+  constexpr base::ProcessId kLauncherPid = 1000;
+  constexpr base::ProcessId kWorkerPid = 2000;
+
+  WtsSessionProcessDelegate::TestApi test_api(delegate.get());
+  test_api.SetElevatedLauncherPid(kLauncherPid);
+  test_api.SetWorkerProcessPid(kWorkerPid);
+  EXPECT_EQ(test_api.GetWorkerProcessPid(), kWorkerPid);
+
+  // Simulate JOB_OBJECT_MSG_EXIT_PROCESS for the worker process.
+  test_api.SimulateJobNotification(JOB_OBJECT_MSG_EXIT_PROCESS, kWorkerPid);
+  FlushIoThread();
+
+  EXPECT_EQ(test_api.GetWorkerProcessPid(), base::kNullProcessId);
+}
+
+TEST_F(WtsSessionProcessDelegateTest,
+       LauncherExitAfterAbnormalWorkerExitDoesNotReportStalePid) {
+  auto target_command =
+      std::make_unique<base::CommandLine>(base::CommandLine::NO_PROGRAM);
+  auto delegate = std::make_unique<WtsSessionProcessDelegate>(
+      io_thread_.task_runner(), std::move(target_command),
+      /*launch_elevated=*/true,
+      /*channel_security=*/std::string());
+
+  std::ignore = delegate->Initialize(WTSGetActiveConsoleSessionId());
+  FlushIoThread();
+
+  constexpr base::ProcessId kLauncherPid = 1000;
+  constexpr base::ProcessId kWorkerPid = 2000;
+
+  WtsSessionProcessDelegate::TestApi test_api(delegate.get());
+  test_api.SetElevatedLauncherPid(kLauncherPid);
+  test_api.SetWorkerProcessPid(kWorkerPid);
+
+  // 1. Worker process crashes abnormally.
+  test_api.SimulateJobNotification(JOB_OBJECT_MSG_ABNORMAL_EXIT_PROCESS,
+                                   kWorkerPid);
+  FlushIoThread();
+
+  EXPECT_EQ(test_api.GetWorkerProcessPid(), base::kNullProcessId);
+
+  // 2. Intermediate launcher exits without launching a new worker.
+  test_api.SimulateJobNotification(JOB_OBJECT_MSG_EXIT_PROCESS, kLauncherPid);
+  FlushIoThread();
+
+  // The stale worker PID must not be retained or processed.
+  EXPECT_EQ(test_api.GetWorkerProcessPid(), base::kNullProcessId);
+}
+
+TEST_F(WtsSessionProcessDelegateTest,
+       LauncherExitWithActiveWorkerTriggersLaunchDetection) {
+  auto target_command =
+      std::make_unique<base::CommandLine>(base::CommandLine::NO_PROGRAM);
+  auto delegate = std::make_unique<WtsSessionProcessDelegate>(
+      io_thread_.task_runner(), std::move(target_command),
+      /*launch_elevated=*/true,
+      /*channel_security=*/std::string());
+
+  std::ignore = delegate->Initialize(WTSGetActiveConsoleSessionId());
+  FlushIoThread();
+
+  mojo::NamedPlatformChannel::Options options;
+  mojo::NamedPlatformChannel channel(options);
+  WtsSessionProcessDelegate::TestApi test_api(delegate.get());
+  test_api.SetElevatedServerEndpoint(channel.TakeServerEndpoint());
+
+  constexpr base::ProcessId kLauncherPid = 1000;
+  test_api.SetElevatedLauncherPid(kLauncherPid);
+
+  // Spawn a child process and assign it to the job object.
+  base::Process child = base::SpawnMultiProcessTestChild(
+      "WtsSessionProcessDelegateTestChild",
+      base::GetMultiProcessTestChildBaseCommandLine(), {});
+  ASSERT_TRUE(child.IsValid());
+  ASSERT_TRUE(test_api.AssignProcessToJob(child.Handle()));
+  FlushIoThread();
+  EXPECT_EQ(test_api.GetWorkerProcessPid(), child.Pid());
+
+  bool process_launched = false;
+  base::RunLoop run_loop;
+  test_api.SetProcessLaunchedCallback(base::BindLambdaForTesting([&]() {
+    process_launched = true;
+    run_loop.Quit();
+  }));
+
+  // Launcher exits after spawning worker.
+  test_api.SimulateJobNotification(JOB_OBJECT_MSG_EXIT_PROCESS, kLauncherPid);
+
+  // Wait for OnProcessLaunchDetected to run on main thread.
+  run_loop.Run();
+  EXPECT_TRUE(process_launched);
+
+  // Clean up child process.
+  child.Terminate(0, false);
+  int exit_code = 0;
+  EXPECT_TRUE(
+      child.WaitForExitWithTimeout(TestTimeouts::action_timeout(), &exit_code));
+}
+
+TEST_F(WtsSessionProcessDelegateTest, ProcessNotInJobRejectedInLaunchDetected) {
+  auto target_command =
+      std::make_unique<base::CommandLine>(base::CommandLine::NO_PROGRAM);
+  auto delegate = std::make_unique<WtsSessionProcessDelegate>(
+      io_thread_.task_runner(), std::move(target_command),
+      /*launch_elevated=*/true,
+      /*channel_security=*/std::string());
+
+  std::ignore = delegate->Initialize(WTSGetActiveConsoleSessionId());
+  FlushIoThread();
+
+  mojo::NamedPlatformChannel::Options options;
+  mojo::NamedPlatformChannel channel(options);
+  WtsSessionProcessDelegate::TestApi test_api(delegate.get());
+  test_api.SetElevatedServerEndpoint(channel.TakeServerEndpoint());
+
+  // Spawn a child process that is NOT assigned to the job object.
+  base::Process unassigned_child = base::SpawnMultiProcessTestChild(
+      "WtsSessionProcessDelegateTestChild",
+      base::GetMultiProcessTestChildBaseCommandLine(), {});
+  ASSERT_TRUE(unassigned_child.IsValid());
+
+  bool fatal_error_reported = false;
+  base::RunLoop run_loop;
+  test_api.SetFatalErrorCallback(base::BindLambdaForTesting([&]() {
+    fatal_error_reported = true;
+    run_loop.Quit();
+  }));
+
+  // Trigger OnProcessLaunchDetected with the unassigned child's PID.
+  test_api.OnProcessLaunchDetected(unassigned_child.Pid());
+
+  // Wait for ReportFatalError to be called.
+  run_loop.Run();
+  EXPECT_TRUE(fatal_error_reported);
+
+  // Clean up child process.
+  unassigned_child.Terminate(0, false);
+  int exit_code = 0;
+  EXPECT_TRUE(unassigned_child.WaitForExitWithTimeout(
+      TestTimeouts::action_timeout(), &exit_code));
+}
+
 MULTIPROCESS_TEST_MAIN(WtsSessionProcessDelegateTestChild) {
   // Block until the parent terminates this process via the job object.
   ::Sleep(INFINITE);