From 4fac708225508abff10cd79ee1d71e3fbd72b4b1 Mon Sep 17 00:00:00 2001
From: S Ganesh <[email protected]>
Date: Fri, 29 May 2026 15:16:58 -0700
Subject: [PATCH] updater: Fix shutdown race condition in PolicyStatus COM class

During service shutdown, AppServer::Uninitialize() resets its
scoped_refptr<Configurator> member config_ to nullptr on the main
sequence thread. However, if a COM client concurrently calls
CoCreateInstance/CreateInstance on an MTA RPC thread, the
PolicyStatusImpl constructor could synchronously fetch the Configurator
pointer via AppServer::config() to obtain the policy service, causing a
Use-After-Free (UAF) data race on the scoped_refptr.

This CL resolves this vulnerability by:
1. Protecting config_ reads and assignments inside AppServer under
   base::Lock config_lock_, ensuring thread safety when WRL COM threads
   query for configurator instance concurrently.
2. Guarding the config_ member in Clang with GUARDED_BY(config_lock_)
   annotations to enforce thread safety at compile time.
3. Moving the policy_service_ initialization from the PolicyStatusImpl
   constructor initializer list into RuntimeClassInitialize().
4. Returning E_FAIL from RuntimeClassInitialize() if config_ has
   already been deallocated during shutdown, gracefully rejecting
   incoming client connections and preventing downstream nullptr
   dereferences.

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

diff --git a/chrome/updater/app/app_server.cc b/chrome/updater/app/app_server.cc
index b7196744..064323d 100644
--- a/chrome/updater/app/app_server.cc
+++ b/chrome/updater/app/app_server.cc
@@ -21,6 +21,7 @@
 #include "base/process/launch.h"
 #include "base/process/process.h"
 #include "base/run_loop.h"
+#include "base/synchronization/lock.h"
 #include "base/time/time.h"
 #include "base/version.h"
 #include "build/build_config.h"
@@ -98,12 +99,17 @@
     if (!local_prefs->GetQualified()) {
       global_prefs = nullptr;
       prefs_ = local_prefs;
-      config_ = base::MakeRefCounted<Configurator>(prefs_, external_constants_,
-                                                   updater_scope());
+      scoped_refptr<Configurator> config;
+      {
+        base::AutoLock lock(config_lock_);
+        config_ = base::MakeRefCounted<Configurator>(
+            prefs_, external_constants_, updater_scope());
+        config = config_;
+      }
       if (IsInternalService()) {
         return base::BindOnce(
             &AppServer::ActiveDutyInternal, this,
-            MakeQualifyingUpdateServiceInternal(config_, local_prefs));
+            MakeQualifyingUpdateServiceInternal(config, local_prefs));
       }
 
 #if BUILDFLAG(IS_WIN)
@@ -140,11 +146,16 @@
 
   server_starts_ = global_prefs->CountServerStarts();
   prefs_ = global_prefs;
