ocsp: replace stapling_mutex with std::shared_mutex and fix leaks

Replace the per-certinfo ink_mutex with a std::shared_mutex so readers
on the TLS handshake hot path (ssl_callback_ocsp_stapling) and the
refresh scan (ocsp_update) can run concurrently; only the cache update
takes an exclusive lock. The handshake reader now copies the DER staple
to a stack buffer under the shared lock and allocates outside the
critical section.

Because the mutex is no longer a C type, certinfo is allocated with
new/delete instead of OPENSSL_malloc.

Also fix three pre-existing bugs:
- certinfo_map_free never freed cinf->cid (leak on every CTX teardown).
- On BoringSSL the X509 key ref taken via X509_up_ref was never
  released; free it in certinfo_map_free.
- ssl_stapling_init_cert's error path could delete a certinfo_map still
  owned by the SSL_CTX. Only delete a map this call created.

Author: c-taylor

src/iocore/net/OCSPStapling.cc

@@ -21,6 +21,9 @@
 
 #include "P_OCSPStapling.h"
 
+#include <mutex>
+#include <shared_mutex>
+
 #include <openssl/ssl.h>
 #include <openssl/x509v3.h>
 #include <openssl/asn1.h>
@@ -284,12 +287,15 @@ struct certinfo {
   char           *uri;     // Responder details
   char           *certname;
   char           *user_agent;
-  ink_mutex       stapling_mutex;
-  unsigned char   resp_der[MAX_STAPLING_DER];
-  unsigned int    resp_derlen;
   bool            is_prefetched;
-  bool            is_expire;
-  time_t          expire_time;
+
+  // OCSP response data, protected by resp_mutex.
+  // Readers take a shared lock; the updater takes an exclusive lock.
+  unsigned char             resp_der[MAX_STAPLING_DER];
+  unsigned int              resp_derlen;
+  bool                      is_expire;
+  time_t                    expire_time;
+  mutable std::shared_mutex resp_mutex;
 };
 
 class HTTPRequest : public Continuation
