From cb6f7a40ec020a093297169fa0bf3fa0afa556a6 Mon Sep 17 00:00:00 2001
From: Ming-Ying Chung <mych@chromium.org>
Date: Thu, 30 Apr 2026 23:20:41 -0700
Subject: [PATCH] [FSA] Update FSEvents stream to use ref-counted context

A vulnerability existed in the macOS implementation of the File System
Access API's directory watcher `FilePathWatcherFSEvents`: The `Cancel()`
method lacked a synchronization barrier, allowing the object to be
destroyed on a ThreadPool sequence while a MacOS callback
`FSEventsCallback` concurrently accesses its raw pointer on a
`libdispatch` thread.

This issue exists since the `content/` fork of `FilePathWatcherFSEvents`
diverged from the original `base/ `implementation after the refactor in
CL [1] and [2], by omitting the `dispatch_sync()` barrier in `Cancel()`.

While re-introducing `dispatch_*` in `Cancel()` and in other
`UpdateEventStream()` related methods would ensure any currently
executing callbacks on the `libdispatch` queue complete before the
watcher object is destroyed, it would also change the behavior of
`Watch()` to async, as stated in [1].

This CL instead uses a ref-counted context object `FSEventsContext` to
pass to the MacOS callback. The context holds the `TaskRunner` and a
`WeakPtr` to the watcher. The MacOS manages its lifetime via
retain/release callbacks in `FSEventStreamContext`. This ensures that
the callback can safely access the `TaskRunner` even if the watcher is
destroyed, without needing a `dispatch_sync` barrier.

[1]: https://crrev.com/c/5584463
[2]: https://crrev.com/c/4814677


TESTED=vpython3 tools/utr -p chromium -B try -b mac-rel -t content_unittests compile-and-test -- --gtest_filter="FilePathWatcherTest.*" --gtest_repeat=1 && vpython3 tools/utr -p chromium -B try -b mac-rel -t content_browsertests compile-and-test -- --gtest_filter="*FileSystemAccessObserverBrowserTest*" --gtest_repeat=1


Bug: 495782021
Change-Id: I939963fcd416ee25d191c28ee987b4ea62b3fb31
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7795189
Commit-Queue: Ming-Ying Chung <mych@chromium.org>
Reviewed-by: Mingyu Lei <leimy@chromium.org>
Reviewed-by: Fergal Daly <fergal@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1623723}
---

diff --git a/content/browser/file_system_access/file_path_watcher/file_path_watcher.h b/content/browser/file_system_access/file_path_watcher/file_path_watcher.h
index 89dd6d9..7b16e7c3 100644
--- a/content/browser/file_system_access/file_path_watcher/file_path_watcher.h
+++ b/content/browser/file_system_access/file_path_watcher/file_path_watcher.h
@@ -262,6 +262,12 @@
   base::Lock& GetWatchThreadLockForTest();
 #endif
 
