Attachment 358463 Details for Bug 193177 – Patch v2

[patch] Patch v2

bug-193177-20190106111603.patch (text/plain), 4.67 KB, created by David Kilzer (:ddkilzer) on 2019-01-06 11:16:04 PST

(hide)

Description:

Filename:

MIME Type:

Creator: David Kilzer (:ddkilzer)

Created: 2019-01-06 11:16:04 PST

Size: 4.67 KB

patch

obsolete

>Subversion Revision: 239367
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 8ba5af4888aa4bf9d14844cc9faf3accc36fa841..5f6473e2ebe449eb1230ceb1827272bee0f0d294 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,25 @@
>+2019-01-06  David Kilzer  <ddkilzer@apple.com>
>+
>+        Leak of WTF::Function objects in WebCore::CryptoKeyRSA::generatePair() (64-80 bytes each) in com.apple.WebKit.WebContent running WebKit layout tests
>+        <https://webkit.org/b/193177>
>+        <rdar://problem/47072196>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * crypto/mac/CryptoKeyRSAMac.cpp:
>+        (WebCore::CryptoKeyRSA::generatePair): Fix the leak by changing
>+        raw pointers to heap-allocated __block variables to hold the
>+        WTF::Function objects until they are consumed within the block
>+        passed to dispatch_async().  The __block variables act like
>+        captured variables in a C++ lambda and have the same lifetime as
>+        the block that they are captured in.  Note that we would have to
>+        convert the source file from C++ to Objective-C++ to use a C++
>+        lambda functor with dispatch_async(), which creates its own
>+        issue because the comipiler requires a copy constructor to
>+        convert the C++ lambda to a block functor, but the copy
>+        constructor for the C++ lambda is implicitly deleted because the
>+        WTF::Function copy constructor is explicitly deleted.  Whew!
>+
> 2019-01-04  David Kilzer  <ddkilzer@apple.com>
> 
>         Leak of two CCRSACryptorRef (4.0 Kbytes/1 page each) in com.apple.WebKit.WebContent running WebKit layout tests
>diff --git a/Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp b/Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp
>index ff35f99caa13a0a70753d37ba83dbcde9977fa4c..84d4e4bea9e0316fee4688874628d701c27fb997 100644
>--- a/Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp
>+++ b/Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp
>@@ -301,34 +301,25 @@ void CryptoKeyRSA::generatePair(CryptoAlgorithmIdentifier algorithm, CryptoAlgor
>         return;
>     }
> 
>-    // We only use the callback functions when back on the main/worker thread, but captured variables are copied on a secondary thread too.
>-    KeyPairCallback* localCallback = new KeyPairCallback(WTFMove(callback));
>-    VoidCallback* localFailureCallback = new VoidCallback(WTFMove(failureCallback));
>+    __block auto blockCallback(WTFMove(callback));
>+    __block auto blockFailureCallback(WTFMove(failureCallback));
>     auto contextIdentifier = context->contextIdentifier();
>-
>-    // FIXME: There is a risk that localCallback and localFailureCallback are never freed.
>-    // Fix this by using unique pointers and move them from one lambda to the other.
>     dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
>         CCRSACryptorRef ccPublicKey = nullptr;
>         CCRSACryptorRef ccPrivateKey = nullptr;
>         CCCryptorStatus status = CCRSACryptorGeneratePair(modulusLength, e, &ccPublicKey, &ccPrivateKey);
>         if (status) {
>             WTFLogAlways("Could not generate a key pair, status %d", status);
>-            ScriptExecutionContext::postTaskTo(contextIdentifier, [localCallback, localFailureCallback](auto&) {
>-                (*localFailureCallback)();
>-                delete localCallback;
>-                delete localFailureCallback;
>+            ScriptExecutionContext::postTaskTo(contextIdentifier, [callback = WTFMove(blockCallback), failureCallback = WTFMove(blockFailureCallback)](auto&) {
>+                failureCallback();
>             });
>             return;
>         }
>-        ScriptExecutionContext::postTaskTo(contextIdentifier, [algorithm, hash, hasHash, extractable, usage, localCallback, localFailureCallback, ccPublicKey = PlatformRSAKeyContainer(ccPublicKey), ccPrivateKey = PlatformRSAKeyContainer(ccPrivateKey)](auto&) mutable {
>+        ScriptExecutionContext::postTaskTo(contextIdentifier, [algorithm, hash, hasHash, extractable, usage, callback = WTFMove(blockCallback), failureCallback = WTFMove(blockFailureCallback), ccPublicKey = PlatformRSAKeyContainer(ccPublicKey), ccPrivateKey = PlatformRSAKeyContainer(ccPrivateKey)](auto&) mutable {
>             auto publicKey = CryptoKeyRSA::create(algorithm, hash, hasHash, CryptoKeyType::Public, WTFMove(ccPublicKey), true, usage);
>             auto privateKey = CryptoKeyRSA::create(algorithm, hash, hasHash, CryptoKeyType::Private, WTFMove(ccPrivateKey), extractable, usage);
> 
>-            (*localCallback)(CryptoKeyPair { WTFMove(publicKey), WTFMove(privateKey) });
>-
>-            delete localCallback;
>-            delete localFailureCallback;
>+            callback(CryptoKeyPair { WTFMove(publicKey), WTFMove(privateKey) });
>         });
>     });
> }

Actions: View | Formatted Diff | Diff

Attachments on bug 193177: 358461 | 358463