[squid-dev] [RFC] [PREVIEW] LockingPointer round 3.

Amos Jeffries squid3 at treenet.co.nz
Thu Jul 21 13:58:07 UTC 2016


On 21/07/2016 5:54 a.m., Alex Rousskov wrote:
> On 07/19/2016 10:45 PM, Amos Jeffries wrote:
>> On 19/07/2016 7:14 p.m., Amos Jeffries wrote:
>>> On 19/07/2016 6:58 a.m., Christos Tsantilas wrote:
>>>> On 07/18/2016 08:32 PM, Alex Rousskov wrote:
>>>>> I can only repeat my earlier suggestions to hide that dangerous
>>>>> constructor behind an explicit static method like
>>>>> LockingPointer::NewWithoutLocking() and to assert that
>>>>> resetWithoutLocking() method is fed a previously locked object.
> 
>>>> something like that.
> 
>>> Nod. I'm about to try a build with dropped construtor to see what
>>> breaks. That should highlight if a builder function is actually
>>> necessary. I hope we can avoid it.
> 
> 
>> Attached is a patch showing what that constructor removal would look
>> like for current Squid trunk.
> 
> Creating a nil pointer just to reset it on the next line looks like bad
> style to me. It makes it impossible to make the pointer object constant
> and increases the chances that the two lines will get separated in the
> future for now good reason, possibly causing bugs.
> 
> Why resist naming an unusual/special construction method?
> 

Just considering all the options without jumping to one too quickly.

How about?

  static SelfType&& MakeFromLockedOpenSslPointer(T *t) {
      SelfType out;
      out.resetWithoutLocking(t);
      return std::move(out);
  }


Theres something I'm not quite understanding going on with the
return-by-value for template types giving me compiler errors. Thus the
std::move, it seems to build but I'm not sure if it will run properly at
all.

> 
>> I've added "// X locks" comments on new resetWithoutLocking() to be
>> clearer where the non-locking comes from. OpenSSL API explicitly
>> mentions that it "increments a reference" / locks.
> 
> The "resetWithoutLocking(X()); // X locks" approach creates a redundant
> distraction IMO. If you feel that we need those comments, then perhaps
> the reset method needs a new name, like resetUsingLocked(X()) or
> resetToLocked(X()).
> 
> Note that you actually did not add a comment where one would be useful:
> 
> ctx->resetWithoutLocking(sslContext); // XXX: how do we know its locked?
> 

Yeah, identifying this ambiguous use (yay, no others) was the point of
the comment experiment.

> 
>> // XXX: why not set fd_table if that add() succeeded ?
> 
> The simple answer is that if you set fd_table after cache add()ing
> succeeded, then both fd_table and the cache will delete the same
> pointer, crashing Squid. Note that ctx is a raw pointer [to a smart
> pointer]. However, I doubt this is the correct/best answer.
> 
> AFAICT, the poor-quality comments in the original code were supposed to
> answer that question like this: The _only_ reason we add ctx to the
> table is so that it gets eventually destroyed. The context object is not
> needed in the table and should not be accessed from there. If we
> successfully add ctx to the cache, then the cache becomes responsible
> for its destruction and we do not need to add it to the table.
> 

So that means the fd_table member can be set *always*, and should be a
Pointer already. An extra +1 and -1 cycle happening on some objects is a
trivial cost to pay for reliable existence of contexts.
 That will also protect any logic that may depend on one or the other
storage place existing from non-obvious nullptr values.


On the other hand. I suspect that the use fd_table is being put to is
better served by one of the MasterXaction / AccessLogEntry /
ConnStateData objects relevant to the transaction using that context.


