Copilot commented on code in PR #13026:
URL: https://github.com/apache/trafficserver/pull/13026#discussion_r2998968283
##########
src/iocore/net/SSLConfig.cc:
##########
@@ -454,6 +454,7 @@ SSLConfigParams::initialize()
SSLConfigParams::origin_session_cache_size = ssl_origin_session_cache_size;
if (ssl_origin_session_cache == 1 && ssl_origin_session_cache_size > 0) {
+ delete origin_sess_cache;
origin_sess_cache = new SSLOriginSessionCache();
Review Comment:
Deleting and re-allocating the global `origin_sess_cache` during
`SSLConfigParams::initialize()` is unsafe on live reload: TLS handshakes on
other threads can still be using `origin_sess_cache` (via
`ssl_new_session_callback()` /
`TLSSessionResumptionSupport::getOriginSession()`), leading to use-after-free
once the old cache is deleted. Prefer keeping `origin_sess_cache` stable for
the process lifetime (allocate once, then clear/resize under its internal
lock), or change the global to an atomically-swapped ref-counted handle (e.g.,
`std::shared_ptr` with atomic load/store) so the old cache isn't freed until no
thread can reference it.
```suggestion
if (origin_sess_cache == nullptr) {
origin_sess_cache = new SSLOriginSessionCache();
}
```
##########
src/iocore/net/SSLSessionCache.cc:
##########
@@ -43,7 +43,13 @@ SSLSessDeleter(SSL_SESSION *_p)
SSLOriginSessionCache::SSLOriginSessionCache() {}
-SSLOriginSessionCache::~SSLOriginSessionCache() {}
+SSLOriginSessionCache::~SSLOriginSessionCache()
+{
+ while (auto *node = orig_sess_que.pop()) {
+ delete node;
+ }
+ orig_sess_map.clear();
+}
Review Comment:
`SSLOriginSessionCache::~SSLOriginSessionCache()` mutates `orig_sess_que` /
`orig_sess_map` without taking `mutex`. With this PR also deleting the global
`origin_sess_cache` during reconfigure, the destructor can run concurrently
with `insert_session()` / `get_session()` / `remove_session()`, causing a data
race / use-after-free. Make destruction (or an explicit `clear()` used by
reconfigure) take a `std::unique_lock<ts::shared_mutex>` and ensure callers
cannot access the cache while it is being torn down (e.g., keep a single
long-lived instance and clear it under lock instead of deleting it).
--
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]
