Fix use-after-free in ModuleInspector

After moving the ModuleDatabase (and its dependent classes) to its own
sequence, the service manager connector can no longer be retrieved
directly using the content::ServiceManagerConnection instance.

Instead pass a cloned connector when calling InitializeModuleDatabase()
from the UI thread.

Bug: 958505
Change-Id: I0f47cf66298d54ca82448e26181ab8b9bb358a58
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1592350Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Sébastien Marchand <sebmarchand@chromium.org>
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659101}

chrome/browser/chrome_browser_main_win.cc

@@ -82,6 +82,8 @@
 #include "content/public/browser/render_process_host.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/main_function_params.h"
+#include "content/public/common/service_manager_connection.h"
+#include "services/service_manager/public/cpp/connector.h"
 #include "ui/base/cursor/cursor_loader_win.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/l10n/l10n_util_win.h"
@@ -195,11 +197,13 @@ void DetectFaultTolerantHeap() {
 
 // Initializes the ModuleDatabase on its owning sequence. Also starts the
 // enumeration of registered modules in the Windows Registry.
-void InitializeModuleDatabase(bool is_third_party_blocking_policy_enabled) {
+void InitializeModuleDatabase(
+    std::unique_ptr<service_manager::Connector> connector,
+    bool is_third_party_blocking_policy_enabled) {
   DCHECK(ModuleDatabase::GetTaskRunner()->RunsTasksInCurrentSequence());
 
-  ModuleDatabase::SetInstance(
-      std::make_unique<ModuleDatabase>(is_third_party_blocking_policy_enabled));
+  ModuleDatabase::SetInstance(std::make_unique<ModuleDatabase>(
+      std::move(connector), is_third_party_blocking_policy_enabled));
 
   auto* module_database = ModuleDatabase::GetInstance();
   module_database->StartDrainingModuleLoadAttemptsLog();
@@ -411,8 +415,12 @@ void SetupModuleDatabase(std::unique_ptr<ModuleWatcher>* module_watcher) {
 #endif
 
   ModuleDatabase::GetTaskRunner()->PostTask(
-      FROM_HERE, base::BindOnce(&InitializeModuleDatabase,
-                                third_party_blocking_policy_enabled));
+      FROM_HERE,
+      base::BindOnce(&InitializeModuleDatabase,
+                     content::ServiceManagerConnection::GetForProcess()
+                         ->GetConnector()
+                         ->Clone(),
+                     third_party_blocking_policy_enabled));
 
   *module_watcher = ModuleWatcher::Create(base::BindRepeating(&OnModuleEvent));
 }

chrome/browser/conflicts/module_database_win.cc

@@ -89,7 +89,9 @@ void InitPrefChangeRegistrarOnUIThread(
 // static
 constexpr base::TimeDelta ModuleDatabase::kIdleTimeout;
 
-ModuleDatabase::ModuleDatabase(bool third_party_blocking_policy_enabled)
+ModuleDatabase::ModuleDatabase(
+    std::unique_ptr<service_manager::Connector> connector,
+    bool third_party_blocking_policy_enabled)
     : idle_timer_(
           FROM_HERE,
           kIdleTimeout,
@@ -103,7 +105,8 @@ ModuleDatabase::ModuleDatabase(bool third_party_blocking_policy_enabled)
       // ModuleDatabase owns |module_inspector_|, so it is safe to use
       // base::Unretained().
       module_inspector_(base::Bind(&ModuleDatabase::OnModuleInspected,
-                                   base::Unretained(this))) {
+                                   base::Unretained(this)),
+                        std::move(connector)) {
   AddObserver(&module_inspector_);
   AddObserver(&third_party_metrics_);
 

chrome/browser/conflicts/module_database_win.h

@@ -17,6 +17,7 @@
 #include "chrome/browser/conflicts/module_inspector_win.h"
 #include "chrome/browser/conflicts/third_party_metrics_recorder_win.h"
 #include "content/public/common/process_type.h"
+#include "services/service_manager/public/cpp/connector.h"
 
 class ModuleDatabaseObserver;
 
@@ -58,7 +59,8 @@ class ModuleDatabase : public ModuleDatabaseEventSource {
 
   // Creates the ModuleDatabase. Must be created and set on the sequence
   // returned by GetTaskRunner().
-  explicit ModuleDatabase(bool third_party_blocking_policy_enabled);
+  explicit ModuleDatabase(std::unique_ptr<service_manager::Connector> connector,
+                          bool third_party_blocking_policy_enabled);
   ~ModuleDatabase() override;
 
   // Returns the SequencedTaskRunner on which the ModuleDatabase lives. Can be

chrome/browser/conflicts/module_database_win_unittest.cc

@@ -41,6 +41,7 @@ class ModuleDatabaseTest : public testing::Test {
             base::test::ScopedTaskEnvironment::MainThreadType::UI_MOCK_TIME),
         scoped_testing_local_state_(TestingBrowserProcess::GetGlobal()),
         module_database_(std::make_unique<ModuleDatabase>(
+            nullptr,
             /* third_party_blocking_policy_enabled = */ false)) {}
 
   ~ModuleDatabaseTest() override {

chrome/browser/conflicts/module_inspector_win.cc

@@ -17,7 +17,6 @@
 #include "chrome/common/chrome_paths.h"
 #include "chrome/services/util_win/public/mojom/constants.mojom.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/common/service_manager_connection.h"
 #include "services/service_manager/public/cpp/connector.h"
 
 namespace {
@@ -76,9 +75,11 @@ constexpr base::Feature ModuleInspector::kWinOOPInspectModuleFeature;
 constexpr base::TimeDelta ModuleInspector::kFlushInspectionResultsTimerTimeout;
 
 ModuleInspector::ModuleInspector(
-    const OnModuleInspectedCallback& on_module_inspected_callback)
+    const OnModuleInspectedCallback& on_module_inspected_callback,
+    std::unique_ptr<service_manager::Connector> connector)
     : on_module_inspected_callback_(on_module_inspected_callback),
       is_after_startup_(false),
+      connector_(std::move(connector)),
       inspection_task_runner_(base::CreateSequencedTaskRunnerWithTraits(
           {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
            base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})),
@@ -169,16 +170,8 @@ void ModuleInspector::EnsureUtilWinServiceBound() {
   DCHECK(base::FeatureList::IsEnabled(kWinOOPInspectModuleFeature));
 
   // Use the |test_connector_| if set.
-  service_manager::Connector* connector = test_connector_;
-  if (!connector) {
-    // The ServiceManagerConnection can be null during shutdown.
-    content::ServiceManagerConnection* service_manager_connection =
-        content::ServiceManagerConnection::GetForProcess();
-    if (!service_manager_connection)
-      return;
-
-    connector = service_manager_connection->GetConnector();
-  }
+  service_manager::Connector* connector =
+      test_connector_ ? test_connector_ : connector_.get();
 
   connector->BindInterface(chrome::mojom::kUtilWinServiceName, &util_win_ptr_);
   util_win_ptr_.set_connection_error_handler(

chrome/browser/conflicts/module_inspector_win.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_CONFLICTS_MODULE_INSPECTOR_WIN_H_
 
 #include <map>
+#include <memory>
 
 #include "base/callback.h"
 #include "base/containers/queue.h"
@@ -61,8 +62,8 @@ class ModuleInspector : public ModuleDatabaseObserver {
       base::Callback<void(const ModuleInfoKey& module_key,
                           ModuleInspectionResult inspection_result)>;
 
-  explicit ModuleInspector(
-      const OnModuleInspectedCallback& on_module_inspected_callback);
+  ModuleInspector(const OnModuleInspectedCallback& on_module_inspected_callback,
+                  std::unique_ptr<service_manager::Connector> connector);
   ~ModuleInspector() override;
 
   // Adds the module to the queue of modules to inspect. Starts the inspection
@@ -132,6 +133,9 @@ class ModuleInspector : public ModuleDatabaseObserver {
   // inspection tasks in order to not negatively impact startup performance.
   bool is_after_startup_;
 
+  // Allows this class to connect to the UtilWin service.
+  std::unique_ptr<service_manager::Connector> connector_;
+
   // A pointer to the UtilWin service. Only used if the WinOOPInspectModule
   // feature is enabled. It is created when inspection is ongoing, and freed
   // when no longer needed.

chrome/browser/conflicts/module_inspector_win_unittest.cc

@@ -90,8 +90,10 @@ class ModuleInspectorTest : public testing::Test {
 }  // namespace
 
 TEST_F(ModuleInspectorTest, OneModule) {
-  ModuleInspector module_inspector(base::Bind(
-      &ModuleInspectorTest::OnModuleInspected, base::Unretained(this)));
+  ModuleInspector module_inspector(
+      base::Bind(&ModuleInspectorTest::OnModuleInspected,
+                 base::Unretained(this)),
+      nullptr);
 
   module_inspector.AddModule({GetKernel32DllFilePath(), 0, 0});
 
@@ -107,8 +109,10 @@ TEST_F(ModuleInspectorTest, MultipleModules) {
       {base::FilePath(), 0, 0},
   };
 
-  ModuleInspector module_inspector(base::Bind(
-      &ModuleInspectorTest::OnModuleInspected, base::Unretained(this)));
+  ModuleInspector module_inspector(
+      base::Bind(&ModuleInspectorTest::OnModuleInspected,
+                 base::Unretained(this)),
+      nullptr);
 
   for (const auto& module : kTestCases)
     module_inspector.AddModule(module);
@@ -128,8 +132,10 @@ TEST_F(ModuleInspectorTest, DisableBackgroundInspection) {
       {base::FilePath(), 0, 0},
   };
 
-  ModuleInspector module_inspector(base::Bind(
-      &ModuleInspectorTest::OnModuleInspected, base::Unretained(this)));
+  ModuleInspector module_inspector(
+      base::Bind(&ModuleInspectorTest::OnModuleInspected,
+                 base::Unretained(this)),
+      nullptr);
 
   for (const auto& module : kTestCases)
     module_inspector.AddModule(module);
@@ -161,8 +167,10 @@ TEST_F(ModuleInspectorTest, OOPInspectModule) {
       {base::FilePath(), 0, 0},
   };
 
-  ModuleInspector module_inspector(base::Bind(
-      &ModuleInspectorTest::OnModuleInspected, base::Unretained(this)));
+  ModuleInspector module_inspector(
+      base::Bind(&ModuleInspectorTest::OnModuleInspected,
+                 base::Unretained(this)),
+      nullptr);
   module_inspector.SetConnectorForTesting(
       test_connector_factory_.GetDefaultConnector());
 
@@ -192,8 +200,10 @@ TEST_F(ModuleInspectorTest, InspectionResultsCache) {
   ASSERT_TRUE(
       CreateInspectionResultsCacheWithEntry(module_key, inspection_result));
 
-  ModuleInspector module_inspector(base::Bind(
-      &ModuleInspectorTest::OnModuleInspected, base::Unretained(this)));
+  ModuleInspector module_inspector(
+      base::Bind(&ModuleInspectorTest::OnModuleInspected,
+                 base::Unretained(this)),
+      nullptr);
 
   module_inspector.AddModule(module_key);
 
@@ -219,8 +229,10 @@ TEST_F(ModuleInspectorTest, InspectionResultsCache_OnModuleDatabaseIdle) {
   base::ScopedPathOverride scoped_user_data_dir_override(
       chrome::DIR_USER_DATA, scoped_temp_dir.GetPath());
 
-  ModuleInspector module_inspector(base::Bind(
-      &ModuleInspectorTest::OnModuleInspected, base::Unretained(this)));
+  ModuleInspector module_inspector(
+      base::Bind(&ModuleInspectorTest::OnModuleInspected,
+                 base::Unretained(this)),
+      nullptr);
 
   ModuleInfoKey module_key(GetKernel32DllFilePath(), 0, 0);
   module_inspector.AddModule(module_key);
@@ -258,8 +270,10 @@ TEST_F(ModuleInspectorTest, InspectionResultsCache_TimerExpired) {
   base::ScopedPathOverride scoped_user_data_dir_override(
       chrome::DIR_USER_DATA, scoped_temp_dir.GetPath());
 
-  ModuleInspector module_inspector(base::Bind(
-      &ModuleInspectorTest::OnModuleInspected, base::Unretained(this)));
+  ModuleInspector module_inspector(
+      base::Bind(&ModuleInspectorTest::OnModuleInspected,
+                 base::Unretained(this)),
+      nullptr);
 
   ModuleInfoKey module_key(GetKernel32DllFilePath(), 0, 0);
   module_inspector.AddModule(module_key);