> 
>> +            if (!ssl_ctx_cache)
>> +                // if there is no storage, just use
>>                  fd_table[clientConnection->fd].dynamicSslContext = sslContext;
>> +            else {
> 
> Missing {} around the multiline if-statement body.

Re-written this section in light of the above explanation.


> 
>> +    explicit LockingPointer() : raw(nullptr) {}
> 
> Default explicit constructors is a convoluted C++ issue. Unless you have
> a specific need for it, I suggest removing "explicit" here. Otherwise,
> document that need in a source code comment.
> 

ok.

> 
>> +     * Our destructor will do the matching unlock.
> 
> This comment is inaccurate because we may unlock in places other than
> the destructor. You can say "We become responsible for the matching
> unlock()." or something like that.
> 

ok.

> 
>> +        tmp.resetWithoutLocking(crl); // XXX: does PEM_read_bio_X509_CRL lock ??
> 
> One more reason to assert that resetWithoutLocking() is fed a locked
> OpenSSL object and also assert that unlock() has a locked OpenSSL object
> (if any).

Asserting that its a locked object, sure and done***. Though that itself
is not a guarantee the caller did the proper locking. Some other things
(including self) might be holding locks that disappear later on us. I've
used >1 for that case but still no certainty.

If you are referring to the self-reset assert(t != raw) and so adamant
that is right, please feel free to apply a patch doing so. I'm sure
enough about the outcome of that not to want my name against it.


*** though I should point out that OpenSSL 1.1.0 is making a lot of
things opaque and that includes the reference counting. We may not be
able to access t->references for long.


> 
> To answer your question, yes, I think it does lock because the BUGS
> section of the PEM_read_bio_X509_CRL manual page contains an example
> that frees the result:
> 
>>         X509_free(x);
>>         x = PEM_read_bio_X509(bp, NULL, 0, NULL);
> 

Updated PREVIEW patch attached. Its still building here, but seems to
build okay for the most part.


Amos

-------------- next part --------------
=== modified file 'src/client_side.cc'
--- src/client_side.cc	2016-07-18 12:36:38 +0000
+++ src/client_side.cc	2016-07-21 09:23:17 +0000
@@ -3043,43 +3043,48 @@
     }
     getSslContextDone(NULL);
 }
 
 void
 ConnStateData::getSslContextDone(Security::ContextPtr sslContext, bool isNew)
 {
     // Try to add generated ssl context to storage.
     if (port->generateHostCertificates && isNew) {
 
         if (sslContext && (signAlgorithm == Ssl::algSignTrusted)) {
             Ssl::chainCertificatesToSSLContext(sslContext, *port);
         } else if (signAlgorithm == Ssl::algSignTrusted) {
             debugs(33, DBG_IMPORTANT, "WARNING: can not add signing certificate to SSL context chain because SSL context chain is invalid!");
         }
         //else it is self-signed or untrusted do not attrach any certificate
 
         Ssl::LocalContextStorage *ssl_ctx_cache = Ssl::TheGlobalContextStorage.getLocalStorage(port->s);
         assert(sslBumpCertKey.size() > 0 && sslBumpCertKey[0] != '\0');
         if (sslContext) {
-            if (!ssl_ctx_cache || !ssl_ctx_cache->add(sslBumpCertKey.termedBuf(), new Security::ContextPointer(sslContext))) {
-                // If it is not in storage delete after using. Else storage deleted it.
-                fd_table[clientConnection->fd].dynamicSslContext = sslContext;
+            // ensure the context stays around for at least the duration of this connection.
+            // fd_table is now responsible for reference counting of the pointer.
+            fd_table[clientConnection->fd].dynamicSslContext.resetWithoutLocking(sslContext);
+            if (ssl_ctx_cache) {
+                // hold another reference in cache (if any) to re-use the context for other connections
+                auto * tmp = new Security::ContextPointer(fd_table[clientConnection->fd].dynamicSslContext);
+                if (!ssl_ctx_cache->add(sslBumpCertKey.termedBuf(), tmp))
+                    delete tmp; // dont leak tmp on failures
             }
         } else {
             debugs(33, 2, HERE << "Failed to generate SSL cert for " << sslConnectHostOrIp);
         }
     }
 
     // If generated ssl context = NULL, try to use static ssl context.
     if (!sslContext) {
         if (!port->secure.staticContext) {
             debugs(83, DBG_IMPORTANT, "Closing " << clientConnection->remote << " as lacking TLS context");
             clientConnection->close();
             return;
         } else {
             debugs(33, 5, "Using static TLS context.");
             sslContext = port->secure.staticContext.get();
         }
     }
 
     if (!httpsCreate(clientConnection, sslContext))
         return;
@@ -3286,41 +3291,41 @@
     Http::StreamPointer context = pipeline.front();
     ClientHttpRequest *http = context ? context->http : NULL;
 
     if (sslServerBump->step == Ssl::bumpStep1) {
         sslServerBump->step = Ssl::bumpStep2;
         // Run a accessList check to check if want to splice or continue bumping
 
         ACLFilledChecklist *acl_checklist = new ACLFilledChecklist(Config.accessList.ssl_bump, sslServerBump->request.getRaw(), NULL);
         acl_checklist->al = http ? http->al : NULL;
         //acl_checklist->src_addr = params.conn->remote;
         //acl_checklist->my_addr = s->s;
         acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpNone));
         acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpClientFirst));
         acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpServerFirst));
         acl_checklist->nonBlockingCheck(httpsSslBumpStep2AccessCheckDone, this);
         return;
     }
 
     // will call httpsPeeked() with certificate and connection, eventually
     auto unConfiguredCTX = Ssl::createSSLContext(port->signingCert, port->signPkey, *port);
