qt: Avoid OpenSSL certstore-related memory leak

laanwj · laanwj · commit ed998ea7a0ec · 2016-11-23T12:33:37.000+01:00
- Correctly manage the X509 and X509_STORE objects lifetime.
diff --git a/src/qt/paymentserver.cpp b/src/qt/paymentserver.cpp
@@ -58,14 +58,19 @@ const char* BIP71_MIMETYPE_PAYMENTREQUEST = "application/bitcoin-paymentrequest"
 // BIP70 max payment request size in bytes (DoS protection)
 const qint64 BIP70_MAX_PAYMENTREQUEST_SIZE = 50000;
 
-X509_STORE* PaymentServer::certStore = NULL;
-void PaymentServer::freeCertStore()
+struct X509StoreDeleter {
+      void operator()(X509_STORE* b) {
+          X509_STORE_free(b);
+      }
+};
+
+struct X509Deleter {
+      void operator()(X509* b) { X509_free(b); }
+};
+
+namespace // Anon namespace
 {
-    if (PaymentServer::certStore != NULL)
-    {
-        X509_STORE_free(PaymentServer::certStore);
-        PaymentServer::certStore = NULL;
-    }
+    std::unique_ptr<X509_STORE, X509StoreDeleter> certStore;
 }
 
 //
@@ -107,20 +112,15 @@ static void ReportInvalidCertificate(const QSslCertificate& cert)
 //
 void PaymentServer::LoadRootCAs(X509_STORE* _store)
 {
-    if (PaymentServer::certStore == NULL)
-        atexit(PaymentServer::freeCertStore);
-    else
-        freeCertStore();
-
     // Unit tests mostly use this, to pass in fake root CAs:
     if (_store)
     {
-        PaymentServer::certStore = _store;
+        certStore.reset(_store);
         return;
     }
 
     // Normal execution, use either -rootcertificates or system certs:
-    PaymentServer::certStore = X509_STORE_new();
+    certStore.reset(X509_STORE_new());
 
     // Note: use "-system-" default here so that users can pass -rootcertificates=""
     // and get 'I don't like X.509 certificates, don't trust anybody' behavior:
@@ -167,11 +167,11 @@ void PaymentServer::LoadRootCAs(X509_STORE* _store)
         QByteArray certData = cert.toDer();
         const unsigned char *data = (const unsigned char *)certData.data();
 
-        X509* x509 = d2i_X509(0, &data, certData.size());
-        if (x509 && X509_STORE_add_cert(PaymentServer::certStore, x509))
+        std::unique_ptr<X509, X509Deleter> x509(d2i_X509(0, &data, certData.size()));
+        if (x509 && X509_STORE_add_cert(certStore.get(), x509.get()))
         {
-            // Note: X509_STORE_free will free the X509* objects when
-            // the PaymentServer is destroyed
+            // Note: X509_STORE increases the reference count to the X509 object,
+            // we still have to release our reference to it.
             ++nRootCerts;
         }
         else
@@ -550,7 +550,7 @@ bool PaymentServer::processPaymentRequest(const PaymentRequestPlus& request, Sen
     recipient.paymentRequest = request;
     recipient.message = GUIUtil::HtmlEscape(request.getDetails().memo());
 
-    request.getMerchant(PaymentServer::certStore, recipient.authenticatedMerchant);
+    request.getMerchant(certStore.get(), recipient.authenticatedMerchant);
 
     QList<std::pair<CScript, CAmount> > sendingTos = request.getPayTo();
     QStringList addresses;
@@ -807,3 +807,8 @@ bool PaymentServer::verifyAmount(const CAmount& requestAmount)
     }
     return fVerified;
 }
+
+X509_STORE* PaymentServer::getCertStore()
+{
+    return certStore.get();
+}
diff --git a/src/qt/paymentserver.h b/src/qt/paymentserver.h
@@ -83,7 +83,7 @@ class PaymentServer : public QObject
     static void LoadRootCAs(X509_STORE* store = NULL);
 
     // Return certificate store
-    static X509_STORE* getCertStore() { return certStore; }
+    static X509_STORE* getCertStore();
 
     // OptionsModel is used for getting proxy settings and display unit
     void setOptionsModel(OptionsModel *optionsModel);
@@ -140,9 +140,6 @@ private Q_SLOTS:
     bool saveURIs;                      // true during startup
     QLocalServer* uriServer;
 
-    static X509_STORE* certStore;       // Trusted root certificates
-    static void freeCertStore();
-
     QNetworkAccessManager* netManager;  // Used to fetch payment requests
 
     OptionsModel *optionsModel;
