Copilot commented on code in PR #12763:
URL: https://github.com/apache/trafficserver/pull/12763#discussion_r2855690567
##########
src/iocore/net/TLSBasicSupport.cc:
##########
@@ -72,6 +72,38 @@ TLSBasicSupport::clear()
{
this->_tls_handshake_begin_time = 0;
this->_tls_handshake_end_time = 0;
+ this->_tls_handshake_bytes_in = 0;
+ this->_tls_handshake_bytes_out = 0;
+}
+
+// Returns true only on the first call that reads from the BIOs, so callers
+// can distinguish fresh values (for metrics) from cached ones.
+bool
+TLSBasicSupport::get_tls_handshake_bytes(uint64_t &bytes_in, uint64_t
&bytes_out) const
+{
+ if (_tls_handshake_bytes_in > 0 || _tls_handshake_bytes_out > 0) {
+ bytes_in = _tls_handshake_bytes_in;
+ bytes_out = _tls_handshake_bytes_out;
+ return false;
+ }
+
+ SSL *ssl = this->_get_ssl_object();
+ if (ssl == nullptr) {
+ bytes_in = 0;
+ bytes_out = 0;
+ return false;
+ }
+
+ BIO *rbio = SSL_get_rbio(ssl);
+ BIO *wbio = SSL_get_wbio(ssl);
+
+ uint64_t bio_in = rbio ? BIO_number_read(rbio) : 0;
+ uint64_t bio_out = wbio ? BIO_number_written(wbio) : 0;
+
+ bytes_in = _tls_handshake_bytes_in = bio_in;
+ bytes_out = _tls_handshake_bytes_out = bio_out;
+
+ return true;
Review Comment:
`get_tls_handshake_bytes()` uses `(_tls_handshake_bytes_in > 0 ||
_tls_handshake_bytes_out > 0)` as its “already cached” sentinel. This doesn’t
match the function contract ("returns true only on the first call") if both
values can legitimately be 0 (e.g., handshake aborted very early), because
subsequent calls will re-read the BIOs and return `true` again. Consider adding
an explicit cached flag (e.g. `mutable bool _tls_handshake_bytes_cached`) so
the return value is deterministic and independent of the byte counts.
##########
src/proxy/logging/LogAccess.cc:
##########
@@ -2430,6 +2461,44 @@ LogAccess::marshal_client_security_alpn(char *buf)
return round_len;
}
+/*-------------------------------------------------------------------------
+ TLS Handshake Bytes - Bytes received from client during TLS handshake
+ -------------------------------------------------------------------------*/
+int
+LogAccess::marshal_client_tls_handshake_bytes_rx(char *buf)
+{
+ if (buf) {
+ marshal_int(buf,
m_http_sm->get_user_agent().get_client_tls_handshake_bytes_rx());
+ }
+ return INK_MIN_ALIGN;
+}
+
+/*-------------------------------------------------------------------------
+ TLS Handshake Bytes - Bytes sent to client during TLS handshake
+ -------------------------------------------------------------------------*/
+int
+LogAccess::marshal_client_tls_handshake_bytes_tx(char *buf)
+{
+ if (buf) {
+ marshal_int(buf,
m_http_sm->get_user_agent().get_client_tls_handshake_bytes_tx());
+ }
Review Comment:
`marshal_int` takes an `int64_t`, but these fields pass `uint64_t` directly.
Even if handshake sizes are usually small, implicit signed conversion can
truncate/overflow if the value exceeds `INT64_MAX`. Consider explicitly casting
(and/or clamping) to `int64_t` to match the logging integer type and avoid
implementation-defined conversions.
##########
src/proxy/logging/LogAccess.cc:
##########
@@ -2206,6 +2206,37 @@ LogAccess::marshal_client_req_squid_len(char *buf)
return INK_MIN_ALIGN;
}
+/*-------------------------------------------------------------------------
+ Client request squid length plus TLS handshake bytes received for TLS
connections.
+ For TLS 1.3 early data (0-RTT), we subtract the early data length from the
+ handshake bytes to avoid double-counting since the early data bytes are
+ already included in client_request_body_bytes.
+ -------------------------------------------------------------------------*/
+int
+LogAccess::marshal_client_req_squid_len_tls(char *buf)
+{
+ if (buf) {
+ int64_t val = 0;
+
+ if (m_client_request) {
+ val = m_client_request->length_get() +
m_http_sm->client_request_body_bytes;
+ }
+
+ if (!m_http_sm->get_user_agent().get_client_tcp_reused()) {
+ uint64_t handshake_rx =
m_http_sm->get_user_agent().get_client_tls_handshake_bytes_rx();
+ size_t early_data_len =
m_http_sm->get_user_agent().get_client_tls_early_data_len();
+
+ if (early_data_len > 0 && handshake_rx > early_data_len) {
+ handshake_rx -= early_data_len;
+ }
+
+ val += handshake_rx;
+ }
Review Comment:
The early-data adjustment currently only subtracts when `handshake_rx >
early_data_len`. If `handshake_rx == early_data_len` (or if early data ever
exceeds the measured handshake bytes), the early-data bytes will still be
double-counted in `cqqtl`. Consider subtracting `min(handshake_rx,
early_data_len)` (or at least using `>=`) to ensure early data is always
removed when present.
--
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]