-    fd_table[clientConnection->fd].dynamicSslContext = unConfiguredCTX;
+    fd_table[clientConnection->fd].dynamicSslContext.resetWithoutLocking(unConfiguredCTX);
 
     if (!httpsCreate(clientConnection, unConfiguredCTX))
         return;
 
     switchedToHttps_ = true;
 
     auto ssl = fd_table[clientConnection->fd].ssl.get();
     BIO *b = SSL_get_rbio(ssl);
     Ssl::ClientBio *bio = static_cast<Ssl::ClientBio *>(b->ptr);
     bio->setReadBufData(inBuf);
     bio->hold(true);
 
     // Here squid should have all of the client hello message so the
     // Squid_SSL_accept should return 0;
     // This block exist only to force openSSL parse client hello and detect
     // ERR_SECURE_ACCEPT_FAIL error, which should be checked and splice if required.
     int ret = 0;
     if ((ret = Squid_SSL_accept(this, NULL)) < 0) {
         debugs(83, 2, "SSL_accept failed.");
         const err_type err = ERR_SECURE_ACCEPT_FAIL;

=== modified file 'src/fde.h'
--- src/fde.h	2016-07-05 23:37:12 +0000
+++ src/fde.h	2016-07-21 09:21:02 +0000
@@ -103,41 +103,41 @@
 
 #if USE_DELAY_POOLS
     ClientInfo * clientInfo;/* pointer to client info used in client write limiter or NULL if not present */
 #endif
     unsigned epoll_state;
 
     _fde_disk disk;
     PF *read_handler;
     void *read_data;
     PF *write_handler;
     void *write_data;
     AsyncCall::Pointer timeoutHandler;
     time_t timeout;
     time_t writeStart;
     void *lifetime_data;
     AsyncCall::Pointer closeHandler;
     AsyncCall::Pointer halfClosedReader; /// read handler for half-closed fds
     READ_HANDLER *read_method;
     WRITE_HANDLER *write_method;
     Security::SessionPointer ssl;
-    Security::ContextPtr dynamicSslContext; ///< cached and then freed when fd is closed
+    Security::ContextPointer dynamicSslContext; ///< cached and then freed when fd is closed
 #if _SQUID_WINDOWS_
     struct {
         long handle;
     } win32;
 #endif
     tos_t tosFromServer;                /**< Stores the TOS flags of the packets from the remote server.
                                             See FwdState::dispatch(). Note that this differs to
                                             tosToServer in that this is the value we *receive* from the,
                                             connection, whereas tosToServer is the value to set on packets
                                             *leaving* Squid.  */
     unsigned int nfmarkFromServer;      /**< Stores the Netfilter mark value of the connection from the remote
                                             server. See FwdState::dispatch(). Note that this differs to
                                             nfmarkToServer in that this is the value we *receive* from the,
                                             connection, whereas nfmarkToServer is the value to set on packets
                                             *leaving* Squid.   */
 
     /** Clear the fde class back to NULL equivalent. */
     inline void clear() {
         type = 0;
         remote_port = 0;
@@ -150,39 +150,39 @@
         memset(&flags,0,sizeof(_fde_flags));
         bytes_read = 0;
         bytes_written = 0;
         pconn.uses = 0;
 #if USE_DELAY_POOLS
         clientInfo = NULL;
 #endif
         epoll_state = 0;
         read_handler = NULL;
         read_data = NULL;
         write_handler = NULL;
         write_data = NULL;
         timeoutHandler = NULL;
         timeout = 0;
         writeStart = 0;
         lifetime_data = NULL;
         closeHandler = NULL;
         halfClosedReader = NULL;
         read_method = NULL;
         write_method = NULL;
-        ssl.resetWithoutLocking(nullptr);
-        dynamicSslContext = NULL;
+        ssl.reset();
+        dynamicSslContext.reset();
 #if _SQUID_WINDOWS_
         win32.handle = (long)NULL;
 #endif
         tosFromServer = '\0';
         nfmarkFromServer = 0;
     }
 };
 
 #define fd_table fde::Table
 
 int fdNFree(void);
 
 #define FD_READ_METHOD(fd, buf, len) (*fd_table[fd].read_method)(fd, buf, len)
 #define FD_WRITE_METHOD(fd, buf, len) (*fd_table[fd].write_method)(fd, buf, len)
 
 #endif /* SQUID_FDE_H */
 

