From 01cf5ac834163941ed9be27307d1a2b2794dfa49 Mon Sep 17 00:00:00 2001
From: Fergal Daly <[email protected]>
Date: Fri, 22 May 2026 02:11:17 -0700
Subject: [PATCH] Fix UAFs arising from lifetime of File*Observers.

Calls to these observers get posted to multiple sequences and can result
in UAFs if the observer is freed while such a call is still queued.

The fix is to
- Make them all ref-counted so that they are not deleted while calls are queued.
- Add a "Disable" method which is called at the point where they would previously have been freed. Disabling ensures that when the queued call is executed, it is a no-op.

Disabling must be done with a lock held as these observers and the
objects that own them are on different threads.

This also requires some tweaks to
- SandboxQuotaObserver to avoid dangling pointers.
- FileSystemAccessBucketPathWatcher to avoid leaking a cycle of scoped_refptrs. This holds a reference to FileSystemContext via its base class FileSystemAccessChangeSource and FileSystemContext holds a refernce to FileSystemAccessBucketPathWatcher.

Because this is a race, I couldn't make a reliable browser test
possible.

The leak was detected by several unittests, so no extra test is added
for that.

Bug: 486761163,505096898
Change-Id: Ibfb5ac2a85f5d5340e3564d7c8a25571c6fbafb5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7805989
Reviewed-by: Ming-Ying Chung <[email protected]>
Commit-Queue: Fergal Daly <[email protected]>
Auto-Submit: Fergal Daly <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1634809}
---

diff --git a/chrome/browser/sync_file_system/local/local_file_change_tracker.cc b/chrome/browser/sync_file_system/local/local_file_change_tracker.cc
index 630cd06..a0f346e 100644
--- a/chrome/browser/sync_file_system/local/local_file_change_tracker.cc
+++ b/chrome/browser/sync_file_system/local/local_file_change_tracker.cc
@@ -80,7 +80,9 @@
     const base::FilePath& base_path,
     leveldb::Env* env_override,
     base::SequencedTaskRunner* file_task_runner)
-    : initialized_(false),
+    : base::RefCountedDeleteOnSequence<LocalFileChangeTracker>(
+          file_task_runner),
+      initialized_(false),
       file_task_runner_(file_task_runner),
       tracker_db_(std::make_unique<TrackerDB>(base_path, env_override)),
       current_change_seq_number_(0),
@@ -91,8 +93,25 @@
   tracker_db_.reset();
 }
 
+void LocalFileChangeTracker::AddRef() const {
+  base::RefCountedDeleteOnSequence<LocalFileChangeTracker>::AddRef();
+}
+
+void LocalFileChangeTracker::Release() const {
+  base::RefCountedDeleteOnSequence<LocalFileChangeTracker>::Release();
+}
+
+void LocalFileChangeTracker::Disable() {
+  base::AutoLock lock(is_disabled_lock_);
+  is_disabled_ = true;
+}
+
 void LocalFileChangeTracker::OnStartUpdate(const FileSystemURL& url) {
   DCHECK(file_task_runner_->RunsTasksInCurrentSequence());
+  base::AutoLock lock(is_disabled_lock_);
+  if (is_disabled_) {
+    return;
+  }
   if (changes_.contains(url) || demoted_changes_.contains(url)) {
     return;
   }
@@ -100,21 +119,36 @@
   MarkDirtyOnDatabase(url);
 }
 
