From b1606afcc6bddf22e8ad1a6f8189e43d3d8b03f5 Mon Sep 17 00:00:00 2001
From: S Ganesh <ganesh@chromium.org>
Date: Tue, 28 Jul 2026 13:11:47 -0700
Subject: [PATCH] [updater] Fix data race and validate input in AppWebImpl

In AppWebImpl (implementing the legacy IAppWeb COM interface for the
Windows Chrome updater service), member variables such as
install_data_index_, set_ready_to_install_, and current_operation_ can
be accessed concurrently by multiple RPC threads in the Multi-Threaded
Apartment (MTA) model without synchronization.

This change:
- Synchronizes access to install_data_index_, set_ready_to_install_, and
  current_operation_ under lock_.
- Adds a thread-safe helper GetInstallDataIndex() to retrieve a copy of
  install_data_index_.
- Validates the length of string input in put_serverInstallDataIndex via
  ValidateInstallDataIndex before mutation.
- Adds GUARDED_BY(lock_) compiler annotations for thread safety analysis.
- Adds unit test coverage (AppWebImplTest.ServerInstallDataIndex) verifying
  property access and validation rejection.

Fixed: 524453236
Change-Id: Ie28c8bad453ac121dfd443d0edab37bf00f1a661
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8145223
Commit-Queue: S Ganesh <ganesh@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1669695}
---

diff --git a/chrome/updater/app/server/win/com_classes_legacy.cc b/chrome/updater/app/server/win/com_classes_legacy.cc
index c31faa1f..62b1fdbf 100644
--- a/chrome/updater/app/server/win/com_classes_legacy.cc
+++ b/chrome/updater/app/server/win/com_classes_legacy.cc
@@ -573,7 +573,10 @@
   // foreground priority and disallows same version updates.
   HRESULT CheckForUpdate() {
     VLOG(2) << __func__;
-    current_operation_ = CurrentOperation::kCheckingForUpdates;
+    {
+      base::AutoLock lock{lock_};
+      current_operation_ = CurrentOperation::kCheckingForUpdates;
+    }
     return DoOperation(
         base::BindOnce(&AppWebImpl::CheckForUpdateImpl, AppWebImplPtr(this)));
   }
