UAF due to cross-thread destruction of worker DeferredPromise in WebLockManager::query()

cdumez · JonWBedard · commit 7a5ee54cd51a · 2026-06-30T14:00:25.000-07:00
https://bugs.webkit.org/show_bug.cgi?id=312456 rdar://174652399 Reviewed by Ryosuke Niwa. WebLockManager::MainThreadBridge::query() was taking in a CompletionHandler but it may sometimes fail to call its completion handler. This happened when the worker thread is exiting, causing `ScriptExecutionContext::ensureOnContextThread()` to fail. Not calling the completion handler is bad but what's worse is that the completion handler would end up getting destroyed on the main thread. The completion handler was capturing a promise from the worker thread, which led to security bugs. To address the issue: 1. Have WebLockManager::MainThreadBridge::query() take in a Function instead of a CompletionHandler given that it cannot always call its callback. 2. Have WebLockManager store the promise in a HashMap and only capture a promise identifier in the MainThreadBridge::query() lambda instead of the promise itself. This pattern was already used for other promises in this class. Test: workers/weblock-manager-query-crash.html * LayoutTests/workers/weblock-manager-query-crash-expected.txt: Added. * LayoutTests/workers/weblock-manager-query-crash.html: Added. * Source/WebCore/Modules/web-locks/WebLockManager.cpp: (WebCore::WebLockManager::MainThreadBridge::abortLockRequest): (WebCore::WebLockManager::MainThreadBridge::query): (WebCore::WebLockManager::query): (WebCore::WebLockManager::clientIsGoingAway): * Source/WebCore/Modules/web-locks/WebLockManager.h: Originally-landed-as: 305413.688@safari-7624-branch (0b964ced2532). rdar://180436136 Canonical link: https://commits.webkit.org/316195@main
diff --git a/LayoutTests/workers/weblock-manager-query-crash-expected.txt b/LayoutTests/workers/weblock-manager-query-crash-expected.txt
@@ -0,0 +1 @@
+This test passes if it doesn't crash.
diff --git a/LayoutTests/workers/weblock-manager-query-crash.html b/LayoutTests/workers/weblock-manager-query-crash.html
@@ -0,0 +1,42 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+globalThis.testRunner?.dumpAsText();
+globalThis.testRunner?.waitUntilDone();
+
+const workerSrc = `
+  for (let i = 0; i < 500; i++) self.navigator.locks.query();
+  self.postMessage("ready");
+  for (let i = 0; i < 500; i++) self.navigator.locks.query();
+`;
+const blob = new Blob([workerSrc], {type: "text/javascript"});
+const url = URL.createObjectURL(blob);
+
+function log(msg) {
+    if (globalThis.$vm) $vm.print(msg);
+    console.log(msg);
+}
+
+let iterations = 0;
+function spawn() {
+    if (iterations++ > 50) {
+        globalThis.testRunner?.notifyDone();
+        return;
+    }
+    let w = new Worker(url);
+    w.onmessage = () => {
+        w.terminate();
+        w = null;
+        if (globalThis.$vm) $vm.gc();
+        setTimeout(spawn, 0);
+    };
+}
+
+window.onload = spawn;
+</script>
+</head>
+<body>
+This test passes if it doesn't crash.
+</body>
+</html>
diff --git a/Source/WebCore/Modules/web-locks/WebLockManager.cpp b/Source/WebCore/Modules/web-locks/WebLockManager.cpp
@@ -96,8 +96,8 @@ class WebLockManager::MainThreadBridge : public ThreadSafeRefCounted<MainThreadB
 
     void requestLock(WebLockIdentifier, const String& name, const Options&, Function<void(bool)>&&, Function<void()>&& lockStolenHandler);
     void releaseLock(WebLockIdentifier, const String& name);
-    void abortLockRequest(WebLockIdentifier, const String& name, CompletionHandler<void(bool)>&&);
-    void query(CompletionHandler<void(Snapshot&&)>&&);
+    void abortLockRequest(WebLockIdentifier, const String& name, Function<void(bool)>&&);
+    void query(Function<void(Snapshot&&)>&&);
     void clientIsGoingAway();
 
 private:
@@ -137,23 +137,23 @@ void WebLockManager::MainThreadBridge::releaseLock(WebLockIdentifier lockIdentif
     });
 }
 