=== modified file 'src/security/LockingPointer.h'
--- src/security/LockingPointer.h	2016-07-13 12:39:22 +0000
+++ src/security/LockingPointer.h	2016-07-21 13:02:50 +0000
@@ -34,117 +34,135 @@
 namespace Security
 {
 
 /**
  * A shared pointer to a reference-counting Object with library-specific
  * absorption, locking, and unlocking implementations. The API largely
  * follows std::shared_ptr.
  *
  * The constructor and the resetWithoutLocking() method import a raw Object pointer.
  * Normally, reset() would lock(), but libraries like OpenSSL
  * pre-lock objects before they are fed to LockingPointer, necessitating
  * this resetWithoutLocking() customization hook.
  */
 template <typename T, void (*UnLocker)(T *t), int lockId>
 class LockingPointer
 {
 public:
     /// a helper label to simplify this objects API definitions below
     typedef Security::LockingPointer<T, UnLocker, lockId> SelfType;
 
-    /**
-     * Construct directly from a raw pointer.
-     * This action requires that the producer of that pointer has already
-     * created one reference lock for the object pointed to.
-     * Our destructor will do the matching unlock.
-     */
-    explicit LockingPointer(T *t = nullptr): raw(nullptr) {
-        // de-optimized for clarity about non-locking
-        resetWithoutLocking(t);
-    }
+    LockingPointer() : raw(nullptr) {}
 
     /// use the custom UnLocker to unlock any value still stored.
     ~LockingPointer() { unlock(); }
 
+    /**
+     * Construct directly from a raw pointer is forbidden.
+     * Use the appropriate reset method instead.
+     */
+    explicit LockingPointer(T *) = delete;
+
     // copy semantics are okay only when adding a lock reference
     explicit LockingPointer(const SelfType &o) : raw(nullptr) {
         resetAndLock(o.get());
     }
     const SelfType &operator =(const SelfType &o) {
         resetAndLock(o.get());
         return *this;
     }
 
     // move semantics are definitely okay, when possible
     explicit LockingPointer(SelfType &&) = default;
     SelfType &operator =(SelfType &&o) {
         if (o.get() != raw)
             resetWithoutLocking(o.release());
         return *this;
     }
 
     bool operator !() const { return !raw; }
     explicit operator bool() const { return raw; }
 
     /// Returns raw and possibly nullptr pointer
     T *get() const { return raw; }
 
-    /// Reset raw pointer - unlock any previous one and save new one without locking.
+    /**
+     * Reset raw pointer - unlock any previous one and save new one without locking.
+     * This requires that the producer of that pointer has already
+     * created one reference lock for the object pointed to.
+     * We will be responsible for the matching unlock.
+     */
     void resetWithoutLocking(T *t) {
+#if USE_OPENSSL
+        assert(!t || t->references > 0);
+        assert(!raw || raw->references > 0);
+        if (raw && t == raw) {
+            assert(raw->references > 1); // us plus caller locks
+        }
+#endif
         unlock();
         raw = t;
     }
 
     void resetAndLock(T *t) {
         if (t != get()) {
             resetWithoutLocking(t);
             lock(t);
         }
     }
 
     /// Forget the raw pointer - unlock if any value was set. Become a nil pointer.
     void reset() { unlock(); }
 
     /// Forget the raw pointer without unlocking it. Become a nil pointer.
     T *release() {
         T *ret = raw;
         raw = nullptr;
         return ret;
     }
 
+    static SelfType&& MakeFromLockedOpenSslPointer(T *t) {
+        SelfType out;
+        out.resetWithoutLocking(t);
+        return std::move(out);
+    }
+
 private:
     /// The lock() method increments Object's reference counter.
     void lock(T *t) {
 #if USE_OPENSSL
         if (t)
             CRYPTO_add(&t->references, 1, lockId);
 #elif USE_GNUTLS
         // XXX: GnuTLS does not provide locking ?
 #else
         assert(false);
 #endif
     }
 
     /// Become a nil pointer. Decrements any pointed-to Object's reference counter
     /// using UnLocker which ideally destroys the object when the counter reaches zero.
     void unlock() {
         if (raw) {
+#if USE_OPENSSL
+            assert(raw->references > 0);
+#endif
             UnLocker(raw);
             raw = nullptr;
         }
     }
 
     /**
      * Normally, no other code will have this raw pointer.
      *
      * However, OpenSSL does some strange and not always consistent things.
      * OpenSSL library may keep its own internal raw pointers and manage
      * their reference counts independently, or it may not. This varies between
      * API functions, though it is usually documented.
      *
      * This means the caller code needs to be carefuly written to use the correct
      * reset method and avoid the raw-pointer constructor unless OpenSSL function
      * producing the pointer is clearly documented as incrementing a lock for it.
      */
     T *raw;
 };
 

=== modified file 'src/security/PeerOptions.cc'
--- src/security/PeerOptions.cc	2016-06-15 14:31:34 +0000
+++ src/security/PeerOptions.cc	2016-07-21 12:42:37 +0000
@@ -519,41 +519,41 @@
     return fl;
 }
 
 /// Load a CRLs list stored in the file whose /path/name is in crlFile
 /// replaces any CRL loaded previously
 void
 Security::PeerOptions::loadCrlFile()
 {
     parsedCrl.clear();
     if (crlFile.isEmpty())
         return;
 
 #if USE_OPENSSL
     BIO *in = BIO_new_file(crlFile.c_str(), "r");
     if (!in) {
         debugs(83, 2, "WARNING: Failed to open CRL file " << crlFile);
         return;
     }
 
     while (X509_CRL *crl = PEM_read_bio_X509_CRL(in,NULL,NULL,NULL)) {
-        parsedCrl.emplace_back(Security::CrlPointer(crl));
+        parsedCrl.emplace_back(Security::CrlPointer::MakeFromLockedOpenSslPointer(crl));
     }
     BIO_free(in);
 #endif
 }
 
 #if USE_OPENSSL && defined(TLSEXT_TYPE_next_proto_neg)
 // Dummy next_proto_neg callback
 static int
 ssl_next_proto_cb(SSL *s, unsigned char **out, unsigned char *outlen, const unsigned char *in, unsigned int inlen, void *arg)
 {
     static const unsigned char supported_protos[] = {8, 'h','t','t', 'p', '/', '1', '.', '1'};
     (void)SSL_select_next_proto(out, outlen, in, inlen, supported_protos, sizeof(supported_protos));
     return SSL_TLSEXT_ERR_OK;
 }
 #endif
 
 void
 Security::PeerOptions::updateContextNpn(Security::ContextPtr &ctx)
 {
     if (!flags.tlsNpn)

=== modified file 'src/security/ServerOptions.cc'
--- src/security/ServerOptions.cc	2016-07-07 19:03:02 +0000
+++ src/security/ServerOptions.cc	2016-07-21 13:43:19 +0000
@@ -105,41 +105,41 @@
 
 #elif USE_GNUTLS
     // Initialize for X.509 certificate exchange
     if (const int x = gnutls_certificate_allocate_credentials(&t)) {
         debugs(83, DBG_CRITICAL, "ERROR: Failed to allocate TLS server context: error=" << x);
     }
 
 #else
     debugs(83, DBG_CRITICAL, "ERROR: Failed to allocate TLS server context: No TLS library");
 
 #endif
 
     return t;
 }
 
 bool
 Security::ServerOptions::createStaticServerContext(AnyP::PortCfg &port)
 {
     updateTlsVersionLimits();
 
-    Security::ContextPointer t(createBlankContext());
+    Security::ContextPointer t(Security::ContextPointer::MakeFromLockedOpenSslPointer(createBlankContext()));
     if (t) {
 #if USE_OPENSSL
         if (!Ssl::InitServerContext(t, port))
             return false;
 #endif
     }
 
     staticContext = std::move(t);
     return bool(staticContext);
 }
 
 void
 Security::ServerOptions::loadDhParams()
 {
     if (dhParamsFile.isEmpty())
         return;
 
 #if USE_OPENSSL
     DH *dhp = nullptr;
     if (FILE *in = fopen(dhParamsFile.c_str(), "r")) {

=== modified file 'src/ssl/PeekingPeerConnector.cc'
--- src/ssl/PeekingPeerConnector.cc	2016-07-13 12:39:22 +0000
+++ src/ssl/PeekingPeerConnector.cc	2016-07-21 11:22:05 +0000
@@ -263,113 +263,113 @@
 Ssl::PeekingPeerConnector::noteWantWrite()
 {
     const int fd = serverConnection()->fd;
     Security::SessionPtr ssl = fd_table[fd].ssl.get();
     BIO *b = SSL_get_rbio(ssl);
     Ssl::ServerBio *srvBio = static_cast<Ssl::ServerBio *>(b->ptr);
 
     if ((srvBio->bumpMode() == Ssl::bumpPeek || srvBio->bumpMode() == Ssl::bumpStare) && srvBio->holdWrite()) {
         debugs(81, 3, "hold write on SSL connection on FD " << fd);
         checkForPeekAndSplice();
         return;
     }
 
     Ssl::PeerConnector::noteWantWrite();
 }
 
 void
 Ssl::PeekingPeerConnector::noteSslNegotiationError(const int result, const int ssl_error, const int ssl_lib_error)
 {
     const int fd = serverConnection()->fd;
-    Security::SessionPtr ssl = fd_table[fd].ssl.get();
+    SSL *ssl = fd_table[fd].ssl.get();
     BIO *b = SSL_get_rbio(ssl);
     Ssl::ServerBio *srvBio = static_cast<Ssl::ServerBio *>(b->ptr);
 
     // In Peek mode, the ClientHello message sent to the server. If the
     // server resuming a previous (spliced) SSL session with the client,
     // then probably we are here because local SSL object does not know
     // anything about the session being resumed.
     //
     if (srvBio->bumpMode() == Ssl::bumpPeek && (resumingSession = srvBio->resumingSession())) {
         // we currently splice all resumed sessions unconditionally
         if (const bool spliceResumed = true) {
             bypassCertValidator();
             checkForPeekAndSpliceMatched(Ssl::bumpSplice);
             return;
         } // else fall through to find a matching ssl_bump action (with limited info)
     }
 
     // If we are in peek-and-splice mode and still we did not write to
     // server yet, try to see if we should splice.
     // In this case the connection can be saved.
     // If the checklist decision is do not splice a new error will
     // occur in the next SSL_connect call, and we will fail again.
     // Abort on certificate validation errors to avoid splicing and
     // thus hiding them.
     // Abort if no certificate found probably because of malformed or
     // unsupported server Hello message (TODO: make configurable).
     if (!SSL_get_ex_data(ssl, ssl_ex_index_ssl_error_detail) &&
             (srvBio->bumpMode() == Ssl::bumpPeek  || srvBio->bumpMode() == Ssl::bumpStare) && srvBio->holdWrite()) {
-        Security::CertPointer serverCert(SSL_get_peer_certificate(ssl));
-        if (serverCert.get()) {
+        Security::CertPointer serverCert(Security::CertPointer::MakeFromLockedOpenSslPointer(SSL_get_peer_certificate(ssl)));
+        if (serverCert) {
             debugs(81, 3, "Error ("  << ERR_error_string(ssl_lib_error, NULL) <<  ") but, hold write on SSL connection on FD " << fd);
             checkForPeekAndSplice();
             return;
         }
     }
 
     // else call parent noteNegotiationError to produce an error page
     Ssl::PeerConnector::noteSslNegotiationError(result, ssl_error, ssl_lib_error);
 }
 
 void
 Ssl::PeekingPeerConnector::handleServerCertificate()
 {
     if (serverCertificateHandled)
         return;
 
     if (ConnStateData *csd = request->clientConnectionManager.valid()) {
         const int fd = serverConnection()->fd;
-        Security::SessionPtr ssl = fd_table[fd].ssl.get();
-        Security::CertPointer serverCert(SSL_get_peer_certificate(ssl));
-        if (!serverCert.get())
+        SSL *ssl = fd_table[fd].ssl.get();
+        Security::CertPointer serverCert(Security::CertPointer::MakeFromLockedOpenSslPointer(SSL_get_peer_certificate(ssl)));
+        if (!serverCert)
             return;
 
         serverCertificateHandled = true;
 
         // remember the server certificate for later use
         if (Ssl::ServerBump *serverBump = csd->serverBump()) {
             serverBump->serverCert = std::move(serverCert);
         }
     }
 }
 
 void
 Ssl::PeekingPeerConnector::serverCertificateVerified()
 {
     if (ConnStateData *csd = request->clientConnectionManager.valid()) {
         Security::CertPointer serverCert;
         if(Ssl::ServerBump *serverBump = csd->serverBump())
             serverCert.resetAndLock(serverBump->serverCert.get());
         else {
             const int fd = serverConnection()->fd;
             Security::SessionPtr ssl = fd_table[fd].ssl.get();
-            serverCert.resetWithoutLocking(SSL_get_peer_certificate(ssl));
+            serverCert.resetWithoutLocking(SSL_get_peer_certificate(ssl)); // SSL_get_peer_certificate locks
         }
         if (serverCert.get()) {
             csd->resetSslCommonName(Ssl::CommonHostName(serverCert.get()));
             debugs(83, 5, "HTTPS server CN: " << csd->sslCommonName() <<
                    " bumped: " << *serverConnection());
         }
     }
 }
 
 void
 Ssl::PeekingPeerConnector::tunnelInsteadOfNegotiating()
 {
     Must(callback != NULL);
     CbDialer *dialer = dynamic_cast<CbDialer*>(callback->getDialer());
     Must(dialer);
     dialer->answer().tunneled = true;
     debugs(83, 5, "The SSL negotiation with server aborted");
 }
 

=== modified file 'src/ssl/PeerConnector.cc'
--- src/ssl/PeerConnector.cc	2016-07-13 12:39:22 +0000
+++ src/ssl/PeerConnector.cc	2016-07-21 12:39:03 +0000
@@ -279,41 +279,41 @@
     for (SVCRECI i = resp.errors.begin(); i != resp.errors.end(); ++i) {
         debugs(83, 7, "Error item: " << i->error_no << " " << i->error_reason);
 
         assert(i->error_no != SSL_ERROR_NONE);
 
         if (!errDetails) {
             bool allowed = false;
             if (check) {
                 check->sslErrors = new Ssl::CertErrors(Ssl::CertError(i->error_no, i->cert.get(), i->error_depth));
                 if (check->fastCheck() == ACCESS_ALLOWED)
                     allowed = true;
             }
             // else the Config.ssl_client.cert_error access list is not defined
             // and the first error will cause the error page
 
             if (allowed) {
                 debugs(83, 3, "bypassing SSL error " << i->error_no << " in " << "buffer");
             } else {
                 debugs(83, 5, "confirming SSL error " << i->error_no);
                 X509 *brokenCert = i->cert.get();
-                Security::CertPointer peerCert(SSL_get_peer_certificate(ssl));
+                Security::CertPointer peerCert(Security::CertPointer::MakeFromLockedOpenSslPointer(SSL_get_peer_certificate(ssl)));
                 const char *aReason = i->error_reason.empty() ? NULL : i->error_reason.c_str();
                 errDetails = new Ssl::ErrorDetail(i->error_no, peerCert.get(), brokenCert, aReason);
             }
             if (check) {
                 delete check->sslErrors;
                 check->sslErrors = NULL;
             }
         }
 
         if (!errs)
             errs = new Ssl::CertErrors(Ssl::CertError(i->error_no, i->cert.get(), i->error_depth));
         else
             errs->push_back_unique(Ssl::CertError(i->error_no, i->cert.get(), i->error_depth));
     }
     if (check)
         delete check;
 
     return errs;
 }
 

=== modified file 'src/ssl/cert_validate_message.h'
--- src/ssl/cert_validate_message.h	2016-01-01 00:12:18 +0000
+++ src/ssl/cert_validate_message.h	2016-07-21 09:06:30 +0000
@@ -31,79 +31,79 @@
     std::string domainName; ///< The server name
     CertValidationRequest() : ssl(NULL), errors(NULL) {}
 };
 
 /**
  * This class is used to store informations found in certificate validation
  * response messages read from certificate validator helper
  */
 class CertValidationResponse: public RefCountable
 {
 public:
     typedef RefCount<CertValidationResponse> Pointer;
 
     /**
      * This class used to hold error informations returned from
      * cert validator helper.
      */
     class  RecvdError
     {
     public:
-        RecvdError(): id(0), error_no(SSL_ERROR_NONE), cert(NULL), error_depth(-1) {}
+        RecvdError(): id(0), error_no(SSL_ERROR_NONE), error_depth(-1) {}
         RecvdError(const RecvdError &);
         RecvdError & operator =(const RecvdError &);
         void setCert(X509 *);  ///< Sets cert to the given certificate
         int id; ///<  The id of the error
         ssl_error_t error_no; ///< The OpenSSL error code
         std::string error_reason; ///< A string describing the error
         Security::CertPointer cert; ///< The broken certificate
         int error_depth; ///< The error depth
     };
 
     typedef std::vector<RecvdError> RecvdErrors;
 
     /// Search in errors list for the error item with id=errorId.
     /// If none found a new RecvdError item added with the given id;
     RecvdError &getError(int errorId);
     RecvdErrors errors; ///< The list of parsed errors
     Helper::ResultCode resultCode; ///< The helper result code
 };
 
 /**
  * This class is responsible for composing or parsing messages destined to
  * or comming from a cert validator helper.
  * The messages format is:
  *   response/request-code SP body-length SP [key=value ...] \x01
  */
 class CertValidationMsg : public CrtdMessage
 {
 private:
     /**
      * This class used to hold the certId/cert pairs found
      * in cert validation messages.
      */
     class CertItem
     {
     public:
         std::string name; ///< The certificate Id to use
         Security::CertPointer cert;       ///< A pointer to certificate
-        CertItem(): cert(NULL) {}
+        CertItem() = default;
         CertItem(const CertItem &);
         CertItem & operator =(const CertItem &);
         void setCert(X509 *); ///< Sets cert to the given certificate
     };
 
 public:
     CertValidationMsg(MessageKind kind): CrtdMessage(kind) {}
 
     /// Build a request message for the cert validation helper
     /// using informations provided by vcert object
     void composeRequest(CertValidationRequest const &vcert);
 
     /// Parse a response message and fill the resp object with parsed informations
     bool parseResponse(CertValidationResponse &resp, STACK_OF(X509) *peerCerts, std::string &error);
 
     /// Search a CertItems list for the certificate with ID "name"
     X509 *getCertByName(std::vector<CertItem> const &, std::string const & name);
 
     /// String code for "cert_validate" messages
     static const std::string code_cert_validate;

=== modified file 'src/ssl/gadgets.cc'
--- src/ssl/gadgets.cc	2016-07-08 06:24:33 +0000
+++ src/ssl/gadgets.cc	2016-07-21 12:50:53 +0000
@@ -1,51 +1,51 @@
 /*
  * 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 "ssl/gadgets.h"
 
 #if HAVE_OPENSSL_X509V3_H
 #include <openssl/x509v3.h>
 #endif
 
 EVP_PKEY * Ssl::createSslPrivateKey()
 {
-    Ssl::EVP_PKEY_Pointer pkey(EVP_PKEY_new());
+    Ssl::EVP_PKEY_Pointer pkey(Ssl::EVP_PKEY_Pointer::MakeFromLockedOpenSslPointer(EVP_PKEY_new()));
 
     if (!pkey)
         return NULL;
 
     Ssl::RSA_Pointer rsa(RSA_generate_key(1024, RSA_F4, NULL, NULL));
 
     if (!rsa)
         return NULL;
 
     if (!EVP_PKEY_assign_RSA(pkey.get(), (rsa.get())))
         return NULL;
 
-    rsa.release();
+    rsa.release(); // XXX: because EVP_PKEY_assign_RSA steals ptr without locking? we leak?
     return pkey.release();
 }
 
 /**
  \ingroup ServerProtocolSSLInternal
  * Set serial random serial number or set random serial number.
  */
 static bool setSerialNumber(ASN1_INTEGER *ai, BIGNUM const* serial)
 {
     if (!ai)
         return false;
     Ssl::BIGNUM_Pointer bn(BN_new());
     if (serial) {
         bn.reset(BN_dup(serial));
     } else {
         if (!bn)
             return false;
 
         if (!BN_pseudo_rand(bn.get(), 64, 0, 0))
             return false;
@@ -499,41 +499,41 @@
         // According to RFC 5280, using extensions requires v3 certificate.
         if (addedExtensions)
             X509_set_version(cert.get(), 2); // value 2 means v3
     }
 
     return true;
 }
 
 static bool generateFakeSslCertificate(Security::CertPointer & certToStore, Ssl::EVP_PKEY_Pointer & pkeyToStore, Ssl::CertificateProperties const &properties,  Ssl::BIGNUM_Pointer const &serial)
 {
     Ssl::EVP_PKEY_Pointer pkey;
     // Use signing certificates private key as generated certificate private key
     if (properties.signWithPkey.get())
         pkey.resetAndLock(properties.signWithPkey.get());
     else // if not exist generate one
         pkey.resetWithoutLocking(Ssl::createSslPrivateKey());
 
     if (!pkey)
         return false;
 
-    Security::CertPointer cert(X509_new());
+    Security::CertPointer cert(Security::CertPointer::MakeFromLockedOpenSslPointer(X509_new()));
     if (!cert)
         return false;
 
     // Set pub key and serial given by the caller
     if (!X509_set_pubkey(cert.get(), pkey.get()))
         return false;
     if (!setSerialNumber(X509_get_serialNumber(cert.get()), serial.get()))
         return false;
 
     // Fill the certificate with the required properties
     if (!buildCertificate(cert, properties))
         return false;
 
     int ret = 0;
     // Set issuer name, from CA or our subject name for self signed cert
     if (properties.signAlgorithm != Ssl::algSignSelf && properties.signWithX509.get())
         ret = X509_set_issuer_name(cert.get(), X509_get_subject_name(properties.signWithX509.get()));
     else // Self signed certificate, set issuer to self
         ret = X509_set_issuer_name(cert.get(), X509_get_subject_name(cert.get()));
     if (!ret)

=== modified file 'src/ssl/support.cc'
--- src/ssl/support.cc	2016-07-11 10:57:11 +0000
+++ src/ssl/support.cc	2016-07-21 12:47:30 +0000
@@ -905,43 +905,43 @@
         X509 *cert = sk_X509_value(chain, i);
         PEM_write_bio_X509(mem, cert);
     }
 
     len = BIO_get_mem_data(mem, &ptr);
 
     str = (char *)xmalloc(len + 1);
     memcpy(str, ptr, len);
     str[len] = '\0';
 
     BIO_free(mem);
 
     return str;
 }
 
 /// Create SSL context and apply ssl certificate and private key to it.
 Security::ContextPtr
 Ssl::createSSLContext(Security::CertPointer & x509, Ssl::EVP_PKEY_Pointer & pkey, AnyP::PortCfg &port)
 {
 #if (OPENSSL_VERSION_NUMBER >= 0x10100000L)
-    Security::ContextPointer sslContext(SSL_CTX_new(TLS_server_method()));
+    Security::ContextPointer sslContext(Security::ContextPointer::MakeFromLockedOpenSslPointer(SSL_CTX_new(TLS_server_method())));
 #else
-    Security::ContextPointer sslContext(SSL_CTX_new(SSLv23_server_method()));
+    Security::ContextPointer sslContext(Security::ContextPointer::MakeFromLockedOpenSslPointer(SSL_CTX_new(SSLv23_server_method())));
 #endif
 
     if (!SSL_CTX_use_certificate(sslContext.get(), x509.get()))
         return NULL;
 
     if (!SSL_CTX_use_PrivateKey(sslContext.get(), pkey.get()))
         return NULL;
 
     if (!configureSslContext(sslContext.get(), port))
         return NULL;
 
     return sslContext.release();
 }
 
 Security::ContextPtr
 Ssl::generateSslContextUsingPkeyAndCertFromMemory(const char * data, AnyP::PortCfg &port)
 {
     Security::CertPointer cert;
     Ssl::EVP_PKEY_Pointer pkey;
     if (!readCertAndPrivateKeyFromMemory(cert, pkey, data) || !cert || !pkey)



More information about the squid-dev mailing list