This is an automated email from the ASF dual-hosted git repository. cmcfarlen pushed a commit to branch 10.0.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit a876bf94dd4fc903af5be5c30b1da434872737d1 Author: Walt Karas <[email protected]> AuthorDate: Wed Jun 12 19:37:07 2024 -0400 Clean up destination buffer overrun handling in logging unmarshalling. (#11437) Plus other miscellaneous cleanup/improvement. (cherry picked from commit d53976b82be7704a325139352be09f90d38dd387) --- src/proxy/logging/LogAccess.cc | 68 ++++++++++++++++++++++++++++++++---------- src/proxy/logging/LogUtils.cc | 12 ++++---- 2 files changed, 59 insertions(+), 21 deletions(-) diff --git a/src/proxy/logging/LogAccess.cc b/src/proxy/logging/LogAccess.cc index 7fd23a462b..9a5075b3ee 100644 --- a/src/proxy/logging/LogAccess.cc +++ b/src/proxy/logging/LogAccess.cc @@ -48,9 +48,13 @@ namespace { DbgCtl dbg_ctl_log_escape{"log-escape"}; DbgCtl dbg_ctl_log_resolve{"log-resolve"}; +DbgCtl dbg_ctl_log_unmarshal_orun{"log-unmarshal-orun"}; // Overrrun of unmarshaling destination buffer. +DbgCtl dbg_ctl_log_unmarshal_data{"log-unmarshal-data"}; // Error in txn data when unmarshalling. } // end anonymous namespace +#define DBG_UNMARSHAL_DEST_OVERRUN Dbg(dbg_ctl_log_unmarshal_orun, "Unmarshal destination buffer overrun."); + /*------------------------------------------------------------------------- LogAccess @@ -446,6 +450,7 @@ LogAccess::unmarshal_with_map(int64_t code, char *dest, int len, const Ptr<LogFi if (codeStrLen < bufSize && codeStrLen < len) { ink_strlcpy(dest, invalidCodeMsg, len); } else { + DBG_UNMARSHAL_DEST_OVERRUN codeStrLen = -1; } } else { @@ -453,6 +458,7 @@ LogAccess::unmarshal_with_map(int64_t code, char *dest, int len, const Ptr<LogFi } break; case LogFieldAliasMap::BUFFER_TOO_SMALL: + DBG_UNMARSHAL_DEST_OVERRUN codeStrLen = -1; break; } @@ -579,6 +585,7 @@ LogAccess::unmarshal_int_to_str(char **buf, char *dest, int len) memcpy(dest, val_buf + 128 - val_len, val_len); return val_len; } + DBG_UNMARSHAL_DEST_OVERRUN return -1; } @@ -603,6 +610,7 @@ LogAccess::unmarshal_int_to_str_hex(char **buf, char *dest, int len) memcpy(dest, val_buf + 128 - val_len, val_len); return val_len; } + DBG_UNMARSHAL_DEST_OVERRUN return -1; } @@ -735,6 +743,7 @@ unmarshal_str_json(char **buf, char *dest, int len, LogSlice *slice) } if (n >= len) { + DBG_UNMARSHAL_DEST_OVERRUN return -1; } @@ -745,6 +754,7 @@ unmarshal_str_json(char **buf, char *dest, int len, LogSlice *slice) escape_json(dest, val_buf, escaped_len); return escaped_len; } + DBG_UNMARSHAL_DEST_OVERRUN return -1; } @@ -784,6 +794,7 @@ LogAccess::unmarshal_str(char **buf, char *dest, int len, LogSlice *slice, LogEs } if (n >= len) { + DBG_UNMARSHAL_DEST_OVERRUN return -1; } @@ -795,6 +806,7 @@ LogAccess::unmarshal_str(char **buf, char *dest, int len, LogSlice *slice, LogEs memcpy(dest, val_buf, val_len); return val_len; } + DBG_UNMARSHAL_DEST_OVERRUN return -1; } @@ -806,8 +818,11 @@ LogAccess::unmarshal_ttmsf(char **buf, char *dest, int len) ink_assert(dest != nullptr); int64_t val = unmarshal_int(buf); - double secs = static_cast<double>(val) / 1000; - int val_len = snprintf(dest, len, "%.3f", secs); + int val_len = snprintf(dest, len, "%" PRId64 ".%03d", val / 1000, int((val < 0 ? -val : val) % 1000)); + if (val_len >= len) { + DBG_UNMARSHAL_DEST_OVERRUN + return -1; + } return val_len; } @@ -822,6 +837,11 @@ LogAccess::unmarshal_int_to_date_str(char **buf, char *dest, int len) char *strval = LogUtils::timestamp_to_date_str(value); int strlen = static_cast<int>(::strlen(strval)); + if (strlen > len) { + DBG_UNMARSHAL_DEST_OVERRUN + return -1; + } + memcpy(dest, strval, strlen); return strlen; } @@ -837,6 +857,11 @@ LogAccess::unmarshal_int_to_time_str(char **buf, char *dest, int len) char *strval = LogUtils::timestamp_to_time_str(value); int strlen = static_cast<int>(::strlen(strval)); + if (strlen > len) { + DBG_UNMARSHAL_DEST_OVERRUN + return -1; + } + memcpy(dest, strval, strlen); return strlen; } @@ -864,6 +889,11 @@ LogAccess::unmarshal_int_to_netscape_str(char **buf, char *dest, int len) char *strval = LogUtils::timestamp_to_netscape_str(value); int strlen = static_cast<int>(::strlen(strval)); + if (strlen > len) { + DBG_UNMARSHAL_DEST_OVERRUN + return -1; + } + memcpy(dest, strval, strlen); return strlen; } @@ -904,31 +934,35 @@ LogAccess::unmarshal_http_version(char **buf, char *dest, int len) ink_assert(*buf != nullptr); ink_assert(dest != nullptr); - static const char *http = "HTTP/"; - static int http_len = static_cast<int>(::strlen(http)); + static const char http[] = "HTTP/"; + static int http_len = static_cast<int>(sizeof(http) - 1); char val_buf[128]; char *p = val_buf; + auto vb_left = [&]() -> int { return sizeof(val_buf) - (p - val_buf); }; + memcpy(p, http, http_len); p += http_len; - int res1 = unmarshal_int_to_str(buf, p, 128 - http_len); + int res1 = unmarshal_int_to_str(buf, p, vb_left()); if (res1 < 0) { return -1; } p += res1; *p++ = '.'; - int res2 = unmarshal_int_to_str(buf, p, 128 - http_len - res1 - 1); + int res2 = unmarshal_int_to_str(buf, p, vb_left()); if (res2 < 0) { + DBG_UNMARSHAL_DEST_OVERRUN return -1; } - int val_len = http_len + res1 + res2 + 1; + int val_len = p - val_buf; if (val_len < len) { memcpy(dest, val_buf, val_len); return val_len; } + DBG_UNMARSHAL_DEST_OVERRUN return -1; } @@ -990,6 +1024,7 @@ LogAccess::unmarshal_http_status(char **buf, char *dest, int len) memcpy(dest, val_buf + 128 - val_len, val_len); return val_len; } + DBG_UNMARSHAL_DEST_OVERRUN return -1; } @@ -1036,18 +1071,19 @@ int LogAccess::unmarshal_ip_to_str(char **buf, char *dest, int len) { IpEndpoint ip; - int zret = -1; if (len > 0) { unmarshal_ip(buf, &ip); if (!ats_is_ip(&ip)) { *dest = '0'; - zret = 1; + Dbg(dbg_ctl_log_unmarshal_data, "Invalid IP address"); + return 1; } else if (ats_ip_ntop(&ip, dest, len)) { - zret = static_cast<int>(::strlen(dest)); + return static_cast<int>(::strlen(dest)); } } - return zret; + DBG_UNMARSHAL_DEST_OVERRUN + return -1; } /*------------------------------------------------------------------------- @@ -1061,19 +1097,20 @@ LogAccess::unmarshal_ip_to_str(char **buf, char *dest, int len) int LogAccess::unmarshal_ip_to_hex(char **buf, char *dest, int len) { - int zret = -1; IpEndpoint ip; if (len > 0) { unmarshal_ip(buf, &ip); if (!ats_is_ip(&ip)) { *dest = '0'; - zret = 1; + Dbg(dbg_ctl_log_unmarshal_data, "Invalid IP address"); + return 1; } else { - zret = ats_ip_to_hex(&ip.sa, dest, len); + return ats_ip_to_hex(&ip.sa, dest, len); } } - return zret; + DBG_UNMARSHAL_DEST_OVERRUN + return -1; } /*------------------------------------------------------------------------- @@ -1173,6 +1210,7 @@ LogAccess::unmarshal_record(char **buf, char *dest, int len) memcpy(dest, val_buf, val_len); return val_len; } + DBG_UNMARSHAL_DEST_OVERRUN return -1; } diff --git a/src/proxy/logging/LogUtils.cc b/src/proxy/logging/LogUtils.cc index 175c187410..ad84781cfa 100644 --- a/src/proxy/logging/LogUtils.cc +++ b/src/proxy/logging/LogUtils.cc @@ -97,8 +97,8 @@ LogUtils::timestamp_to_str(long timestamp, char *buf, int size) char * LogUtils::timestamp_to_netscape_str(long timestamp) { - static char timebuf[64]; // NOTE: not MT safe - static long last_timestamp = 0; + static thread_local char timebuf[64]; + static thread_local long last_timestamp = 0; // safety check if (timestamp < 0) { @@ -149,8 +149,8 @@ LogUtils::timestamp_to_netscape_str(long timestamp) char * LogUtils::timestamp_to_date_str(long timestamp) { - static char timebuf[64]; // NOTE: not MT safe - static long last_timestamp = 0; + static thread_local char timebuf[64]; + static thread_local long last_timestamp = -1L; // safety check if (timestamp < 0) { @@ -181,8 +181,8 @@ LogUtils::timestamp_to_date_str(long timestamp) char * LogUtils::timestamp_to_time_str(long timestamp) { - static char timebuf[64]; // NOTE: not MT safe - static long last_timestamp = 0; + static thread_local char timebuf[64]; + static thread_local long last_timestamp = 0; // safety check if (timestamp < 0) {
