Copilot commented on code in PR #13490:
URL: https://github.com/apache/trafficserver/pull/13490#discussion_r3715299188
##########
src/iocore/net/OCSPStapling.cc:
##########
@@ -282,19 +284,22 @@ 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 = nullptr; // Certificate ID for OCSP requests
- char *uri = nullptr; // Responder details
- char *certname = nullptr;
- char *user_agent = nullptr;
- ink_mutex stapling_mutex;
- unsigned char resp_der[MAX_STAPLING_DER] = {};
- unsigned int resp_derlen = 0;
- bool is_prefetched = false;
- bool is_expire = true;
- time_t expire_time = 0;
-
- certinfo() { ink_mutex_init(&stapling_mutex); }
+ unsigned char idx[20] = {}; // Index in session cache SHA1 hash
of certificate
+ TS_OCSP_CERTID *cid = nullptr; // Certificate ID for OCSP requests
or nullptr if ID cannot be determined
+ 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:
`is_prefetched` is outside the block/commented set of fields protected by
`resp_mutex`, but it participates in staleness decisions alongside
`expire_time` in the stapling callback. If `is_prefetched` can ever be mutated
after initialization, this creates a likely data race / inconsistent read
unless all accesses (reads and writes) are also synchronized via `resp_mutex`
(or made `std::atomic<bool>` / otherwise immutable). Suggested fix: either (a)
include `is_prefetched` in the `resp_mutex`-protected set and always update it
under an exclusive lock, or (b) make it atomic/const and document that it never
changes after concurrent readers begin.
##########
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:
This relies on C++17 class template argument deduction (`std::lock_guard
lock(...)`) and also implicitly assumes `ts::bravo::shared_mutex` satisfies
`BasicLockable` (`lock()`/`unlock()`). To make the intent and compatibility
explicit, prefer an explicit lock type for exclusive locking (e.g.,
`std::lock_guard<ts::bravo::shared_mutex>` or a Bravo-provided exclusive/unique
lock type if available). This also makes it clearer to readers that this is the
writer path.
##########
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:
On the OpenSSL path this introduces an extra full copy on the handshake hot
path (`cinf->resp_der -> resp_copy -> p`). Previously there was only one copy
into `p`. If the goal is to keep allocations outside the lock while minimizing
memcpy cost, consider allocating `p` after snapshotting `resp_copylen`, then
taking a (shared) lock only for the single `memcpy(p, cinf->resp_der,
resp_copylen)` (optionally re-validating staleness/length before copying). This
keeps the critical section short without doubling copy bandwidth.
--
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]