-void WebLockManager::MainThreadBridge::abortLockRequest(WebLockIdentifier lockIdentifier, const String& name, CompletionHandler<void(bool)>&& completionHandler)
+void WebLockManager::MainThreadBridge::abortLockRequest(WebLockIdentifier lockIdentifier, const String& name, Function<void(bool)>&& callback)
 {
-    callOnMainThread([this, protectedThis = Ref { *this }, lockIdentifier, name = crossThreadCopy(name), completionHandler = WTF::move(completionHandler)]() mutable {
-        WebLockRegistry::singleton().abortLockRequest(m_sessionID, m_clientOrigin, lockIdentifier, m_clientID, name, [clientID = m_clientID, completionHandler = WTF::move(completionHandler)](bool wasAborted) mutable {
-            ScriptExecutionContext::ensureOnContextThread(clientID, [completionHandler = WTF::move(completionHandler), wasAborted](auto&) mutable {
-                completionHandler(wasAborted);
+    callOnMainThread([this, protectedThis = Ref { *this }, lockIdentifier, name = crossThreadCopy(name), callback = WTF::move(callback)]() mutable {
+        WebLockRegistry::singleton().abortLockRequest(m_sessionID, m_clientOrigin, lockIdentifier, m_clientID, name, [clientID = m_clientID, callback = WTF::move(callback)](bool wasAborted) mutable {
+            ScriptExecutionContext::ensureOnContextThread(clientID, [callback = WTF::move(callback), wasAborted](auto&) mutable {
+                callback(wasAborted);
             });
         });
     });
 }
 
-void WebLockManager::MainThreadBridge::query(CompletionHandler<void(Snapshot&&)>&& completionHandler)
+void WebLockManager::MainThreadBridge::query(Function<void(Snapshot&&)>&& callback)
 {
-    callOnMainThread([this, protectedThis = Ref { *this }, completionHandler = WTF::move(completionHandler)]() mutable {
-        WebLockRegistry::singleton().snapshot(m_sessionID, m_clientOrigin, [clientID = m_clientID, completionHandler = WTF::move(completionHandler)](Snapshot&& snapshot) mutable {
-            ScriptExecutionContext::ensureOnContextThread(clientID, [completionHandler = WTF::move(completionHandler), snapshot = crossThreadCopy(snapshot)](auto&) mutable {
-                completionHandler(WTF::move(snapshot));
+    callOnMainThread([this, protectedThis = Ref { *this }, callback = WTF::move(callback)]() mutable {
+        WebLockRegistry::singleton().snapshot(m_sessionID, m_clientOrigin, [clientID = m_clientID, callback = WTF::move(callback)](Snapshot&& snapshot) mutable {
+            ScriptExecutionContext::ensureOnContextThread(clientID, [callback = WTF::move(callback), snapshot = crossThreadCopy(snapshot)](auto&) mutable {
+                callback(WTF::move(snapshot));
             });
         });
     });
@@ -322,11 +322,17 @@ void WebLockManager::query(Ref<DeferredPromise>&& promise)
         return;
     }
 
-    m_mainThreadBridge->query([weakThis = WeakPtr { *this }, promise = WTF::move(promise)](Snapshot&& snapshot) mutable {
+    auto promiseIdentifier = WebLockIdentifier::generate();
+    m_queryPromises.add(promiseIdentifier, WTF::move(promise));
+    m_mainThreadBridge->query([weakThis = WeakPtr { *this }, promiseIdentifier](Snapshot&& snapshot) mutable {
         RefPtr protectedThis = weakThis.get();
         if (!protectedThis)
             return;
 
+        auto promise = protectedThis->m_queryPromises.take(promiseIdentifier);
+        if (!promise)
+            return;
+
         queueTaskKeepingObjectAlive(*protectedThis, TaskSource::DOMManipulation, [promise = WTF::move(promise), snapshot = WTF::move(snapshot)](auto&) mutable {
             promise->resolve<IDLDictionary<Snapshot>>(WTF::move(snapshot));
         });
@@ -373,16 +379,16 @@ void WebLockManager::stop()
 
 void WebLockManager::clientIsGoingAway()
 {
-    if (m_pendingRequests.isEmpty() && m_releasePromises.isEmpty())
-        return;
-
     for (auto& request : m_pendingRequests.values())
         request.removeSignalAlgorithm();
 
-    // Reject all pending promises before clearing
-    for (Ref promise : m_releasePromises.values())
-        promise->reject(ExceptionCode::AbortError, "Promise was rejected because the browsing context is going away"_s);
-
+    // Reject all pending promises before clearing.
+    auto releasePromises = std::exchange(m_releasePromises, { });
+    for (auto& promise : releasePromises.values())
+        protect(promise)->reject(ExceptionCode::AbortError, "Promise was rejected because the browsing context is going away"_s);
+    auto queryPromises = std::exchange(m_queryPromises, { });
+    for (auto& promise : queryPromises.values())
+        protect(promise)->reject(ExceptionCode::AbortError, "Promise was rejected because the browsing context is going away"_s);
     m_pendingRequests.clear();
     m_releasePromises.clear();
 
@@ -392,7 +398,7 @@ void WebLockManager::clientIsGoingAway()
 
 bool WebLockManager::virtualHasPendingActivity() const
 {
-    return !m_pendingRequests.isEmpty() || !m_releasePromises.isEmpty();
+    return !m_pendingRequests.isEmpty() || !m_releasePromises.isEmpty() || !m_queryPromises.isEmpty();
 }
 
 void WebLockManager::suspend(ReasonForSuspension reason)
diff --git a/Source/WebCore/Modules/web-locks/WebLockManager.h b/Source/WebCore/Modules/web-locks/WebLockManager.h
@@ -83,6 +83,8 @@ class WebLockManager : public RefCounted<WebLockManager>, public ActiveDOMObject
 
     struct LockRequest;
     HashMap<WebLockIdentifier, LockRequest> m_pendingRequests;
+
+    HashMap<WebLockIdentifier, Ref<DeferredPromise>> m_queryPromises;
 };
 
 } // namespace WebCore
