From c5418c2975948e97d4c88545d9fbeb30cfd0b50e Mon Sep 17 00:00:00 2001
From: S Ganesh <[email protected]>
Date: Mon, 08 Jun 2026 13:39:26 -0700
Subject: [PATCH] [updater] Thread-safe process management in LegacyAppCommandWebImpl

LegacyAppCommandWebImpl implements the COM interface IAppCommandWeb,
which can be accessed from different threads concurrently (MTA). The
member variable process_ was accessed concurrently in execute(),
get_status(), and get_exitCode(), which caused a data race and potential
double-close of handles.

This CL introduces a base::Lock to protect process_. In execute(), we
launch the process into a local variable and then move it into process_
under the lock. We also introduce a state flag `is_executing_` to
prevent concurrent execution requests from launching multiple process
instances before `process_` is assigned.

In get_exitCode(), we duplicate the process under the lock and call
WaitForExitWithTimeout on the duplicate to avoid holding the lock during
blocking calls. Accesses in get_status() and the process() accessor are
also protected by the lock.

Bug: b:520532191
Change-Id: I7e7556483ffc25b086f2fcc76735595371f21608
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7905606
Reviewed-by: Xiaoling Bao <[email protected]>
Commit-Queue: S Ganesh <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1643441}
---

diff --git a/chrome/updater/app/server/win/com_classes_legacy.cc b/chrome/updater/app/server/win/com_classes_legacy.cc
index 8941239..c2dff1d 100644
--- a/chrome/updater/app/server/win/com_classes_legacy.cc
+++ b/chrome/updater/app/server/win/com_classes_legacy.cc
@@ -27,6 +27,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/to_string.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/synchronization/lock.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/task/sequenced_task_runner.h"
 #include "base/task/task_traits.h"
@@ -1238,7 +1239,13 @@
     return E_UNEXPECTED;
   }
 
-  if (!process_.IsValid()) {
+  bool is_valid = false;
+  {
+    base::AutoLock lock(lock_);
+    is_valid = process_.IsValid();
+  }
+
+  if (!is_valid) {
     *status = COMMAND_STATUS_INIT;
   } else {
     *status = app_command_runner_.value()->TimedWait() ? COMMAND_STATUS_COMPLETE
@@ -1253,9 +1260,13 @@
     return E_INVALIDARG;
   }
 
+  base::Process process_dup = process();
+  if (!process_dup.IsValid()) {
+    return S_FALSE;
+  }
+
   int code = -1;
-  if (!process_.IsValid() ||
-      !process_.WaitForExitWithTimeout(base::TimeDelta(), &code)) {
+  if (!process_dup.WaitForExitWithTimeout(base::TimeDelta(), &code)) {
     return S_FALSE;
   }
 
@@ -1283,8 +1294,13 @@
                                               VARIANT substitution7,
                                               VARIANT substitution8,
                                               VARIANT substitution9) {
-  if (!app_command_runner_.has_value() || process_.IsValid()) {
-    return E_UNEXPECTED;
+  {
+    base::AutoLock lock(lock_);
+    if (!app_command_runner_.has_value() || is_executing_ ||
+        process_.IsValid()) {
+      return E_UNEXPECTED;
+    }
+    is_executing_ = true;
   }
 
   std::vector<std::wstring> substitutions;
@@ -1303,7 +1319,16 @@
     substitutions.push_back(substitution_string.value());
   }
 
-  const HRESULT hr = app_command_runner_.value()->Run(substitutions, process_);
+  base::Process process;
+  const HRESULT hr = app_command_runner_.value()->Run(substitutions, process);
+  {
+    base::AutoLock lock(lock_);
+    if (SUCCEEDED(hr)) {
+      process_ = std::move(process);
+    }
+    is_executing_ = false;
+  }
+
   using LegacyAppCommandWebImplPtr =
       Microsoft::WRL::ComPtr<LegacyAppCommandWebImpl>;
   AppServerWin::PostOnTaskRunner(
@@ -1356,7 +1381,7 @@
                         << " completed or was skipped: " << error;
               },
               LegacyAppCommandWebImplPtr(this)),
-          process_.Duplicate(), hr));
+          this->process(), hr));
   return hr;
 }
 
diff --git a/chrome/updater/app/server/win/com_classes_legacy.h b/chrome/updater/app/server/win/com_classes_legacy.h
index 319bc59c..233871b 100644
--- a/chrome/updater/app/server/win/com_classes_legacy.h
+++ b/chrome/updater/app/server/win/com_classes_legacy.h
@@ -21,6 +21,8 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/path_service.h"
 #include "base/process/process.h"
+#include "base/synchronization/lock.h"
+#include "base/thread_annotations.h"
 #include "base/types/expected.h"
 #include "base/win/win_util.h"
 #include "chrome/updater/app/server/win/updater_legacy_idl.h"
@@ -306,7 +308,10 @@
                          VARIANT substitution8,
                          VARIANT substitution9) override;
 
-  const base::Process& process() const { return process_; }
+  base::Process process() const {
+    base::AutoLock lock(lock_);
+    return process_.Duplicate();
+  }
 
  private:
   friend class LegacyAppCommandWebImplTest;
@@ -319,7 +324,9 @@
 
   ~LegacyAppCommandWebImpl() override;
 
-  base::Process process_;
+  mutable base::Lock lock_;
+  base::Process process_ GUARDED_BY(lock_);
+  bool is_executing_ GUARDED_BY(lock_) = false;
   HResultOr<scoped_refptr<AppCommandRunner>> app_command_runner_;
   UpdaterScope scope_ = UpdaterScope::kSystem;
   std::string app_id_;