Changeset 248024 in webkit

Timestamp:

Jul 30, 2019, 4:52:55 PM (6 years ago)

Author:

rniwa@webkit.org

Message:

WorkerGlobalScope::wrapCryptoKey/unwrapCryptoKey should use local heap objects for replies
​https://bugs.webkit.org/show_bug.cgi?id=200179
<rdar://problem/52334658>

Reviewed by Brent Fulgham.

Based on the patch by Jiewen Tan.

WorkerGlobalScope::wrapCryptoKey and WorkerGlobalScope::unwrapCryptoKey had a bug that they could exit
the function before the main thread had finished writing to the result vector passed in to these functions
when the worker's runloop receives MessageQueueTerminated before the main thread finishes writing.

Fixed the bug by creating a new temporary Vector inside a ThreadSafeRefCounted object shared between
the main thread and the worker thread, which extends the lifetime of the Vector until when the worker thread
receives the result or when the main thread finishes writing to the Vector, whichever happens last.

Unfortunately no new tests since there is no reproducible test case, and this crash is highly racy.

• workers/WorkerGlobalScope.cpp:

(WebCore::CryptoBufferContainer): Added.
(WebCore::CryptoBufferContainer::create): Added.
(WebCore::CryptoBufferContainer::buffer): Added.
(WebCore::WorkerGlobalScope::wrapCryptoKey):
(WebCore::WorkerGlobalScope::unwrapCryptoKey):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• workers/WorkerGlobalScope.cpp (modified) (4 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-07-29  Ryosuke Niwa  <rniwa@webkit.org>
+
+        WorkerGlobalScope::wrapCryptoKey/unwrapCryptoKey should use local heap objects for replies
+        https://bugs.webkit.org/show_bug.cgi?id=200179
+        <rdar://problem/52334658>
+
+        Reviewed by Brent Fulgham.
+
+        Based on the patch by Jiewen Tan.
+
+        WorkerGlobalScope::wrapCryptoKey and WorkerGlobalScope::unwrapCryptoKey had a bug that they could exit
+        the function before the main thread had finished writing to the result vector passed in to these functions
+        when the worker's runloop receives MessageQueueTerminated before the main thread finishes writing.
+
+        Fixed the bug by creating a new temporary Vector inside a ThreadSafeRefCounted object shared between
+        the main thread and the worker thread, which extends the lifetime of the Vector until when the worker thread
+        receives the result or when the main thread finishes writing to the Vector, whichever happens last.
+
+        Unfortunately no new tests since there is no reproducible test case, and this crash is highly racy.
+
+        * workers/WorkerGlobalScope.cpp:
+        (WebCore::CryptoBufferContainer): Added.
+        (WebCore::CryptoBufferContainer::create): Added.
+        (WebCore::CryptoBufferContainer::buffer): Added.
+        (WebCore::WorkerGlobalScope::wrapCryptoKey):
+        (WebCore::WorkerGlobalScope::unwrapCryptoKey):
+
 2019-07-30  Saam Barati  <sbarati@apple.com>
 

trunk/Source/WebCore/workers/WorkerGlobalScope.cpp

@@ -386 +386 @@
 #if ENABLE(WEB_CRYPTO)
 
+class CryptoBufferContainer : public ThreadSafeRefCounted<CryptoBufferContainer> {
+public:
+    static Ref<CryptoBufferContainer> create() { return adoptRef(*new CryptoBufferContainer); }
+    Vector<uint8_t>& buffer() { return m_buffer; }
+
+private:
+    Vector<uint8_t> m_buffer;
+};
+
 bool WorkerGlobalScope::wrapCryptoKey(const Vector<uint8_t>& key, Vector<uint8_t>& wrappedKey)
 {
     bool result = false;
-    bool done = false;
-    m_thread.workerLoaderProxy().postTaskToLoader([&result, &key, &wrappedKey, &done, workerGlobalScope = this](ScriptExecutionContext& context) {
-        result = context.wrapCryptoKey(key, wrappedKey);
+    std::atomic<bool> done = false;
+    auto container = CryptoBufferContainer::create();
+    m_thread.workerLoaderProxy().postTaskToLoader([&result, key, containerRef = container.copyRef(), &done, workerGlobalScope = this](ScriptExecutionContext& context) {
+        result = context.wrapCryptoKey(key, containerRef->buffer());
         done = true;
         workerGlobalScope->postTask([](ScriptExecutionContext& context) {
@@ -402 +412 @@
         waitResult = m_thread.runLoop().runInMode(this, WorkerRunLoop::defaultMode());
 
+    if (done)
+        wrappedKey.swap(container->buffer());
     return result;
 }
@@ -407 +419 @@
 bool WorkerGlobalScope::unwrapCryptoKey(const Vector<uint8_t>& wrappedKey, Vector<uint8_t>& key)
 {
-    bool result = false, done = false;
-    m_thread.workerLoaderProxy().postTaskToLoader([&result, &wrappedKey, &key, &done, workerGlobalScope = this](ScriptExecutionContext& context) {
-        result = context.unwrapCryptoKey(wrappedKey, key);
+    bool result = false;
+    std::atomic<bool> done = false;
+    auto container = CryptoBufferContainer::create();
+    m_thread.workerLoaderProxy().postTaskToLoader([&result, wrappedKey, containerRef = container.copyRef(), &done, workerGlobalScope = this](ScriptExecutionContext& context) {
+        result = context.unwrapCryptoKey(wrappedKey, containerRef->buffer());
         done = true;
         workerGlobalScope->postTask([](ScriptExecutionContext& context) {
@@ -420 +434 @@
         waitResult = m_thread.runLoop().runInMode(this, WorkerRunLoop::defaultMode());
 
+    if (done)
+        key.swap(container->buffer());
     return result;
 }