-void LocalFileChangeTracker::OnEndUpdate(const FileSystemURL& url) {}
+void LocalFileChangeTracker::OnEndUpdate(const FileSystemURL& url) {
+  // If you add code in here, make sure to take the lock and check
+  // `is_disabled_`.
+}
 
 void LocalFileChangeTracker::OnCreateFile(const FileSystemURL& url) {
+  base::AutoLock lock(is_disabled_lock_);
+  if (is_disabled_) {
+    return;
+  }
   RecordChange(url, FileChange(FileChange::FILE_CHANGE_ADD_OR_UPDATE,
                                SYNC_FILE_TYPE_FILE));
 }
 
 void LocalFileChangeTracker::OnCreateFileFrom(const FileSystemURL& url,
                                               const FileSystemURL& src) {
+  base::AutoLock lock(is_disabled_lock_);
+  if (is_disabled_) {
+    return;
+  }
   RecordChange(url, FileChange(FileChange::FILE_CHANGE_ADD_OR_UPDATE,
                                SYNC_FILE_TYPE_FILE));
 }
 
 void LocalFileChangeTracker::OnMoveFileFrom(const FileSystemURL& url,
                                             const FileSystemURL& src) {
+  base::AutoLock lock(is_disabled_lock_);
+  if (is_disabled_) {
+    return;
+  }
   RecordChange(url, FileChange(FileChange::FILE_CHANGE_ADD_OR_UPDATE,
                                SYNC_FILE_TYPE_FILE));
   RecordChange(src,
@@ -122,21 +156,37 @@
 }
 
 void LocalFileChangeTracker::OnRemoveFile(const FileSystemURL& url) {
+  base::AutoLock lock(is_disabled_lock_);
+  if (is_disabled_) {
+    return;
+  }
   RecordChange(url, FileChange(FileChange::FILE_CHANGE_DELETE,
                                SYNC_FILE_TYPE_FILE));
 }
 
 void LocalFileChangeTracker::OnModifyFile(const FileSystemURL& url) {
+  base::AutoLock lock(is_disabled_lock_);
+  if (is_disabled_) {
+    return;
+  }
   RecordChange(url, FileChange(FileChange::FILE_CHANGE_ADD_OR_UPDATE,
                                SYNC_FILE_TYPE_FILE));
 }
 
 void LocalFileChangeTracker::OnCreateDirectory(const FileSystemURL& url) {
+  base::AutoLock lock(is_disabled_lock_);
+  if (is_disabled_) {
+    return;
+  }
   RecordChange(url, FileChange(FileChange::FILE_CHANGE_ADD_OR_UPDATE,
                                SYNC_FILE_TYPE_DIRECTORY));
 }
 
 void LocalFileChangeTracker::OnRemoveDirectory(const FileSystemURL& url) {
+  base::AutoLock lock(is_disabled_lock_);
+  if (is_disabled_) {
+    return;
+  }
   RecordChange(url, FileChange(FileChange::FILE_CHANGE_DELETE,
                                SYNC_FILE_TYPE_DIRECTORY));
 }
diff --git a/chrome/browser/sync_file_system/local/local_file_change_tracker.h b/chrome/browser/sync_file_system/local/local_file_change_tracker.h
index 0ce13ec..910f4d1 100644
--- a/chrome/browser/sync_file_system/local/local_file_change_tracker.h
+++ b/chrome/browser/sync_file_system/local/local_file_change_tracker.h
@@ -12,6 +12,8 @@
 
 #include "base/containers/circular_deque.h"
 #include "base/files/file_path.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/ref_counted_delete_on_sequence.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/synchronization/lock.h"
 #include "chrome/browser/sync_file_system/file_change.h"
@@ -38,8 +40,10 @@
 // Tracks local file changes for cloud-backed file systems.
 // All methods must be called on the file_task_runner given to the constructor.
 // Owned by FileSystemContext.
-class LocalFileChangeTracker : public storage::FileUpdateObserver,
-                               public storage::FileChangeObserver {
+class LocalFileChangeTracker
+    : public storage::FileUpdateObserver,
+      public storage::FileChangeObserver,
+      public base::RefCountedDeleteOnSequence<LocalFileChangeTracker> {
  public:
   // |file_task_runner| must be the one where the observee file operations run.
   // (So that we can make sure DB operations are done before actual update
@@ -51,9 +55,12 @@
   LocalFileChangeTracker(const LocalFileChangeTracker&) = delete;
   LocalFileChangeTracker& operator=(const LocalFileChangeTracker&) = delete;
 
-  ~LocalFileChangeTracker() override;
-
   // FileUpdateObserver overrides.
+  void AddRef() const override;
+  void Release() const override;
+
+  void Disable() override;
+
   void OnStartUpdate(const storage::FileSystemURL& url) override;
   void OnUpdate(const storage::FileSystemURL& url, int64_t delta) override {}
   void OnEndUpdate(const storage::FileSystemURL& url) override;
@@ -124,6 +131,13 @@
   }
 
  private:
+  friend class base::RefCountedDeleteOnSequence<LocalFileChangeTracker>;
+  friend class base::DeleteHelper<LocalFileChangeTracker>;
+
+  mutable base::Lock is_disabled_lock_;

diff --git a/content/browser/file_system_access/file_system_access_watcher_manager_unittest.cc b/content/browser/file_system_access/file_system_access_watcher_manager_unittest.cc
index e60ad3b..f71c5c6 100644
--- a/content/browser/file_system_access/file_system_access_watcher_manager_unittest.cc
+++ b/content/browser/file_system_access/file_system_access_watcher_manager_unittest.cc
@@ -323,7 +323,8 @@
     file_system_context_.reset();
     chrome_blob_context_.reset();
     task_environment_.RunUntilIdle();
-    EXPECT_TRUE(dir_.Delete());
+    // On Windows, a synchronous delete of the directory can fail.
+    GetDeleteFileCallback(dir_.GetPath()).Run();
   }
 
   bool CreateDirectory(const base::FilePath& full_path) {
diff --git a/storage/browser/file_system/sandbox_file_system_backend_delegate_unittest.cc b/storage/browser/file_system/sandbox_file_system_backend_delegate_unittest.cc
index a372ef0..fa83ccf 100644
--- a/storage/browser/file_system/sandbox_file_system_backend_delegate_unittest.cc
+++ b/storage/browser/file_system/sandbox_file_system_backend_delegate_unittest.cc
@@ -24,6 +24,58 @@
 
 namespace {
 
+class MockFileChangeObserver : public FileChangeObserver {
+ public:
+  MockFileChangeObserver() = default;
+  ~MockFileChangeObserver() override = default;
+
+  void AddRef() const override {}
+  void Release() const override {}
+
+  void Disable() override { is_disabled_ = true; }
+
+  void OnCreateFile(const FileSystemURL& url) override {
+    if (is_disabled_) {
+      return;
+    }
+  }
+  void OnCreateFileFrom(const FileSystemURL& url,
+                        const FileSystemURL& src) override {
+    if (is_disabled_) {
+      return;
+    }
+  }
+  void OnMoveFileFrom(const FileSystemURL& url,
+                      const FileSystemURL& src) override {
+    if (is_disabled_) {
+      return;
+    }
+  }
+  void OnRemoveFile(const FileSystemURL& url) override {
+    if (is_disabled_) {
+      return;
+    }
+  }
+  void OnModifyFile(const FileSystemURL& url) override {
+    if (is_disabled_) {
+      return;
+    }
+  }
+  void OnCreateDirectory(const FileSystemURL& url) override {
+    if (is_disabled_) {
+      return;
+    }
+  }
+  void OnRemoveDirectory(const FileSystemURL& url) override {
+    if (is_disabled_) {
+      return;
+    }
+  }
+
+ private:
+  bool is_disabled_ = false;
+};
+
 FileSystemURL CreateFileSystemURL(const char* path) {
   return FileSystemURL::CreateForTest(
       blink::StorageKey::CreateFromStringForTesting("http://foo/"),
@@ -34,6 +86,8 @@
 
 class SandboxFileSystemBackendDelegateTest : public testing::Test {
  protected:
+  std::unique_ptr<SandboxFileSystemBackendDelegate> delegate_;
+
   void SetUp() override {
     ASSERT_TRUE(data_dir_.CreateUniqueTempDir());
     quota_manager_proxy_ = base::MakeRefCounted<MockQuotaManagerProxy>(
@@ -80,7 +134,6 @@
   base::ScopedTempDir data_dir_;
   base::test::TaskEnvironment task_environment_;
   scoped_refptr<MockQuotaManagerProxy> quota_manager_proxy_;
-  std::unique_ptr<SandboxFileSystemBackendDelegate> delegate_;
 
   int callback_count_ = 0;
   base::File::Error last_error_ = base::File::FILE_OK;
diff --git a/storage/browser/test/mock_file_change_observer.cc b/storage/browser/test/mock_file_change_observer.cc
index b9808ec..752d33a 100644
--- a/storage/browser/test/mock_file_change_observer.cc
+++ b/storage/browser/test/mock_file_change_observer.cc
@@ -19,25 +19,39 @@
 
 // static
 ChangeObserverList MockFileChangeObserver::CreateList(
-    MockFileChangeObserver* observer) {
+    scoped_refptr<MockFileChangeObserver> observer) {
   ChangeObserverList list;
   return list.AddObserver(
-      observer, base::SingleThreadTaskRunner::GetCurrentDefault().get());
+      std::move(observer),
+      base::SingleThreadTaskRunner::GetCurrentDefault().get());
+}
+
+void MockFileChangeObserver::Disable() {
+  is_disabled_ = true;
 }
 
 void MockFileChangeObserver::OnCreateFile(const FileSystemURL& url) {
+  if (is_disabled_) {
+    return;
+  }
   create_file_count_++;
   changed_urls_.insert(url);
 }
 
 void MockFileChangeObserver::OnCreateFileFrom(const FileSystemURL& url,
                                               const FileSystemURL& src) {
+  if (is_disabled_) {
+    return;
+  }
   create_file_from_count_++;
   changed_urls_.insert(url);
 }
 
 void MockFileChangeObserver::OnMoveFileFrom(const FileSystemURL& url,
                                             const FileSystemURL& src) {
+  if (is_disabled_) {
+    return;
+  }
   create_file_from_count_++;
   remove_file_count_++;
   changed_urls_.insert(url);
@@ -45,21 +59,33 @@
 }
 
 void MockFileChangeObserver::OnRemoveFile(const FileSystemURL& url) {
+  if (is_disabled_) {
+    return;
+  }
   remove_file_count_++;
   changed_urls_.insert(url);
 }
 
 void MockFileChangeObserver::OnModifyFile(const FileSystemURL& url) {
+  if (is_disabled_) {
+    return;
+  }
   modify_file_count_++;
   changed_urls_.insert(url);
 }
 
 void MockFileChangeObserver::OnCreateDirectory(const FileSystemURL& url) {
+  if (is_disabled_) {
+    return;
+  }
   create_directory_count_++;
   changed_urls_.insert(url);
 }
 
 void MockFileChangeObserver::OnRemoveDirectory(const FileSystemURL& url) {
+  if (is_disabled_) {
+    return;
+  }
   remove_directory_count_++;
   changed_urls_.insert(url);
 }
diff --git a/storage/browser/test/mock_file_change_observer.h b/storage/browser/test/mock_file_change_observer.h
index c8bc562..a9b05db6 100644
--- a/storage/browser/test/mock_file_change_observer.h
+++ b/storage/browser/test/mock_file_change_observer.h
@@ -23,9 +23,13 @@
   ~MockFileChangeObserver() override;
 
   // Creates a ChangeObserverList which only contains given |observer|.
-  static ChangeObserverList CreateList(MockFileChangeObserver* observer);
+  static ChangeObserverList CreateList(
+      scoped_refptr<MockFileChangeObserver> observer);
 
   // FileChangeObserver overrides.
+  void AddRef() const override {}
+  void Release() const override {}
+
   void OnCreateFile(const FileSystemURL& url) override;
   void OnCreateFileFrom(const FileSystemURL& url,
                         const FileSystemURL& src) override;
@@ -36,6 +40,8 @@
   void OnCreateDirectory(const FileSystemURL& url) override;
   void OnRemoveDirectory(const FileSystemURL& url) override;
 
+  void Disable() override;
+
   void ResetCount() {
     create_file_count_ = 0;
     create_file_from_count_ = 0;
@@ -101,6 +107,7 @@
   }
 
  private:
+  bool is_disabled_ = false;
   FileSystemURLSet changed_urls_;
 
   int create_file_count_;
diff --git a/storage/browser/test/mock_file_update_observer.cc b/storage/browser/test/mock_file_update_observer.cc
index 6421d16b..7689855 100644
--- a/storage/browser/test/mock_file_update_observer.cc
+++ b/storage/browser/test/mock_file_update_observer.cc
@@ -15,10 +15,17 @@
 
 // static
 UpdateObserverList MockFileUpdateObserver::CreateList(
-    MockFileUpdateObserver* observer) {
+    scoped_refptr<MockFileUpdateObserver> observer) {
   UpdateObserverList list;
   return list.AddObserver(
-      observer, base::SingleThreadTaskRunner::GetCurrentDefault().get());
+      std::move(observer),
+      base::SingleThreadTaskRunner::GetCurrentDefault().get());
+}
+
+void MockFileUpdateObserver::Disable() {
+  start_update_count_.clear();
+  end_update_count_.clear();
+  is_ready_ = false;
 }
 
 void MockFileUpdateObserver::OnStartUpdate(const FileSystemURL& url) {
diff --git a/storage/browser/test/mock_file_update_observer.h b/storage/browser/test/mock_file_update_observer.h
index 32372ca..133bcc8 100644
--- a/storage/browser/test/mock_file_update_observer.h
+++ b/storage/browser/test/mock_file_update_observer.h
@@ -27,20 +27,20 @@
   ~MockFileUpdateObserver() override;
 
   // Creates a ChangeObserverList which only contains given |observer|.
-  static UpdateObserverList CreateList(MockFileUpdateObserver* observer);
+  static UpdateObserverList CreateList(
+      scoped_refptr<MockFileUpdateObserver> observer);
 
   // FileUpdateObserver overrides.
+  void AddRef() const override {}
+  void Release() const override {}
+
   void OnStartUpdate(const FileSystemURL& url) override;
   void OnUpdate(const FileSystemURL& url, int64_t delta) override;
   void OnEndUpdate(const FileSystemURL& url) override;
 
   void Enable() { is_ready_ = true; }
 
-  void Disable() {
-    start_update_count_.clear();
-    end_update_count_.clear();
-    is_ready_ = false;
-  }
+  void Disable() override;
 
  private:
   std::map<FileSystemURL, int, FileSystemURL::Comparator> start_update_count_;
diff --git a/storage/browser/test/test_file_system_backend.cc b/storage/browser/test/test_file_system_backend.cc
index 3f5d47a..97d0171 100644
--- a/storage/browser/test/test_file_system_backend.cc
+++ b/storage/browser/test/test_file_system_backend.cc
@@ -103,14 +103,31 @@
   }
 
   // FileUpdateObserver overrides.
-  void OnStartUpdate(const FileSystemURL& url) override {}
+  void AddRef() const override {}
+  void Release() const override {}
+
+  void Disable() override { is_disabled_ = true; }
+
+  void OnStartUpdate(const FileSystemURL& url) override {
+    if (is_disabled_) {
+      return;
+    }
+  }
   void OnUpdate(const FileSystemURL& url, int64_t delta) override {
+    if (is_disabled_) {
+      return;
+    }
     usage_ += delta;
   }
-  void OnEndUpdate(const FileSystemURL& url) override {}
+  void OnEndUpdate(const FileSystemURL& url) override {
+    if (is_disabled_) {
+      return;
+    }
+  }
 
  private:
   int64_t usage_;
+  bool is_disabled_ = false;
... (truncated)