@@ -594,7 +597,10 @@
 
   HRESULT UpdateOrInstall() {
     VLOG(2) << __func__;
-    current_operation_ = CurrentOperation::kUpdatingOrInstalling;
+    {
+      base::AutoLock lock{lock_};
+      current_operation_ = CurrentOperation::kUpdatingOrInstalling;
+    }
     return DoOperation(base::BindOnce(
         is_install_ ? &AppWebImpl::InstallImpl : &AppWebImpl::UpdateImpl,
         AppWebImplPtr(this)));
@@ -613,9 +619,10 @@
     request.brand_code = brand_code_;
     request.ap = ap_;
 
-    update_service->Install(
-        request, {}, install_data_index_, UpdateService::Priority::kForeground,
-        language_, state_change_callback, std::move(complete_callback));
+    update_service->Install(request, {}, GetInstallDataIndex(),
+                            UpdateService::Priority::kForeground, language_,
+                            state_change_callback,
+                            std::move(complete_callback));
   }
 
   void UpdateImpl(
@@ -625,7 +632,7 @@
       scoped_refptr<UpdateService> update_service) {
     CHECK(update_service);
 
-    update_service->Update(app_id_, install_data_index_,
+    update_service->Update(app_id_, GetInstallDataIndex(),
                            UpdateService::Priority::kForeground,
                            policy_same_version_update_, language_,
                            state_change_callback, std::move(complete_callback));
@@ -633,7 +640,10 @@
 
   // Legacy compatibility: sets a flag that causes `get_currentState` to return
   // `STATE_READY_TO_INSTALL` when the update state is `kUpdateAvailable`.
-  void SetReadyToInstall() { set_ready_to_install_ = true; }
+  void SetReadyToInstall() {
+    base::AutoLock lock{lock_};
+    set_ready_to_install_ = true;
+  }
 
   // Overrides for IAppWeb.
   IFACEMETHODIMP get_appId(BSTR* app_id) override {
@@ -854,16 +864,20 @@
     }
 
     *install_data_index =
-        base::win::ScopedBstr(base::UTF8ToWide(install_data_index_)).Release();
+        base::win::ScopedBstr(base::UTF8ToWide(GetInstallDataIndex()))
+            .Release();
     return S_OK;
   }
 
   IFACEMETHODIMP put_serverInstallDataIndex(BSTR install_data_index) override {
-    if (!install_data_index) {
+    std::optional<std::string> install_data_index_str =
+        ValidateInstallDataIndex(install_data_index);
+    if (!install_data_index_str) {
       return E_INVALIDARG;
     }
 
-    install_data_index_ = base::WideToUTF8(install_data_index);
+    base::AutoLock lock{lock_};
+    install_data_index_ = *std::move(install_data_index_str);
     return S_OK;
   }
 
@@ -935,6 +949,11 @@
     result_ = result;
   }
 
+  std::string GetInstallDataIndex() const {
+    base::AutoLock lock{lock_};
+    return install_data_index_;
+  }
+
   // Handles the update service callbacks.
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
 
@@ -944,19 +963,20 @@
   std::string brand_code_;
   std::string ap_;
   std::string language_;
-  std::string install_data_index_;
   UpdateService::PolicySameVersionUpdate policy_same_version_update_ =
       UpdateService::PolicySameVersionUpdate::kNotAllowed;
-  bool set_ready_to_install_ = false;
   ProgressSampler download_progress_sampler_;
   ProgressSampler install_progress_sampler_;
 
-  // Access to `state_update_` and `result_` must be serialized by using the
-  // lock.
+  // Serializes access to the members that may be read or written on multiple
+  // COM RPC threads.
   mutable base::Lock lock_;
-  std::optional<UpdateService::UpdateState> state_update_;
-  std::optional<UpdateService::Result> result_;
-  CurrentOperation current_operation_ = CurrentOperation::kUnknown;
+  std::string install_data_index_ GUARDED_BY(lock_);
+  bool set_ready_to_install_ GUARDED_BY(lock_) = false;
+  CurrentOperation current_operation_ GUARDED_BY(lock_) =
+      CurrentOperation::kUnknown;
+  std::optional<UpdateService::UpdateState> state_update_ GUARDED_BY(lock_);
+  std::optional<UpdateService::Result> result_ GUARDED_BY(lock_);
 };
 
 // This class implements the legacy Omaha3 IAppBundleWeb interface as expected
diff --git a/chrome/updater/app/server/win/com_classes_legacy_unittest.cc b/chrome/updater/app/server/win/com_classes_legacy_unittest.cc
index acd52ce..8d99cc15 100644
--- a/chrome/updater/app/server/win/com_classes_legacy_unittest.cc
+++ b/chrome/updater/app/server/win/com_classes_legacy_unittest.cc
@@ -391,6 +391,44 @@
   EXPECT_EQ(exit_code, 7U);
 }
 