@@ -737,15 +743,20 @@ certinfo_map_free(void * /*parent*/, void *ptr, CRYPTO_EX_DATA * /*ad*/, int /*i
 
   for (auto &iter : *map) {
     certinfo *cinf = iter.second;
+    if (cinf->cid) {
+      TS_OCSP_CERTID_free(cinf->cid);
+    }
     if (cinf->uri) {
       OPENSSL_free(cinf->uri);
     }
 
     ats_free(cinf->certname);
     ats_free(cinf->user_agent);
 
-    ink_mutex_destroy(&cinf->stapling_mutex);
-    OPENSSL_free(cinf);
+#ifdef OPENSSL_IS_BORINGSSL
+    X509_free(iter.first);
+#endif
+    delete cinf;
   }
   delete map;
 }
@@ -831,12 +842,13 @@ stapling_cache_response(TS_OCSP_RESPONSE *rsp, certinfo *cinf)
     return false;
   }
 
-  ink_mutex_acquire(&cinf->stapling_mutex);
-  memcpy(cinf->resp_der, resp_der, resp_derlen);
-  cinf->resp_derlen = resp_derlen;
-  cinf->is_expire   = false;
-  cinf->expire_time = time(nullptr) + SSLConfigParams::ssl_ocsp_cache_timeout;
-  ink_mutex_release(&cinf->stapling_mutex);
+  {
+    std::lock_guard lock(cinf->resp_mutex);
+    memcpy(cinf->resp_der, resp_der, resp_derlen);
+    cinf->resp_derlen = resp_derlen;
+    cinf->is_expire   = false;
+    cinf->expire_time = time(nullptr) + SSLConfigParams::ssl_ocsp_cache_timeout;
+  }
 
   Dbg(dbg_ctl_ssl_ocsp, "stapling_cache_response: success to cache response");
   return true;
@@ -860,15 +872,12 @@ ssl_stapling_init_cert(SSL_CTX *ctx, X509 *cert, const char *certname, const cha
     return false;
   }
 
+  bool map_is_new = false;
   if (!map) {
-    map = new certinfo_map;
-  }
-  certinfo *cinf = static_cast<certinfo *>(OPENSSL_malloc(sizeof(certinfo)));
-  if (!cinf) {
-    Error("error allocating memory for %s", certname);
-    delete map;
-    return false;
+    map        = new certinfo_map;
+    map_is_new = true;
   }
+  certinfo *cinf = new certinfo();
 
   // Initialize certinfo
   cinf->cid      = nullptr;
@@ -877,8 +886,7 @@ ssl_stapling_init_cert(SSL_CTX *ctx, X509 *cert, const char *certname, const cha
   if (SSLConfigParams::ssl_ocsp_user_agent != nullptr) {
     cinf->user_agent = ats_strdup(SSLConfigParams::ssl_ocsp_user_agent);
   }
-  cinf->resp_derlen = 0;
-  ink_mutex_init(&cinf->stapling_mutex);
+  cinf->resp_derlen   = 0;
   cinf->is_prefetched = rsp_file ? true : false;
   cinf->is_expire     = true;
   cinf->expire_time   = 0;
@@ -963,10 +971,11 @@ ssl_stapling_init_cert(SSL_CTX *ctx, X509 *cert, const char *certname, const cha
   ats_free(cinf->certname);
   ats_free(cinf->user_agent);
 
-  if (cinf) {
-    OPENSSL_free(cinf);
-  }
-  if (map) {
+  delete cinf;
+
+  // Only tear down the map if this call created it; an existing map is still
+  // owned by the SSL_CTX.
+  if (map_is_new) {
     delete map;
   }
 
@@ -1327,20 +1336,21 @@ ocsp_update()
           if (map) {
             // Walk over all certs associated with this CTX
             for (auto &iter : *map) {
-              cinf = iter.second;
-              ink_mutex_acquire(&cinf->stapling_mutex);
+              cinf         = iter.second;
               current_time = time(nullptr);
-              if (cinf->resp_derlen == 0 || cinf->is_expire || cinf->expire_time < current_time) {
-                ink_mutex_release(&cinf->stapling_mutex);
+              bool needs_refresh;
+              {
+                std::shared_lock lock(cinf->resp_mutex);
+                needs_refresh = cinf->resp_derlen == 0 || cinf->is_expire || cinf->expire_time < current_time;
+              }
+              if (needs_refresh) {
                 if (stapling_refresh_response(cinf, &resp)) {
                   Dbg(dbg_ctl_ssl_ocsp, "Successfully refreshed OCSP for %s certificate. url=%s", cinf->certname, cinf->uri);
                   Metrics::Counter::increment(ssl_rsb.ocsp_refreshed_cert);
                 } else {
                   Error("Failed to refresh OCSP for %s certificate. url=%s", cinf->certname, cinf->uri);
                   Metrics::Counter::increment(ssl_rsb.ocsp_refresh_cert_failure);
                 }
-              } else {
-                ink_mutex_release(&cinf->stapling_mutex);
               }
             }
           }
@@ -1402,24 +1412,31 @@ ssl_callback_ocsp_stapling(SSL *ssl, void *)
     return SSL_TLSEXT_ERR_NOACK;
   }
 
-  ink_mutex_acquire(&cinf->stapling_mutex);
-  time_t current_time = time(nullptr);
-  if ((cinf->resp_derlen == 0 || cinf->is_expire) || (cinf->expire_time < current_time && !cinf->is_prefetched)) {
-    ink_mutex_release(&cinf->stapling_mutex);
-    Error("ssl_callback_ocsp_stapling: failed to get certificate status for %s", cinf->certname);
-    return SSL_TLSEXT_ERR_NOACK;
-  } else {
-    unsigned char *p = static_cast<unsigned char *>(OPENSSL_malloc(cinf->resp_derlen));
-    if (p == nullptr) {
-      ink_mutex_release(&cinf->stapling_mutex);
-      Dbg(dbg_ctl_ssl_ocsp, "ssl_callback_ocsp_stapling: failed to allocate memory for %s", cinf->certname);
+  unsigned char resp_copy[MAX_STAPLING_DER];
+  unsigned int  resp_copylen;
+
+  {
+    std::shared_lock lock(cinf->resp_mutex);
+
+    time_t current_time = time(nullptr);
+    if (cinf->resp_derlen == 0 || cinf->is_expire || (cinf->expire_time < current_time && !cinf->is_prefetched)) {
+      Error("ssl_callback_ocsp_stapling: failed to get certificate status for %s", cinf->certname);
       return SSL_TLSEXT_ERR_NOACK;
     }
-    memcpy(p, cinf->resp_der, cinf->resp_derlen);
-    ink_mutex_release(&cinf->stapling_mutex);
-    SSL_set_tlsext_status_ocsp_resp(ssl, p, cinf->resp_derlen);
-    Dbg(dbg_ctl_ssl_ocsp, "ssl_callback_ocsp_stapling: successfully got certificate status for %s", cinf->certname);
-    Dbg(dbg_ctl_ssl_ocsp, "is_prefetched:%d uri:%s", cinf->is_prefetched, cinf->uri);
-    return SSL_TLSEXT_ERR_OK;
+
+    resp_copylen = cinf->resp_derlen;
+    memcpy(resp_copy, cinf->resp_der, resp_copylen);
+  }
+
+  unsigned char *p = static_cast<unsigned char *>(OPENSSL_malloc(resp_copylen));
+  if (p == nullptr) {
+    Dbg(dbg_ctl_ssl_ocsp, "ssl_callback_ocsp_stapling: failed to allocate memory for %s", cinf->certname);
+    return SSL_TLSEXT_ERR_NOACK;
   }
+  memcpy(p, resp_copy, resp_copylen);
+  SSL_set_tlsext_status_ocsp_resp(ssl, p, resp_copylen);
+
+  Dbg(dbg_ctl_ssl_ocsp, "ssl_callback_ocsp_stapling: successfully got certificate status for %s", cinf->certname);
+  Dbg(dbg_ctl_ssl_ocsp, "is_prefetched:%d uri:%s", cinf->is_prefetched, cinf->uri);
+  return SSL_TLSEXT_ERR_OK;
 }