-  config_ = base::MakeRefCounted<Configurator>(prefs_, external_constants_,
-                                               updater_scope());
+  scoped_refptr<Configurator> config;
+  {
+    base::AutoLock lock(config_lock_);
+    config_ = base::MakeRefCounted<Configurator>(prefs_, external_constants_,
+                                                 updater_scope());
+    config = config_;
+  }
   return base::BindOnce(
       &AppServer::ActiveDuty, this,
-      base::MakeRefCounted<UpdateServiceImpl>(updater_scope(), config_));
+      base::MakeRefCounted<UpdateServiceImpl>(updater_scope(), config));
 }
 
 void AppServer::TaskStarted() {
@@ -176,9 +187,14 @@
 }
 
 void AppServer::Uninitialize() {
-  if (config_ && config_->GetEventLogger()) {
+  scoped_refptr<Configurator> config;
+  {
+    base::AutoLock lock(config_lock_);
+    config = config_;
+  }
+  if (config && config->GetEventLogger()) {
     base::RunLoop run_loop;
-    config_->GetEventLogger()->Flush(run_loop.QuitClosure());
+    config->GetEventLogger()->Flush(run_loop.QuitClosure());
     run_loop.Run();
   }
   // Simply stopping the timer does not destroy its task. The task holds a
@@ -199,16 +215,24 @@
   // Because this instance is leaky when running on Windows, the following
   // references must be reset to destroy the objects, otherwise `Prefs` leaks.
   prefs_ = nullptr;
-  config_ = nullptr;
+  {
+    base::AutoLock lock(config_lock_);
+    config_ = nullptr;
+  }
 }
 
 void AppServer::MaybeUninstall() {
-  if (!config_ || IsInternalService()) {
+  scoped_refptr<Configurator> config;
+  {
+    base::AutoLock lock(config_lock_);
+    config = config_;
+  }
+  if (!config || IsInternalService()) {
     return;
   }
 
   scoped_refptr<PersistedData> persisted_data =
-      config_->GetUpdaterPersistedData();
+      config->GetUpdaterPersistedData();
   if (ShouldUninstall(persisted_data->GetAppIds(), server_starts_,
                       persisted_data->GetHadApps())) {
     std::optional<base::FilePath> executable =
diff --git a/chrome/updater/app/app_server.h b/chrome/updater/app/app_server.h
index 5bf028f..03f7d77 100644
--- a/chrome/updater/app/app_server.h
+++ b/chrome/updater/app/app_server.h
@@ -8,6 +8,8 @@
 #include "base/functional/callback_forward.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/sequence_checker.h"
+#include "base/synchronization/lock.h"
+#include "base/thread_annotations.h"
 #include "base/timer/timer.h"
 #include "chrome/updater/app/app.h"
 #include "chrome/updater/configurator.h"
@@ -41,7 +43,12 @@
 
   scoped_refptr<const UpdaterPrefs> prefs() const { return prefs_; }
 
-  scoped_refptr<Configurator> config() const { return config_; }
+  // Thread-safe: COM MTA RPC threads access this getter concurrently
+  // with main sequence writes during shutdown.
+  scoped_refptr<Configurator> config() const {
+    base::AutoLock lock(config_lock_);
+    return config_;
+  }
 
   void TaskStarted();
   void TaskCompleted();
@@ -104,7 +111,8 @@
       CreateExternalConstants();
   base::OnceClosure first_task_;
   scoped_refptr<UpdaterPrefs> prefs_;
-  scoped_refptr<Configurator> config_;
+  mutable base::Lock config_lock_;
+  scoped_refptr<Configurator> config_ GUARDED_BY(config_lock_);
   base::RepeatingTimer hang_timer_;
 
   // If true, this version of the updater uninstalls itself during shutdown.
diff --git a/chrome/updater/app/server/win/com_classes_legacy.cc b/chrome/updater/app/server/win/com_classes_legacy.cc
index 50696aca..8941239 100644
--- a/chrome/updater/app/server/win/com_classes_legacy.cc
+++ b/chrome/updater/app/server/win/com_classes_legacy.cc
@@ -1416,14 +1416,26 @@
                                    {IID_MAP_ENTRY_SYSTEM(IPolicyStatus4),
                                     IID_MAP_ENTRY_SYSTEM(IPolicyStatus3),
                                     IID_MAP_ENTRY_SYSTEM(IPolicyStatus2),
-                                    IID_MAP_ENTRY_SYSTEM(IPolicyStatus)}),
-      policy_service_(GetAppServerWinInstance()->config()->GetPolicyService()) {
-}
+                                    IID_MAP_ENTRY_SYSTEM(IPolicyStatus)}) {}
 PolicyStatusImpl::~PolicyStatusImpl() = default;
 
 HRESULT PolicyStatusImpl::RuntimeClassInitialize() {
   VLOG(2) << __func__;
   LogComCaller(__FUNCTION__);
+
+  scoped_refptr<AppServerWin> app_server = GetAppServerWinInstance();
+  if (!app_server) {
+    return E_FAIL;
+  }
+  scoped_refptr<Configurator> config = app_server->config();
+  if (!config) {
+    return E_FAIL;
+  }
+  policy_service_ = config->GetPolicyService();
+  if (!policy_service_) {
+    return E_FAIL;
+  }
+