+TEST(AppWebImplTest, ServerInstallDataIndex) {
+  base::test::TaskEnvironment environment;
+
+  Microsoft::WRL::ComPtr<LegacyOnDemandImpl> on_demand =
+      Microsoft::WRL::Make<LegacyOnDemandImpl>();
+  Microsoft::WRL::ComPtr<IDispatch> bundle_dispatch;
+  ASSERT_HRESULT_SUCCEEDED(on_demand->createAppBundleWeb(&bundle_dispatch));
+  Microsoft::WRL::ComPtr<IAppBundleWeb> bundle;
+  ASSERT_HRESULT_SUCCEEDED(bundle_dispatch.As(&bundle));
+  ASSERT_HRESULT_SUCCEEDED(
+      bundle->createInstalledApp(base::win::ScopedBstr(kAppId1).Get()));
+  Microsoft::WRL::ComPtr<IDispatch> app_dispatch;
+  ASSERT_HRESULT_SUCCEEDED(bundle->get_appWeb(0, &app_dispatch));
+  Microsoft::WRL::ComPtr<IAppWeb> app;
+  ASSERT_HRESULT_SUCCEEDED(app_dispatch.As(&app));
+
+  EXPECT_HRESULT_SUCCEEDED(app->put_serverInstallDataIndex(
+      base::win::ScopedBstr(L"installdataindex").Get()));
+  {
+    base::win::ScopedBstr install_data_index;
+    EXPECT_HRESULT_SUCCEEDED(
+        app->get_serverInstallDataIndex(install_data_index.Receive()));
+    EXPECT_STREQ(install_data_index.Get(), L"installdataindex");
+  }
+
+  // Values exceeding the input-length limit are rejected and do not change the
+  // stored value.
+  EXPECT_EQ(app->put_serverInstallDataIndex(
+                base::win::ScopedBstr(std::wstring(0x4001, L'a')).Get()),
+            E_INVALIDARG);
+  {
+    base::win::ScopedBstr install_data_index;
+    EXPECT_HRESULT_SUCCEEDED(
+        app->get_serverInstallDataIndex(install_data_index.Receive()));
+    EXPECT_STREQ(install_data_index.Get(), L"installdataindex");

diff --git a/chrome/updater/app/server/win/com_classes_legacy_unittest.cc b/chrome/updater/app/server/win/com_classes_legacy_unittest.cc
index acd52ce..8d99cc15 100644
--- a/chrome/updater/app/server/win/com_classes_legacy_unittest.cc
+++ b/chrome/updater/app/server/win/com_classes_legacy_unittest.cc
@@ -391,6 +391,44 @@
   EXPECT_EQ(exit_code, 7U);
 }
 
+TEST(AppWebImplTest, ServerInstallDataIndex) {
+  base::test::TaskEnvironment environment;
+
+  Microsoft::WRL::ComPtr<LegacyOnDemandImpl> on_demand =
+      Microsoft::WRL::Make<LegacyOnDemandImpl>();
+  Microsoft::WRL::ComPtr<IDispatch> bundle_dispatch;
+  ASSERT_HRESULT_SUCCEEDED(on_demand->createAppBundleWeb(&bundle_dispatch));
+  Microsoft::WRL::ComPtr<IAppBundleWeb> bundle;
+  ASSERT_HRESULT_SUCCEEDED(bundle_dispatch.As(&bundle));
+  ASSERT_HRESULT_SUCCEEDED(
+      bundle->createInstalledApp(base::win::ScopedBstr(kAppId1).Get()));
+  Microsoft::WRL::ComPtr<IDispatch> app_dispatch;
+  ASSERT_HRESULT_SUCCEEDED(bundle->get_appWeb(0, &app_dispatch));
+  Microsoft::WRL::ComPtr<IAppWeb> app;
+  ASSERT_HRESULT_SUCCEEDED(app_dispatch.As(&app));
+
+  EXPECT_HRESULT_SUCCEEDED(app->put_serverInstallDataIndex(
+      base::win::ScopedBstr(L"installdataindex").Get()));
+  {
+    base::win::ScopedBstr install_data_index;
+    EXPECT_HRESULT_SUCCEEDED(
+        app->get_serverInstallDataIndex(install_data_index.Receive()));
+    EXPECT_STREQ(install_data_index.Get(), L"installdataindex");
+  }
+
+  // Values exceeding the input-length limit are rejected and do not change the
+  // stored value.
+  EXPECT_EQ(app->put_serverInstallDataIndex(
+                base::win::ScopedBstr(std::wstring(0x4001, L'a')).Get()),
+            E_INVALIDARG);
+  {
+    base::win::ScopedBstr install_data_index;
+    EXPECT_HRESULT_SUCCEEDED(
+        app->get_serverInstallDataIndex(install_data_index.Receive()));
+    EXPECT_STREQ(install_data_index.Get(), L"installdataindex");
+  }
+}
+
 TEST(LegacyCOMClassesTest, CheckLegacyInterfaceIDs) {
 #if BUILDFLAG(GOOGLE_CHROME_BRANDING)
   EXPECT_EQ(StringFromGuid(__uuidof(GoogleUpdate3WebUserClass)),