bneradt commented on code in PR #13490:
URL: https://github.com/apache/trafficserver/pull/13490#discussion_r3716054961
##########
src/iocore/net/OCSPStapling.cc:
##########
@@ -848,12 +852,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;
+ }
Review Comment:
The writer path now explicitly uses std::lock_guard<ts::bravo::shared_mutex>.
##########
src/iocore/net/OCSPStapling.cc:
##########
@@ -1422,37 +1428,44 @@ 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);
- SiteThrottledError("ssl_callback_ocsp_stapling: failed to get certificate
status for %s", cinf->certname);
- return SSL_TLSEXT_ERR_NOACK;
- } else {
-#ifdef OPENSSL_IS_BORINGSSL
- // SSL_set_ocsp_response copies the response, so hand it the cached buffer
directly.
- int set_ok = SSL_set_ocsp_response(ssl, cinf->resp_der, cinf->resp_derlen);
- ink_mutex_release(&cinf->stapling_mutex);
-#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)) {
+ SiteThrottledError("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);
- // Takes ownership of p and frees it on success; on failure it does not.
- int set_ok = SSL_set_tlsext_status_ocsp_resp(ssl, p, cinf->resp_derlen);
- if (set_ok == 0) {
- OPENSSL_free(p);
- }
+
+ resp_copylen = cinf->resp_derlen;
+ memcpy(resp_copy, cinf->resp_der, resp_copylen);
+ }
+
+#ifdef OPENSSL_IS_BORINGSSL
+ // SSL_set_ocsp_response copies the response, so the stack buffer remains
ours.
+ int set_ok = SSL_set_ocsp_response(ssl, resp_copy, resp_copylen);
+#else
+ 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);
Review Comment:
The OpenSSL path now allocates outside the shared lock, revalidates size,
and copies directly once. It resizes and retries if a concurrent refresh grows
the response.
--
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]
