Add support for GnuTLS and other non-OpenSSL logics to LockingPointer

This brings LockingPointer and TidyPointer back to the same API definition
for the reset() method. And ensures that cases where LockingPointer could
accidentally have been used with reset() now lock/unlock properly.

To do this a static global is defined to store the currently active Lock
for each allocated base T objects. For each type T the template Locks()
method needs to be instantiated in an appropriate place. If there is no
better place src/security/support.cc is used.

diff --git a/src/base/TidyPointer.h b/src/base/TidyPointer.h
index ce2c3bd96631db5739bf28fda56f9cfd40825114..5eb7d43b65d702974a3cdf92e7c738fb6dbffd35 100644 (file)
--- a/src/base/TidyPointer.h
+++ b/src/base/TidyPointer.h
@@ -30,7 +30,7 @@ public:
     /// Address of the raw pointer, for pointer-setting functions
     T **addr() { return &raw; }
     /// Reset raw pointer - delete last one and save new one.
-    void reset(T *t) {
+    virtual void reset(T *t) {
         deletePointer();
         raw = t;
     }
@@ -42,7 +42,7 @@ public:
         return ret;
     }
     /// Deallocate raw pointer.
-    ~TidyPointer() {
+    virtual ~TidyPointer() {
         deletePointer();
     }
 private:

diff --git a/src/client_side.cc b/src/client_side.cc
index b9634c8fa13bd463df318e22c12d4b08ad97b888..ce5cbc671e54b32c0f1e3420a1d47eecc609b208 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -3573,9 +3573,9 @@ void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &cer
     // fake certificate adaptation requires bump-server-first mode
     if (!sslServerBump) {
         assert(port->signingCert.get());
-        certProperties.signWithX509.resetAndLock(port->signingCert.get());
+        certProperties.signWithX509.reset(port->signingCert.get());
         if (port->signPkey.get())
-            certProperties.signWithPkey.resetAndLock(port->signPkey.get());
+            certProperties.signWithPkey.reset(port->signPkey.get());
         certProperties.signAlgorithm = Ssl::algSignTrusted;
         return;
     }
@@ -3587,7 +3587,7 @@ void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &cer
     assert(sslServerBump->entry);
     if (sslServerBump->entry->isEmpty()) {
         if (X509 *mimicCert = sslServerBump->serverCert.get())
-            certProperties.mimicCert.resetAndLock(mimicCert);
+            certProperties.mimicCert.reset(mimicCert);
 
         ACLFilledChecklist checklist(NULL, sslServerBump->request.getRaw(),
                                      clientConnection != NULL ? clientConnection->rfc931 : dash_str);
@@ -3640,14 +3640,14 @@ void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &cer
 
     if (certProperties.signAlgorithm == Ssl::algSignUntrusted) {
         assert(port->untrustedSigningCert.get());
-        certProperties.signWithX509.resetAndLock(port->untrustedSigningCert.get());
-        certProperties.signWithPkey.resetAndLock(port->untrustedSignPkey.get());
+        certProperties.signWithX509.reset(port->untrustedSigningCert.get());
+        certProperties.signWithPkey.reset(port->untrustedSignPkey.get());
     } else {
         assert(port->signingCert.get());
-        certProperties.signWithX509.resetAndLock(port->signingCert.get());
+        certProperties.signWithX509.reset(port->signingCert.get());
 
         if (port->signPkey.get())
-            certProperties.signWithPkey.resetAndLock(port->signPkey.get());
+            certProperties.signWithPkey.reset(port->signPkey.get());
     }
     signAlgorithm = certProperties.signAlgorithm;
 

diff --git a/src/security/LockingPointer.h b/src/security/LockingPointer.h
index 3d3c6e089041d9aa549dc3b13e425dae2310246c..106d7e90270f1947c8546af77ab075069f8a90e1 100644 (file)
--- a/src/security/LockingPointer.h
+++ b/src/security/LockingPointer.h
@@ -12,6 +12,7 @@
 #include "base/TidyPointer.h"
 
 #if USE_OPENSSL
+
 #if HAVE_OPENSSL_CRYPTO_H
 #include <openssl/crypto.h>
 #endif
@@ -24,6 +25,11 @@
             sk_object ## _pop_free(a, freefunction); \
         }
 
+#else // !USE_OPENSSL
+
+#include "base/Lock.h"
+#include <unordered_map>
+
 #endif
 
 // Macro to be used to define the C++ equivalent function of an extern "C"
@@ -46,44 +52,70 @@ public:
     typedef TidyPointer<T, DeAllocator> Parent;
     typedef LockingPointer<T, DeAllocator, lock> SelfType;
 
-    explicit LockingPointer(T *t = nullptr): Parent(t) {}
+    explicit LockingPointer(T *t = nullptr): Parent() {reset(t);}
+
+    virtual ~LockingPointer() { Parent::reset(nullptr); }
 
     explicit LockingPointer(const SelfType &o): Parent() {
-        resetAndLock(o.get());
+        reset(o.get());
     }
 
     SelfType &operator =(const SelfType & o) {
-        resetAndLock(o.get());
+        reset(o.get());
         return *this;
     }
 
-#if __cplusplus >= 201103L
-    explicit LockingPointer(LockingPointer<T, DeAllocator, lock> &&o): Parent(o.get()) {
-        *o.addr() = nullptr;
+    explicit LockingPointer(LockingPointer<T, DeAllocator, lock> &&o) : Parent() {
+        *(this->addr()) = o.get();
+        o.release();
     }
 
     LockingPointer<T, DeAllocator, lock> &operator =(LockingPointer<T, DeAllocator, lock> &&o) {
         if (o.get() != this->get()) {
-            this->reset(o.get());
-            *o.addr() = nullptr;
+            if (this->get()) {
+                Parent::reset(o.get());
+            } else {
+                *(this->addr()) = o.get();
+                o.release();
+            }
         }
         return *this;
     }
+
+    virtual void reset(T *t) {
+        if (t == this->get())
+            return;
+
+#if !USE_OPENSSL
+        // OpenSSL maintains the reference locks through calls to Deallocator
+        // our manual locking does not have that luxury
+        if (this->get()) {
+            if (SelfType::Locks().at(this->get()).unlock())
+                SelfType::Locks().erase(this->get());
+        }
 #endif
+        Parent::reset(t);
 
-    void resetAndLock(T *t) {
-        if (t != this->get()) {
-            this->reset(t);
+        if (t) {
 #if USE_OPENSSL
-            if (t)
-                CRYPTO_add(&t->references, 1, lock);
-#elif USE_GNUTLS
-            // XXX: GnuTLS does not provide locking ?
+            CRYPTO_add(&t->references, 1, lock);
 #else
-            assert(false);
+            SelfType::Locks()[t].lock(); // find/create and lock
 #endif
         }
     }
+
+private:
+#if !USE_OPENSSL
+    // since we can never be sure if a raw-* passed to us is already being
+    // lock counted by another LockingPointer<> and the types pointed to are
+    // defined by third-party libraries we have to maintain the locks in a
+    // type-specific static external to both the Pointer and base classes.
+    static std::unordered_map<T*, Lock> & Locks() {
+        static std::unordered_map<T*, Lock> Instance;
+        return Instance;
+    }
+#endif
 };
 
 } // namespace Security

diff --git a/src/security/Makefile.am b/src/security/Makefile.am
index 17affaff1c6778e3f1fe0e007b484318e7dc7a4b..afc0007fe067a7b521d40155759828842a063539 100644 (file)
--- a/src/security/Makefile.am
+++ b/src/security/Makefile.am
@@ -23,4 +23,5 @@ libsecurity_la_SOURCES= \
        PeerOptions.h \
        ServerOptions.cc \
        ServerOptions.h \
-       Session.h
+       Session.h \
+       support.cc

diff --git a/src/security/forward.h b/src/security/forward.h
index 54d6b06fb34fccf967cdc2ace88a7fd46dce1110..382f2c96165d4cdcc977c8c370a1d3b6d3884cdb 100644 (file)
--- a/src/security/forward.h
+++ b/src/security/forward.h
@@ -43,7 +43,7 @@ typedef Security::LockingPointer<X509, X509_free_cpp, CRYPTO_LOCK_X509> CertPoin
 CtoCpp1(gnutls_x509_crt_deinit, gnutls_x509_crt_t)
 typedef Security::LockingPointer<struct gnutls_x509_crt_int, gnutls_x509_crt_deinit, -1> CertPointer;
 #else
-typedef void * CertPointer;
+typedef Security::LockingPointer<void, nullptr, -1> CertPointer;
 #endif
 
 #if USE_OPENSSL
@@ -53,7 +53,7 @@ typedef LockingPointer<X509_CRL, X509_CRL_free_cpp, CRYPTO_LOCK_X509_CRL> CrlPoi
 CtoCpp1(gnutls_x509_crl_deinit, gnutls_x509_crl_t)
 typedef Security::LockingPointer<struct gnutls_x509_crl_int, gnutls_x509_crl_deinit, -1> CrlPointer;
 #else
-typedef void *CrlPointer;
+typedef Security::LockingPointer<void, nullptr, -1> CrlPointer;
 #endif
 
 typedef std::list<Security::CrlPointer> CertRevokeList;
@@ -62,7 +62,7 @@ typedef std::list<Security::CrlPointer> CertRevokeList;
 CtoCpp1(DH_free, DH *);
 typedef Security::LockingPointer<DH, DH_free_cpp, CRYPTO_LOCK_DH> DhePointer;
 #else
-typedef void *DhePointer;
+typedef Security::LockingPointer<void, nullptr, -1> DhePointer;
 #endif
 
 class KeyData;

diff --git a/src/security/support.cc b/src/security/support.cc
new file mode 100644 (file)
index 0000000..0e376d1
--- /dev/null
+++ b/src/security/support.cc
@@ -0,0 +1,30 @@
+/*
+ * Copyright (C) 1996-2016 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+#include "squid.h"
+#include "security/forward.h"
+
+#if !USE_OPENSSL
+
+namespace Security
+{
+
+#if USE_GNUTLS
+template std::unordered_map<Security::ContextPtr, Lock> & ContextPointer::Locks();
+
+template std::unordered_map<gnutls_x509_crt_t, Lock> & CertPointer::Locks();
+
+template std::unordered_map<gnutls_x509_crl_t, Lock> & CrlPointer::Locks();
+#endif
+
+template std::unordered_map<void*, Lock> & DhePointer::Locks();
+
+} // namespace Security
+
+#endif /* !USE_OPENSSL */
+

diff --git a/src/ssl/ErrorDetail.cc b/src/ssl/ErrorDetail.cc
index b6ce27ff9222e2eb8f7cd9144d409789eda36d5c..ec3ff474618afb238f1d137dc4a108e45c168f2a 100644 (file)
--- a/src/ssl/ErrorDetail.cc
+++ b/src/ssl/ErrorDetail.cc
@@ -628,12 +628,12 @@ const String &Ssl::ErrorDetail::toString() const
 Ssl::ErrorDetail::ErrorDetail( Ssl::ssl_error_t err_no, X509 *cert, X509 *broken, const char *aReason): error_no (err_no), lib_error_no(SSL_ERROR_NONE), errReason(aReason)
 {
     if (cert)
-        peer_cert.resetAndLock(cert);
+        peer_cert.reset(cert);
 
     if (broken)
-        broken_cert.resetAndLock(broken);
+        broken_cert.reset(broken);
     else
-        broken_cert.resetAndLock(cert);
+        broken_cert.reset(cert);
 
     detailEntry.error_no = SSL_ERROR_NONE;
 }
@@ -644,11 +644,11 @@ Ssl::ErrorDetail::ErrorDetail(Ssl::ErrorDetail const &anErrDetail)
     request = anErrDetail.request;
 
     if (anErrDetail.peer_cert.get()) {
-        peer_cert.resetAndLock(anErrDetail.peer_cert.get());
+        peer_cert.reset(anErrDetail.peer_cert.get());
     }
 
     if (anErrDetail.broken_cert.get()) {
-        broken_cert.resetAndLock(anErrDetail.broken_cert.get());
+        broken_cert.reset(anErrDetail.broken_cert.get());
     }
 
     detailEntry = anErrDetail.detailEntry;

diff --git a/src/ssl/PeerConnector.cc b/src/ssl/PeerConnector.cc
index 2f0199d92857db4dcdcfccd42d4cd7fa93960fee..a5d872d98a06d3f2a2f250842110b114515ecfa5 100644 (file)
--- a/src/ssl/PeerConnector.cc
+++ b/src/ssl/PeerConnector.cc
@@ -760,7 +760,7 @@ Ssl::PeekingPeerConnector::noteNegotiationDone(ErrorState *error)
         if (!serverBump->serverCert.get()) {
             // remember the server certificate from the ErrorDetail object
             if (error && error->detail && error->detail->peerCert())
-                serverBump->serverCert.resetAndLock(error->detail->peerCert());
+                serverBump->serverCert.reset(error->detail->peerCert());
             else {
                 handleServerCertificate();
             }
@@ -885,7 +885,7 @@ Ssl::PeekingPeerConnector::serverCertificateVerified()
     if (ConnStateData *csd = request->clientConnectionManager.valid()) {
         Security::CertPointer serverCert;
         if(Ssl::ServerBump *serverBump = csd->serverBump())
-            serverCert.resetAndLock(serverBump->serverCert.get());
+            serverCert.reset(serverBump->serverCert.get());
         else {
             const int fd = serverConnection()->fd;
             SSL *ssl = fd_table[fd].ssl;

diff --git a/src/ssl/cert_validate_message.cc b/src/ssl/cert_validate_message.cc
index 7c2249ed617cb9a5261af21a8c7ab15ee7d612c8..409283309f67fc5be06697bd8e65680ec8bf447a 100644 (file)
--- a/src/ssl/cert_validate_message.cc
+++ b/src/ssl/cert_validate_message.cc
@@ -216,7 +216,7 @@ Ssl::CertValidationResponse::RecvdError & Ssl::CertValidationResponse::RecvdErro
 void
 Ssl::CertValidationResponse::RecvdError::setCert(X509 *aCert)
 {
-    cert.resetAndLock(aCert);
+    cert.reset(aCert);
 }
 
 Ssl::CertValidationMsg::CertItem::CertItem(const CertItem &old)
@@ -235,7 +235,7 @@ Ssl::CertValidationMsg::CertItem & Ssl::CertValidationMsg::CertItem::operator =
 void
 Ssl::CertValidationMsg::CertItem::setCert(X509 *aCert)
 {
-    cert.resetAndLock(aCert);
+    cert.reset(aCert);
 }
 
 const std::string Ssl::CertValidationMsg::code_cert_validate("cert_validate");

diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc
index fba15426a72dbd5a2aa802ccef746854c257a800..da8370777ba5fd63cf6c1c3fa90bfa3d4e8ab7ab 100644 (file)
--- a/src/ssl/gadgets.cc
+++ b/src/ssl/gadgets.cc
@@ -424,7 +424,7 @@ static bool generateFakeSslCertificate(Security::CertPointer & certToStore, Ssl:
     Ssl::EVP_PKEY_Pointer pkey;
     // Use signing certificates private key as generated certificate private key
     if (properties.signWithPkey.get())
-        pkey.resetAndLock(properties.signWithPkey.get());
+        pkey.reset(properties.signWithPkey.get());
     else // if not exist generate one
         pkey.reset(Ssl::createSslPrivateKey());
 

diff --git a/src/ssl/support.cc b/src/ssl/support.cc
index d4fc4c8196cac54f141bf2d6db01a1635949e77b..9fecc54efc135dd454075a8238c65ecd7409b767 100644 (file)
--- a/src/ssl/support.cc
+++ b/src/ssl/support.cc
@@ -307,7 +307,7 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx)
                 ACLFilledChecklist *filledCheck = Filled(check);
                 assert(!filledCheck->sslErrors);
                 filledCheck->sslErrors = new Ssl::CertErrors(Ssl::CertError(error_no, broken_cert));
-                filledCheck->serverCert.resetAndLock(peer_cert);
+                filledCheck->serverCert.reset(peer_cert);
                 if (check->fastCheck() == ACCESS_ALLOWED) {
                     debugs(83, 3, "bypassing SSL error " << error_no << " in " << buffer);
                     ok = 1;
@@ -1288,8 +1288,8 @@ bool Ssl::generateUntrustedCert(Security::CertPointer &untrustedCert, EVP_PKEY_P
     // O, OU, and other CA subject fields will be mimicked
     // Expiration date and other common properties will be mimicked
     certProperties.signAlgorithm = Ssl::algSignSelf;
-    certProperties.signWithPkey.resetAndLock(pkey.get());
-    certProperties.mimicCert.resetAndLock(cert.get());
+    certProperties.signWithPkey.reset(pkey.get());
+    certProperties.mimicCert.reset(cert.get());
     return Ssl::generateSslCertificate(untrustedCert, untrustedPkey, certProperties);
 }
 
@@ -1342,19 +1342,19 @@ Ssl::CreateServer(Security::ContextPtr sslContext, const int fd, const char *squ
 
 Ssl::CertError::CertError(ssl_error_t anErr, X509 *aCert, int aDepth): code(anErr), depth(aDepth)
 {
-    cert.resetAndLock(aCert);
+    cert.reset(aCert);
 }
 
 Ssl::CertError::CertError(CertError const &err): code(err.code), depth(err.depth)
 {
-    cert.resetAndLock(err.cert.get());
+    cert.reset(err.cert.get());
 }
 
 Ssl::CertError &
 Ssl::CertError::operator = (const CertError &old)
 {
     code = old.code;
-    cert.resetAndLock(old.cert.get());
+    cert.reset(old.cert.get());
     return *this;
 }