+#if BUILDFLAG(IS_MAC)
+  // Creates a watcher with a test hook for FSEvents.
+  static std::unique_ptr<FilePathWatcher> CreateWithFSEventsHookForTesting(
+      base::RepeatingClosure hook);
+#endif  // BUILDFLAG(IS_MAC)
+
   static base::AutoReset<size_t> SetQuotaLimitForTesting(
       size_t quota_limit_override) {
     return base::AutoReset<size_t>(&quota_limit_override_for_testing_,
diff --git a/content/browser/file_system_access/file_path_watcher/file_path_watcher_fsevents.cc b/content/browser/file_system_access/file_path_watcher/file_path_watcher_fsevents.cc
index 5ae9f1a..04127239 100644
--- a/content/browser/file_system_access/file_path_watcher/file_path_watcher_fsevents.cc
+++ b/content/browser/file_system_access/file_path_watcher/file_path_watcher_fsevents.cc
@@ -12,12 +12,15 @@
 #include "base/apple/foundation_util.h"
 #include "base/apple/scoped_cftyperef.h"
 #include "base/check.h"
+#include "base/check_is_test.h"
 #include "base/compiler_specific.h"
 #include "base/containers/span.h"
 #include "base/files/file_util.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback_helpers.h"
 #include "base/lazy_instance.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/task/sequenced_task_runner.h"
@@ -29,6 +32,42 @@
 
 namespace {
 
+// Ref-counted context to hold state needed by the static `FSEventsCallback`,
+// which is invoked by MacOS FSEvents API on a background libdispatch thread.
+struct FSEventsContext : public base::RefCountedThreadSafe<FSEventsContext> {
+  FSEventsContext(scoped_refptr<base::SequencedTaskRunner> task_runner,
+                  base::WeakPtr<FilePathWatcherFSEvents> weak_ptr,
+                  base::RepeatingClosure test_callback)
+      : task_runner(std::move(task_runner)),
+        weak_ptr(std::move(weak_ptr)),
+        test_callback(std::move(test_callback)) {}
+
+  // C-style wrappers required by `FSEventStreamContext`
+  static const void* RetainContext(const void* info) {
+    static_cast<const FSEventsContext*>(info)->AddRef();
+    return info;
+  }
+  static void ReleaseContext(const void* info) {
+    static_cast<const FSEventsContext*>(info)->Release();
+  }
+
+  // Holds reference to task_runner. Since the FSEventsCallback runs on an
+  // external OS thread, we must allow it to use this TaskRunner to post a task
+  // back to the original Chromium sequence where the FilePathWatcher resides.
+  const scoped_refptr<base::SequencedTaskRunner> task_runner;
+  // Holds weakptr to the `FilePathWatcherFSEvents` object. It may be destroyed
+  // on the Chromium sequence while the FSEventsCallback is running or while a
+  // task is pending.
+  // Accessing via this ensures the task `OnFilePathsChanged` will only execute
+  // if the watcher object is still valid.
+  const base::WeakPtr<FilePathWatcherFSEvents> weak_ptr;
+  const base::RepeatingClosure test_callback;
+
+ private:
+  friend class base::RefCountedThreadSafe<FSEventsContext>;
+  ~FSEventsContext() = default;
+};
+
 // The latency parameter passed to FSEventsStreamCreate().
 const CFAbsoluteTime kEventLatencySeconds = 0.7;
 
@@ -146,8 +185,20 @@
     void* event_paths,
     const FSEventStreamEventFlags flags[],
     const FSEventStreamEventId event_ids[]) {
-  FilePathWatcherFSEvents* watcher =
-      reinterpret_cast<FilePathWatcherFSEvents*>(event_watcher);
+  // The MacOS framework holds a reference to the ref-counted `FSEventsContext`,
+  // created in `UpdateEventStream()`, as long as the `FSEventStream` is active.
+  // The context connects this static callback to the `FilePathWatcherFSEvents`
+  // object via a WeakPtr in `UpdateEventStream()`.
+  //
+  // If the `FilePathWatcherFSEvents` object is destroyed concurrently on the
+  // Chromium thread, it releases the stream, which causes the OS to drop its
+  // reference to the context.
+  //
+  // By holding a local `scoped_refptr` here, we guarantee that the
+  // `FSEventsContext` object stays alive until this static function returns,
+  // even if the `FilePathWatcherFSEvents` object is deleted mid-flight.
+  scoped_refptr<FSEventsContext> context =
+      base::WrapRefCounted(static_cast<FSEventsContext*>(event_watcher));
   bool is_root_changed_event = false;
 
   // The `root_changed_at` value represents the highest-numbered FSEvents event
@@ -209,11 +260,22 @@
     }
     events[event_id] = ChangeEvent(event_flags, event_path, std::nullopt);
   }
-  watcher->task_runner()->PostTask(
-      FROM_HERE,
-      base::BindOnce(&FilePathWatcherFSEvents::OnFilePathsChanged,
-                     watcher->weak_factory_.GetWeakPtr(), is_root_changed_event,
-                     root_change_at, std::move(events)));
+
+  if (context->test_callback) {
+    CHECK_IS_TEST();
+    context->test_callback.Run();
+  }
+
+  // Note: It is safe to pass `context->weak_ptr` across threads here.
+  // `base::BindOnce` merely copies the `WeakPtr` into its bound state without
+  // dereferencing it or checking its validity on this background thread.
+  // The actual validity check and dereference will only occur when the
+  // `OnFilePathsChanged()` is executed on the destination sequence, the origin
+  // task runner, which is the correct and safe sequence for that `WeakPtr`.
+  context->task_runner->PostTask(
+      FROM_HERE, base::BindOnce(&FilePathWatcherFSEvents::OnFilePathsChanged,
+                                context->weak_ptr, is_root_changed_event,
+                                root_change_at, std::move(events)));
 }
 
 void FilePathWatcherFSEvents::OnFilePathsChanged(
@@ -259,12 +321,16 @@
       CFArrayCreate(NULL, reinterpret_cast<const void**>(paths_array),
                     std::size(paths_array), &kCFTypeArrayCallBacks));
 
+  auto context_obj = base::MakeRefCounted<FSEventsContext>(
+      task_runner(), weak_factory_.GetWeakPtr(),
+      on_fsevents_callback_for_testing_);
+
   FSEventStreamContext context;
   context.version = 0;
-  context.info = this;
-  context.retain = NULL;
-  context.release = NULL;
-  context.copyDescription = NULL;
+  context.info = context_obj.get();
+  context.retain = FSEventsContext::RetainContext;
+  context.release = FSEventsContext::ReleaseContext;
+  context.copyDescription = nullptr;
 
   // Ensure that if more `FSEventStreamCreate` calls are added that
   // `kNumberOfFSEventStreamCreateCalls` is updated to match.
