Leak of WTF::Function objects in WebCore::CryptoKeyRSA::generatePair() (64-80 bytes...
authorddkilzer@apple.com <ddkilzer@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 7 Jan 2019 02:05:44 +0000 (02:05 +0000)
committerddkilzer@apple.com <ddkilzer@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 7 Jan 2019 02:05:44 +0000 (02:05 +0000)
<https://webkit.org/b/193177>
<rdar://problem/47072196>

Reviewed by Saam Barati.

* 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!

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@239664 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp

index e0534ad..4f0f106 100644 (file)
@@ -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 Saam Barati.
+
+        * 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-06  Pablo Saavedra  <psaavedra@igalia.com>
 
         [WPE][GTK] Building with ENABLE_VIDEO=OFF fails trying to use Document MediaPlayback functions.
index 214e5c0..0122188 100644 (file)
@@ -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) });
         });
     });
 }