Copilot commented on code in PR #13097:
URL: https://github.com/apache/trafficserver/pull/13097#discussion_r3108374635
##########
src/iocore/net/OCSPStapling.cc:
##########
@@ -831,12 +848,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);
Review Comment:
This file now uses `std::lock_guard`, but it doesn't include `<mutex>`
directly. To avoid relying on transitive includes (which can vary by standard
library implementation), add the explicit standard header include for the
locking helpers being used here.
##########
src/iocore/net/OCSPStapling.cc:
##########
@@ -1370,56 +1372,69 @@ ssl_callback_ocsp_stapling(SSL *ssl, void *)
return SSL_TLSEXT_ERR_NOACK;
}
- // Fetch the specific certificate used in this negotiation
- X509 *cert = SSL_get_certificate(ssl);
- if (!cert) {
- Error("ssl_callback_ocsp_stapling: failed to get certificate");
- return SSL_TLSEXT_ERR_NOACK;
- }
-
certinfo *cinf = nullptr;
-#if HAVE_NATIVE_DUAL_CERT_SUPPORT
- certinfo_map::iterator iter = map->find(cert);
- if (iter != map->end()) {
- cinf = iter->second;
- }
-#else
- for (certinfo_map::iterator iter = map->begin(); iter != map->end(); ++iter)
{
- X509 *key = iter->first;
- if (key == nullptr) {
- continue;
+
+ // Fast path: if only one certificate in the map, skip SSL_get_certificate()
lookup
+ if (map->size() == 1) {
+ cinf = map->begin()->second;
+ } else {
+ // Fetch the specific certificate used in this negotiation
+ X509 *cert = SSL_get_certificate(ssl);
+ if (!cert) {
+ Error("ssl_callback_ocsp_stapling: failed to get certificate");
+ return SSL_TLSEXT_ERR_NOACK;
}
- if (X509_cmp(key, cert) == 0) {
+#if HAVE_NATIVE_DUAL_CERT_SUPPORT
+ certinfo_map::iterator iter = map->find(cert);
+ if (iter != map->end()) {
cinf = iter->second;
- break;
}
- }
+#else
+ for (certinfo_map::iterator iter = map->begin(); iter != map->end();
++iter) {
+ X509 *key = iter->first;
+ if (key == nullptr) {
+ continue;
+ }
+
+ if (X509_cmp(key, cert) == 0) {
+ cinf = iter->second;
+ break;
+ }
+ }
#endif
+ }
if (cinf == nullptr) {
Error("ssl_callback_ocsp_stapling: failed to get certificate information
for ssl=%p", ssl);
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;
+
+ {
+ ts::bravo::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);
Review Comment:
The new reader path copies the OCSP response twice (into the fixed-size
stack buffer, then into the heap buffer passed to OpenSSL). Since this runs on
the TLS handshake hot path, consider whether you can avoid the extra copy while
still keeping lock hold times short (e.g., allocate once and copy once).
##########
src/iocore/net/OCSPStapling.cc:
##########
@@ -279,17 +281,38 @@ namespace
// Cached info stored in SSL_CTX ex_info
struct certinfo {
- unsigned char idx[20]; // Index in session cache SHA1 hash of certificate
- TS_OCSP_CERTID *cid; // Certificate ID for OCSP requests or nullptr if
ID cannot be determined
- 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;
+ unsigned char idx[20] = {}; // Index in session cache SHA1 hash
of certificate
+ TS_OCSP_CERTID *cid = nullptr; // Certificate ID for OCSP requests
+ char *uri = nullptr; // Responder details
+ char *certname = nullptr;
+ char *user_agent = nullptr;
+ bool is_prefetched = false;
+
+ // 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 = 0;
+ bool is_expire = true;
+ time_t expire_time = 0;
+ mutable ts::bravo::shared_mutex resp_mutex;
Review Comment:
`ts::bravo::shared_mutex` embeds an array of 256 cacheline-aligned reader
slots (see tsutil/Bravo.h), so storing it inline in each `certinfo`
significantly increases per-certificate memory footprint (~16KB just for the
slot array, in addition to the 10KB `resp_der`). If deployments can have many
SSL_CTX/SNI certs, consider whether a single mutex per `certinfo_map` (or
another shared location) would provide the same concurrency benefits with less
memory overhead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