diff --git a/content/browser/file_system_access/file_path_watcher/file_path_watcher_fsevents.h b/content/browser/file_system_access/file_path_watcher/file_path_watcher_fsevents.h
index d2a20930..a7d64b8 100644
--- a/content/browser/file_system_access/file_path_watcher/file_path_watcher_fsevents.h

diff --git a/content/browser/file_system_access/file_path_watcher/file_path_watcher_unittest.cc b/content/browser/file_system_access/file_path_watcher/file_path_watcher_unittest.cc
index 09352ea..9eac18b 100644
--- a/content/browser/file_system_access/file_path_watcher/file_path_watcher_unittest.cc
+++ b/content/browser/file_system_access/file_path_watcher/file_path_watcher_unittest.cc
@@ -22,6 +22,7 @@
 #include "base/sequence_checker.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/to_string.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/task/bind_post_task.h"
 #include "base/task/sequenced_task_runner.h"
 #include "base/task/single_thread_task_runner.h"
@@ -34,6 +35,7 @@
 #include "base/test/test_timeouts.h"
 #include "base/thread_annotations.h"
 #include "base/threading/thread.h"
+#include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
 #include "content/browser/file_system_access/features.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -988,6 +990,80 @@
   delegate.RunUntilEventsMatch(event_expecter);
 }
 
+#if BUILDFLAG(IS_MAC)
+// Tests that deleting a FilePathWatcher while an FSEvents callback is pending
+// or executing does not result in a UAF.
+//
+// This test uses a hook to deterministically force the race condition. The
+// callback hook blocks the FSEvents thread while the watcher is being
+// destroyed. If the destruction process were to wait for the FSEvents thread to
+// complete, e.g., via a thread join, a deadlock would occur. This test
+// sequence is explicitly ordered to trigger such a deadlock if the
+// implementation is vulnerable.
+//
+// It operates as follows:
+// 1. Create a FilePathWatcher and set a testing hook via
+//    `FilePathWatcher::CreateWithFSEventsHookForTesting()`. This hook will
+//    pause the OS callback execution using a `base::WaitableEvent()`.
+// 2. Start watching a file using `SetupWatch()` and trigger a file system event
+//    by writing to it with `WriteFile()`.
+// 3. Wait for the OS to invoke `FSEventsCallback()` on its background thread
+//    and hit our hook by calling `hook_called_event.Wait()`.
+// 4. While `FSEventsCallback()` is paused in mid-flight on the OS thread,
+//    destroy the `FilePathWatcher` instance on the main test sequence by
+//    calling `watcher.reset()`.
+// 5. Unblock the paused OS callback by calling
+//    `watcher_destroyed_event.Signal()`, allowing it to proceed to post tasks.
+//
+// Expected behavior:
+// - Without fix: The resumed `FSEventsCallback()` attempts to access the
+//   destroyed watcher's raw pointer or `TaskRunner`, triggering a crash.
+// - With fix: The callback accesses the ref-counted `FSEventsContext`,
+//   which the OS keeps alive, sees that the `WeakPtr` to the watcher is now
+//   invalidated, and safely aborts.
+//
+// See also https://crbug.com/495782021.
+TEST_F(FilePathWatcherTest, DeleteWhileFSEventsPendingDeterministic) {
+  auto delegate = std::make_unique<TestDelegate>();
+
+  base::WaitableEvent hook_called_event;
+  base::WaitableEvent watcher_destroyed_event;
+
+  // Create the watcher and set the hook all in one go.
+  auto watcher =
+      FilePathWatcher::CreateWithFSEventsHookForTesting(base::BindRepeating(
+          [](base::WaitableEvent* hook_called,
+             base::WaitableEvent* watcher_destroyed) {
+            VLOG(1) << "Triggering FSEventsHook callback";
+            hook_called->Signal();
+            watcher_destroyed->Wait();
+          },
+          &hook_called_event, &watcher_destroyed_event));
+
+  ASSERT_TRUE(SetupWatch(test_file(), watcher.get(), delegate.get(),
+                         FilePathWatcher::Type::kRecursive));
+
+  ASSERT_TRUE(WriteFile(test_file(), "content"));
+
+  // Wait for the hook to be called on the FSEvents queue.
+  VLOG(1) << "Waiting for FSEventsHook callback";
+  base::ScopedAllowBaseSyncPrimitivesForTesting allow_blocking;
+  hook_called_event.Wait();
+
+  // Concurrently destroy the watcher.
+  VLOG(1) << "Destroying watcher";
+  watcher.reset();
+
+  // Unblock the FSEvents callback so it can proceed to PostTask.
+  VLOG(1) << "Unblocking FSEvents callback";
+  watcher_destroyed_event.Signal();
+
+  SpinEventLoopForABit();
+
+  // Expect no crashes.
+}
+#endif  // BUILDFLAG(IS_MAC)
+
 #if BUILDFLAG(IS_WIN)
 TEST_F(FilePathWatcherTest, WindowsBufferOverflow) {
   FilePathWatcher watcher;