Copilot commented on code in PR #13711:
URL: https://github.com/apache/trafficserver/pull/13711#discussion_r4074152239
##########
src/iocore/net/SSLSessionCache.cc:
##########
@@ -54,12 +54,12 @@ SSLOriginSessionCache::~SSLOriginSessionCache()
TS_NO_THREAD_SAFETY_ANALYSIS //
void
SSLOriginSessionCache::insert_session(const std::string &lookup_key,
SSL_SESSION *sess, SSL *ssl)
{
- size_t len = i2d_SSL_SESSION(sess, nullptr); // make sure we're not going to
need more than SSL_MAX_ORIG_SESSION_SIZE bytes
+ size_t len = i2d_SSL_SESSION(sess, nullptr);
/* do not cache a session that's too big. */
- if (len > static_cast<size_t>(SSL_MAX_ORIG_SESSION_SIZE)) {
- Dbg(dbg_ctl_ssl_origin_session_cache, "Unable to save SSL session because
size of %zd exceeds the max of %d", len,
- SSL_MAX_ORIG_SESSION_SIZE);
+ if (len > SSLConfigParams::origin_session_max_size) {
+ Dbg(dbg_ctl_ssl_origin_session_cache, "Unable to save SSL session because
size of %zd exceeds the max of %zu", len,
+ SSLConfigParams::origin_session_max_size);
return;
} else if (len == 0) {
Dbg(dbg_ctl_ssl_origin_session_cache, "Unable to save SSL session because
size is 0");
Review Comment:
i2d_SSL_SESSION() returns an int and can fail with a negative value; storing
it in a size_t can wrap to a huge number and misclassify failures as 'too big'.
Capture the return value in an int, treat <= 0 as failure, and only cast to
size_t after validating the value.
##########
src/iocore/net/P_SSLConfig.h:
##########
@@ -73,6 +73,7 @@ struct SSLConfigParams : public ConfigInfo {
int verify_depth;
int ssl_origin_session_cache{0};
int ssl_origin_session_cache_size{0};
+ int ssl_origin_session_max_size{0};
Review Comment:
ssl_origin_session_max_size is stored as an int in the instance config but
ultimately published into a size_t static (origin_session_max_size). Using
size_t for ssl_origin_session_max_size as well would keep types consistent and
avoid signed/unsigned conversion hazards when this value is consumed.
##########
src/records/RecordsConfig.cc:
##########
@@ -1245,6 +1245,9 @@ static constexpr RecordElement RecordsConfig[] =
,
{RECT_CONFIG, "proxy.config.ssl.origin_session_cache.size", RECD_INT,
"10240", RECU_RESTART_TS, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
,
+ {RECT_CONFIG, "proxy.config.ssl.origin_session_cache.max_session_size",
RECD_INT, "8192", RECU_RESTART_TS, RR_NULL,
+ RECC_INT, "[0-65536]", RECA_NULL}
Review Comment:
The validation range allows 0, but a configured max of 0 effectively
prevents caching any non-empty session (since any len > 0 will exceed it). If
that behavior isn’t explicitly intended, tighten the range to [1-65536]. If it
is intended, the admin guide should clearly document the semantics of 0 (e.g.,
'disable caching by size') to avoid accidental misconfiguration.
##########
tests/gold_tests/tls/tls_origin_session_reuse_large.test.py:
##########
@@ -0,0 +1,145 @@
+'''
+'''
Review Comment:
The file starts with an empty triple-quoted string literal, which is
effectively an empty module docstring and doesn’t add information. Consider
removing it, or replace it with a short module-level docstring describing the
test’s intent (the detailed description is already in Test.Summary).
--
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]
