From 2824b243a97bc5f6056b0107d250313259ce01e9 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Fri, 14 Aug 2026 11:21:40 -0700 Subject: [PATCH 1/7] Fix memory leaks reported by Coverity Scan Each leak is on a path where the leaked allocation is unreachable afterwards, so releasing it changes no observable behavior. - Plugin option parsing: a repeatable command line option overwrote the previously duplicated string. Affects regex_revalidate, remap_purge, xdebug, stale_response and the uri_signing issuer id. Every one of these fields starts out null, so the first pass frees nothing. - TSMgmtStringGet hands back a copy the caller owns. maxmind_acl and an API regression test dropped it. - jax_fingerprint leaked its configuration on three plugin initialization failure paths. - The YAML remap parser duplicated a redirect URL that nothing owned. parse_format_redirect_url copies what it needs, so the local string's storage can be passed directly. - traffic_cache_tool never released its URL set or stripe hash table. Cache is neither copyable nor movable, so the new destructor cannot double free. Verified with a clean build (no new warnings) and the full unit test suite on Fedora, GCC 16.1.1. --- plugins/experimental/jax_fingerprint/plugin.cc | 4 ++++ plugins/experimental/maxmind_acl/mmdb.cc | 5 ++++- plugins/experimental/stale_response/stale_response.cc | 4 ++++ plugins/experimental/uri_signing/config.cc | 2 ++ plugins/regex_revalidate/regex_revalidate.cc | 3 +++ plugins/remap_purge/remap_purge.cc | 4 ++++ plugins/xdebug/xdebug.cc | 1 + src/api/InkAPITest.cc | 3 +++ src/proxy/http/remap/RemapYamlConfig.cc | 4 +++- src/traffic_cache_tool/CacheTool.cc | 11 +++++++++-- 10 files changed, 37 insertions(+), 4 deletions(-) diff --git a/plugins/experimental/jax_fingerprint/plugin.cc b/plugins/experimental/jax_fingerprint/plugin.cc index 374d66faecb..03ee35d2844 100644 --- a/plugins/experimental/jax_fingerprint/plugin.cc +++ b/plugins/experimental/jax_fingerprint/plugin.cc @@ -378,12 +378,14 @@ TSPluginInit(int argc, char const **argv) if (!read_config_option(argc, argv, *config)) { TSError("[%s] Failed to parse options.", PLUGIN_NAME); + delete config; return; } if (!config->log_filename.empty()) { if (!create_log_file(config->log_filename, config->log_handle)) { TSError("[%s] Failed to create log.", PLUGIN_NAME); + delete config; return; } else { Dbg(dbg_ctl, "Created log file."); @@ -465,6 +467,7 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE if (!config->log_filename.empty()) { if (!create_log_file(config->log_filename, config->log_handle)) { TSError("[%s] Failed to create log.", PLUGIN_NAME); + delete config; return TS_ERROR; } else { Dbg(dbg_ctl, "Created log file."); @@ -473,6 +476,7 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE if (reserve_user_arg(*config) == TS_ERROR) { TSError("[%s] Failed to reserve user arg index.", PLUGIN_NAME); + delete config; return TS_ERROR; } diff --git a/plugins/experimental/maxmind_acl/mmdb.cc b/plugins/experimental/maxmind_acl/mmdb.cc index 26aa8070472..4b41c459caa 100644 --- a/plugins/experimental/maxmind_acl/mmdb.cc +++ b/plugins/experimental/maxmind_acl/mmdb.cc @@ -79,10 +79,12 @@ Acl::init(char const *filename) } // Associate our config file with remap.config or .yaml if possible to be able to initiate reloads - TSMgmtString result; + TSMgmtString result = nullptr; const char *var_name = "proxy.config.url_remap_yaml.filename"; if (TS_SUCCESS != TSMgmtStringGet(var_name, &result) || TS_SUCCESS != TSMgmtConfigFileAdd(result, configloc.c_str())) { // Fall back to remap.config + TSfree(result); + result = nullptr; var_name = "proxy.config.url_remap.filename"; if (TS_SUCCESS != TSMgmtStringGet(var_name, &result)) { TSWarning("[%s] Could not retrieve remap filename", PLUGIN_NAME); @@ -90,6 +92,7 @@ Acl::init(char const *filename) TSWarning("[%s] Error adding mgmt config file", PLUGIN_NAME); } } + TSfree(result); // Find our database name and convert to full path as needed status = loaddb(maxmind["database"]); diff --git a/plugins/experimental/stale_response/stale_response.cc b/plugins/experimental/stale_response/stale_response.cc index 80b27082e61..519ab8b8a74 100644 --- a/plugins/experimental/stale_response/stale_response.cc +++ b/plugins/experimental/stale_response/stale_response.cc @@ -1070,6 +1070,10 @@ parse_args(int argc, char const *argv[]) plugin_config->log_info.stale_if_error = true; break; case 'd': + // The option may be repeated; release the previously duplicated name first. + if (plugin_config->log_info.filename != PLUGIN_TAG) { + free(const_cast(plugin_config->log_info.filename)); + } plugin_config->log_info.filename = strdup(optarg); break; diff --git a/plugins/experimental/uri_signing/config.cc b/plugins/experimental/uri_signing/config.cc index 36435b9db5f..27372790d56 100644 --- a/plugins/experimental/uri_signing/config.cc +++ b/plugins/experimental/uri_signing/config.cc @@ -282,6 +282,8 @@ read_config_from_json(json_t *const issuer_json) if (id_json) { id = json_string_value(id_json); if (id) { + /* An earlier issuer may have set an id; free it so it is not leaked. Last issuer wins. */ + free(cfg->id); cfg->id = static_cast(malloc(strlen(id) + 1)); strcpy(cfg->id, id); PluginDebug("Found Id in the config: %s", cfg->id); diff --git a/plugins/regex_revalidate/regex_revalidate.cc b/plugins/regex_revalidate/regex_revalidate.cc index 5e8d8cf2f7b..e3605b70005 100644 --- a/plugins/regex_revalidate/regex_revalidate.cc +++ b/plugins/regex_revalidate/regex_revalidate.cc @@ -778,6 +778,7 @@ TSPluginInit(int argc, const char *argv[]) while ((c = getopt_long(argc, (char *const *)argv, "c:l:f:m:", longopts, nullptr)) != -1) { switch (c) { case 'c': + TSfree(pstate->config_path); // An option can be repeated, so the earlier value is not leaked pstate->config_path = TSstrdup(optarg); break; case 'l': @@ -790,9 +791,11 @@ TSPluginInit(int argc, const char *argv[]) disable_timed_reload = true; break; case 'f': + TSfree(pstate->state_path); pstate->state_path = make_state_path(optarg); break; case 'm': + TSfree(pstate->match_header); pstate->match_header = TSstrdup(optarg); break; default: diff --git a/plugins/remap_purge/remap_purge.cc b/plugins/remap_purge/remap_purge.cc index fd0b8198fbe..eba0fce5ece 100644 --- a/plugins/remap_purge/remap_purge.cc +++ b/plugins/remap_purge/remap_purge.cc @@ -287,17 +287,21 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE purge->allow_get = true; break; case 'h': + TSfree(purge->header); // An option can be repeated, so the earlier value is not leaked purge->header = TSstrdup(optarg); purge->header_len = strlen(purge->header); break; case 'i': + TSfree(purge->id); purge->id = TSstrdup(optarg); break; case 's': + TSfree(purge->secret); purge->secret = TSstrdup(optarg); purge->secret_len = strlen(purge->secret); break; case 'f': + TSfree(purge->state_file); purge->state_file = make_state_path(optarg); break; } diff --git a/plugins/xdebug/xdebug.cc b/plugins/xdebug/xdebug.cc index 32cd8631372..d228b89e4da 100644 --- a/plugins/xdebug/xdebug.cc +++ b/plugins/xdebug/xdebug.cc @@ -947,6 +947,7 @@ TSPluginInit(int argc, const char *argv[]) switch (opt) { case 'h': Dbg(dbg_ctl, "Setting header: %s", optarg); + TSfree(const_cast(xDebugHeader.str)); // The option can be repeated, so the earlier value is not leaked xDebugHeader.str = TSstrdup(optarg); break; case 'e': diff --git a/src/api/InkAPITest.cc b/src/api/InkAPITest.cc index eac60ccd3fc..881f0d91ebb 100644 --- a/src/api/InkAPITest.cc +++ b/src/api/InkAPITest.cc @@ -6656,6 +6656,9 @@ REGRESSION_TEST(SDK_API_TSMgmtGet)(RegressionTest *test, int /* atype ATS_UNUSED SDK_RPRINT(test, "TSMgmtStringGet", "TestCase1.4", TC_PASS, "ok"); } + // TSMgmtStringGet() hands back a copy the caller owns. + TSfree(svalue); + { TSRecordDataType result; auto ret = TSMgmtDataTypeGet(CONFIG_PARAM_STRING_NAME, &result); diff --git a/src/proxy/http/remap/RemapYamlConfig.cc b/src/proxy/http/remap/RemapYamlConfig.cc index 7a441434463..ba905caa357 100644 --- a/src/proxy/http/remap/RemapYamlConfig.cc +++ b/src/proxy/http/remap/RemapYamlConfig.cc @@ -388,7 +388,9 @@ parse_map_referer(const YAML::Node &node, url_mapping *url_mapping) !strcasecmp(url.c_str(), "") || !strcasecmp(url.c_str(), "default_redirect_url")) { url_mapping->default_redirect_url = true; } - url_mapping->redir_chunk_list = redirect_tag_str::parse_format_redirect_url(ats_strdup(url.c_str())); + // parse_format_redirect_url() copies what it needs out of the buffer, so hand it the local + // string's storage rather than a fresh allocation that nothing would own. + url_mapping->redir_chunk_list = redirect_tag_str::parse_format_redirect_url(url.data()); if (!node["regex"] || !node["regex"].IsSequence()) { return swoc::Errata("'regex' field must be sequence"); diff --git a/src/traffic_cache_tool/CacheTool.cc b/src/traffic_cache_tool/CacheTool.cc index c28c2111c42..8da841c4d2b 100644 --- a/src/traffic_cache_tool/CacheTool.cc +++ b/src/traffic_cache_tool/CacheTool.cc @@ -208,7 +208,7 @@ struct Cache { std::map _volumes; std::vector globalVec_stripe; std::unordered_set URLset; - unsigned short *stripes_hash_table; + unsigned short *stripes_hash_table = nullptr; }; Errata @@ -685,7 +685,14 @@ Cache::calcTotalSpanPhysicalSize() } #endif -Cache::~Cache() {} +Cache::~Cache() +{ + // The URL set and the stripe hash table are owned solely by this instance. + for (auto *url : URLset) { + delete url; + } + ats_free(stripes_hash_table); +} Errata Span::load() From cfa139c7f5995cad2f649c321dcb0e1e7851aaff Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Fri, 14 Aug 2026 13:41:28 -0700 Subject: [PATCH 2/7] jax_fingerprint: own the plugin configuration with a unique_ptr Replaces the explicit delete on each initialization failure path with a unique_ptr that releases at the point ownership actually transfers: to the instance handle in TSRemapNewInstance, and to the log field callback and continuation in TSPluginInit. This also closes a leak in TSPluginInit. The user argument reservation failure path returned without freeing the configuration, which is only correct when the log field callback has captured it, and that capture is conditional on a log symbol being configured. Without one, nothing owned the configuration and it leaked. Reserving the index before registering the log field puts every failure exit inside the span where the unique_ptr still owns the object, so no path needs to reason about who else holds it. --- .../experimental/jax_fingerprint/plugin.cc | 55 ++++++++++--------- 1 file changed, 29 insertions(+), 26 deletions(-) diff --git a/plugins/experimental/jax_fingerprint/plugin.cc b/plugins/experimental/jax_fingerprint/plugin.cc index 03ee35d2844..0bd8c327948 100644 --- a/plugins/experimental/jax_fingerprint/plugin.cc +++ b/plugins/experimental/jax_fingerprint/plugin.cc @@ -373,25 +373,34 @@ TSPluginInit(int argc, char const **argv) return; } - PluginConfig *config = new PluginConfig(); - config->plugin_type = PluginType::GLOBAL; + auto owned_config = std::make_unique(); + owned_config->plugin_type = PluginType::GLOBAL; - if (!read_config_option(argc, argv, *config)) { + if (!read_config_option(argc, argv, *owned_config)) { TSError("[%s] Failed to parse options.", PLUGIN_NAME); - delete config; return; } - if (!config->log_filename.empty()) { - if (!create_log_file(config->log_filename, config->log_handle)) { + if (!owned_config->log_filename.empty()) { + if (!create_log_file(owned_config->log_filename, owned_config->log_handle)) { TSError("[%s] Failed to create log.", PLUGIN_NAME); - delete config; return; } else { Dbg(dbg_ctl, "Created log file."); } } + // Reserve the index before registering the log field, so that every failure exit happens while the + // configuration is still owned here and nothing has taken a reference to it yet. + if (reserve_user_arg(*owned_config) == TS_ERROR) { + TSError("[%s] Failed to reserve user arg index.", PLUGIN_NAME); + return; + } + + // A global plugin's configuration lives for the life of the process: the log field callback and the + // continuation below both keep a reference to it, so release it from the unique_ptr here. + PluginConfig *config = owned_config.release(); + if (!config->log_symbol.empty()) { std::string name = "jax_fingerprint-"; name += config->method.name; @@ -414,11 +423,6 @@ TSPluginInit(int argc, char const **argv) TSLogIntUnmarshal); } - if (reserve_user_arg(*config) == TS_ERROR) { - TSError("[%s] Failed to reserve user arg index.", PLUGIN_NAME); - return; - } - TSCont cont = TSContCreate(main_handler, nullptr); TSContDataSet(cont, config); if (config->method.on_client_hello) { @@ -447,19 +451,17 @@ TSReturnCode TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSED */, int /* errbuf_size ATS_UNUSED */) { Dbg(dbg_ctl, "New instance for client matching %s to %s", argv[0], argv[1]); - auto config = new PluginConfig(); + auto config = std::make_unique(); config->plugin_type = PluginType::REMAP; // Parse parameters if (!read_config_option(argc - 1, const_cast(argv + 1), *config)) { - delete config; Dbg(dbg_ctl, "Bad arguments"); return TS_ERROR; } if (!config->log_symbol.empty()) { TSError("[%s] --log-field is not supported in remap.config. Use it in plugin.config instead.", PLUGIN_NAME); - delete config; return TS_ERROR; } @@ -467,7 +469,6 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE if (!config->log_filename.empty()) { if (!create_log_file(config->log_filename, config->log_handle)) { TSError("[%s] Failed to create log.", PLUGIN_NAME); - delete config; return TS_ERROR; } else { Dbg(dbg_ctl, "Created log file."); @@ -476,26 +477,28 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE if (reserve_user_arg(*config) == TS_ERROR) { TSError("[%s] Failed to reserve user arg index.", PLUGIN_NAME); - delete config; return TS_ERROR; } + // Past here the instance handle owns the configuration and TSRemapDeleteInstance releases it. + PluginConfig *instance = config.release(); + // Create continuation - if (config->standalone) { + if (instance->standalone) { Dbg(dbg_ctl, "Standalone mode. Adding hooks."); - config->handler = TSContCreate(main_handler, nullptr); - if (config->method.on_client_hello) { - TSHttpHookAdd(TS_SSL_CLIENT_HELLO_HOOK, config->handler); + instance->handler = TSContCreate(main_handler, nullptr); + if (instance->method.on_client_hello) { + TSHttpHookAdd(TS_SSL_CLIENT_HELLO_HOOK, instance->handler); } - if (config->method.type == Method::Type::CONNECTION_BASED) { - TSHttpHookAdd(TS_VCONN_CLOSE_HOOK, config->handler); + if (instance->method.type == Method::Type::CONNECTION_BASED) { + TSHttpHookAdd(TS_VCONN_CLOSE_HOOK, instance->handler); } else { - TSHttpHookAdd(TS_HTTP_TXN_CLOSE_HOOK, config->handler); + TSHttpHookAdd(TS_HTTP_TXN_CLOSE_HOOK, instance->handler); } - TSContDataSet(config->handler, config); + TSContDataSet(instance->handler, instance); } - *ih = static_cast(config); + *ih = static_cast(instance); return TS_SUCCESS; } From bc46c50658b1502208e112af93edfa489b9f9fd8 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Fri, 14 Aug 2026 22:47:47 -0700 Subject: [PATCH 3/7] Give the redirect URL parser a buffer it may write to parse_format_redirect_url() nul terminates each chunk in place before copying it out and restores the byte afterwards. For a url containing no format specifier the scan runs to the end and that write lands on the terminating nul, and std::string does not permit a caller to assign through the reference at index size(). Pass a buffer this function owns instead, released once the parser returns. The chunk list holds its own copies, so nothing outlives the call, and the allocation that previously leaked here stays fixed. --- src/proxy/http/remap/RemapYamlConfig.cc | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/proxy/http/remap/RemapYamlConfig.cc b/src/proxy/http/remap/RemapYamlConfig.cc index ba905caa357..c515a6970d4 100644 --- a/src/proxy/http/remap/RemapYamlConfig.cc +++ b/src/proxy/http/remap/RemapYamlConfig.cc @@ -35,6 +35,7 @@ #include #include "tscore/Diags.h" +#include "tscore/ink_memory.h" #include "tscore/ink_string.h" #include "tsutil/ts_errata.h" #include "tsutil/PostScript.h" @@ -388,9 +389,12 @@ parse_map_referer(const YAML::Node &node, url_mapping *url_mapping) !strcasecmp(url.c_str(), "") || !strcasecmp(url.c_str(), "default_redirect_url")) { url_mapping->default_redirect_url = true; } - // parse_format_redirect_url() copies what it needs out of the buffer, so hand it the local - // string's storage rather than a fresh allocation that nothing would own. - url_mapping->redir_chunk_list = redirect_tag_str::parse_format_redirect_url(url.data()); + // parse_format_redirect_url() nul terminates each chunk in place before copying it out, and for a + // url with no format specifier that write lands on the terminating nul, which std::string does not + // allow a caller to assign. Give it a buffer we own instead, and release it once it returns; the + // chunk list holds copies. + ats_scoped_str redirect_url(ats_strdup(url.c_str())); + url_mapping->redir_chunk_list = redirect_tag_str::parse_format_redirect_url(redirect_url.get()); if (!node["regex"] || !node["regex"].IsSequence()) { return swoc::Errata("'regex' field must be sequence"); From 34323ccdd7461fa5989dd317d93a1f7bdd951bc8 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Tue, 18 Aug 2026 04:51:13 -0700 Subject: [PATCH 4/7] Address review: consistent naming, and drop two const_casts jax_fingerprint: TSPluginInit and TSRemapNewInstance were using the same name for different things, config being the unique_ptr in one and the raw pointer in the other. Both now spell the owning handle owned_config and the released raw pointer config. xdebug: the header name field is only ever assigned a TSstrdup result, never a literal, so storing it as char * removes the const_cast at the free and a second one at TSUserArgSet. stale_response: the log filename field defaulted to the static PLUGIN_TAG, which forced both a const_cast to free it and a pointer identity comparison to decide whether freeing was safe. It now defaults to null and the tag is substituted where the name is used, so freeing is unconditional and the comparison is gone. --- .../experimental/jax_fingerprint/plugin.cc | 34 +++++++++---------- .../stale_response/stale_response.cc | 9 +++-- .../stale_response/stale_response.h | 6 ++-- plugins/xdebug/xdebug.cc | 8 ++--- 4 files changed, 27 insertions(+), 30 deletions(-) diff --git a/plugins/experimental/jax_fingerprint/plugin.cc b/plugins/experimental/jax_fingerprint/plugin.cc index 0bd8c327948..1980c2fca11 100644 --- a/plugins/experimental/jax_fingerprint/plugin.cc +++ b/plugins/experimental/jax_fingerprint/plugin.cc @@ -451,23 +451,23 @@ TSReturnCode TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSED */, int /* errbuf_size ATS_UNUSED */) { Dbg(dbg_ctl, "New instance for client matching %s to %s", argv[0], argv[1]); - auto config = std::make_unique(); - config->plugin_type = PluginType::REMAP; + auto owned_config = std::make_unique(); + owned_config->plugin_type = PluginType::REMAP; // Parse parameters - if (!read_config_option(argc - 1, const_cast(argv + 1), *config)) { + if (!read_config_option(argc - 1, const_cast(argv + 1), *owned_config)) { Dbg(dbg_ctl, "Bad arguments"); return TS_ERROR; } - if (!config->log_symbol.empty()) { + if (!owned_config->log_symbol.empty()) { TSError("[%s] --log-field is not supported in remap.config. Use it in plugin.config instead.", PLUGIN_NAME); return TS_ERROR; } // Create a log file - if (!config->log_filename.empty()) { - if (!create_log_file(config->log_filename, config->log_handle)) { + if (!owned_config->log_filename.empty()) { + if (!create_log_file(owned_config->log_filename, owned_config->log_handle)) { TSError("[%s] Failed to create log.", PLUGIN_NAME); return TS_ERROR; } else { @@ -475,30 +475,30 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE } } - if (reserve_user_arg(*config) == TS_ERROR) { + if (reserve_user_arg(*owned_config) == TS_ERROR) { TSError("[%s] Failed to reserve user arg index.", PLUGIN_NAME); return TS_ERROR; } // Past here the instance handle owns the configuration and TSRemapDeleteInstance releases it. - PluginConfig *instance = config.release(); + PluginConfig *config = owned_config.release(); // Create continuation - if (instance->standalone) { + if (config->standalone) { Dbg(dbg_ctl, "Standalone mode. Adding hooks."); - instance->handler = TSContCreate(main_handler, nullptr); - if (instance->method.on_client_hello) { - TSHttpHookAdd(TS_SSL_CLIENT_HELLO_HOOK, instance->handler); + config->handler = TSContCreate(main_handler, nullptr); + if (config->method.on_client_hello) { + TSHttpHookAdd(TS_SSL_CLIENT_HELLO_HOOK, config->handler); } - if (instance->method.type == Method::Type::CONNECTION_BASED) { - TSHttpHookAdd(TS_VCONN_CLOSE_HOOK, instance->handler); + if (config->method.type == Method::Type::CONNECTION_BASED) { + TSHttpHookAdd(TS_VCONN_CLOSE_HOOK, config->handler); } else { - TSHttpHookAdd(TS_HTTP_TXN_CLOSE_HOOK, instance->handler); + TSHttpHookAdd(TS_HTTP_TXN_CLOSE_HOOK, config->handler); } - TSContDataSet(instance->handler, instance); + TSContDataSet(config->handler, config); } - *ih = static_cast(instance); + *ih = static_cast(config); return TS_SUCCESS; } diff --git a/plugins/experimental/stale_response/stale_response.cc b/plugins/experimental/stale_response/stale_response.cc index 519ab8b8a74..f687e7a6c5e 100644 --- a/plugins/experimental/stale_response/stale_response.cc +++ b/plugins/experimental/stale_response/stale_response.cc @@ -1071,9 +1071,7 @@ parse_args(int argc, char const *argv[]) break; case 'd': // The option may be repeated; release the previously duplicated name first. - if (plugin_config->log_info.filename != PLUGIN_TAG) { - free(const_cast(plugin_config->log_info.filename)); - } + free(plugin_config->log_info.filename); plugin_config->log_info.filename = strdup(optarg); break; @@ -1112,8 +1110,9 @@ parse_args(int argc, char const *argv[]) } if (plugin_config->log_info.all || plugin_config->log_info.stale_while_revalidate || plugin_config->log_info.stale_if_error) { - SRDBG(TAG, "[%s] Logging to %s", __FUNCTION__, plugin_config->log_info.filename); - TSTextLogObjectCreate(plugin_config->log_info.filename, TS_LOG_MODE_ADD_TIMESTAMP, &(plugin_config->log_info.object)); + char const *const log_filename = plugin_config->log_info.filename ? plugin_config->log_info.filename : PLUGIN_TAG; + SRDBG(TAG, "[%s] Logging to %s", __FUNCTION__, log_filename); + TSTextLogObjectCreate(log_filename, TS_LOG_MODE_ADD_TIMESTAMP, &(plugin_config->log_info.object)); } SRDBG(TAG, "[%s] global stale if error override = %" PRIdMAX, __FUNCTION__, diff --git a/plugins/experimental/stale_response/stale_response.h b/plugins/experimental/stale_response/stale_response.h index 8d9f9be8fff..bab94832620 100644 --- a/plugins/experimental/stale_response/stale_response.h +++ b/plugins/experimental/stale_response/stale_response.h @@ -49,7 +49,7 @@ struct LogInfo { bool all = false; bool stale_if_error = false; bool stale_while_revalidate = false; - char const *filename = PLUGIN_TAG; + char *filename = nullptr; }; struct ConfigInfo { @@ -65,9 +65,7 @@ struct ConfigInfo { if (this->body_data_mutex) { TSMutexDestroy(this->body_data_mutex); } - if (this->log_info.filename != PLUGIN_TAG) { - free(const_cast(this->log_info.filename)); - } + free(this->log_info.filename); } UintBodyMap *body_data = nullptr; TSMutex body_data_mutex; diff --git a/plugins/xdebug/xdebug.cc b/plugins/xdebug/xdebug.cc index d228b89e4da..c131d3317eb 100644 --- a/plugins/xdebug/xdebug.cc +++ b/plugins/xdebug/xdebug.cc @@ -55,8 +55,8 @@ namespace atscppapi::TxnAuxMgrData mgrData; static struct { - const char *str; - int len; + char *str; + int len; } xDebugHeader = {nullptr, 0}; enum { @@ -947,7 +947,7 @@ TSPluginInit(int argc, const char *argv[]) switch (opt) { case 'h': Dbg(dbg_ctl, "Setting header: %s", optarg); - TSfree(const_cast(xDebugHeader.str)); // The option can be repeated, so the earlier value is not leaked + TSfree(xDebugHeader.str); // The option can be repeated, so the earlier value is not leaked xDebugHeader.str = TSstrdup(optarg); break; case 'e': @@ -975,7 +975,7 @@ TSPluginInit(int argc, const char *argv[]) auto ret = TSUserArgIndexReserve(TS_USER_ARGS_GLB, "XDebugHeader", "XDebug header name", &idx); TSReleaseAssert(ret == TS_SUCCESS); TSReleaseAssert(idx >= 0); - TSUserArgSet(nullptr, idx, const_cast(xDebugHeader.str)); + TSUserArgSet(nullptr, idx, xDebugHeader.str); AuxDataMgr::init("xdebug"); From e860bb75595a4a1d5600af55363e67cd868c2fd6 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Tue, 18 Aug 2026 05:13:12 -0700 Subject: [PATCH 5/7] Address suppressed review notes on allocators and comment accuracy The redirect URL comment described a hazard the final code never has: it explained why passing std::string storage was wrong, which is not visible in this change at all. It now states what is true of the code as written, that the parser needs a mutable buffer, keeps no pointer into it, and that the duplicate previously leaked. stale_response was managing the log filename with strdup and free while every other allocation in the file uses the ATS wrappers. Switched to TSstrdup and TSfree so the pair matches its neighbours and cannot be mixed up with the wrong deallocator later. build_stripe_hash_table() replaced the stripe hash table without releasing the previous one. Harmless while each Cache builds it once, but the destructor now owns that pointer and the function is called from two places, so it releases any table already installed. --- plugins/experimental/stale_response/stale_response.cc | 4 ++-- plugins/experimental/stale_response/stale_response.h | 2 +- src/proxy/http/remap/RemapYamlConfig.cc | 7 +++---- src/traffic_cache_tool/CacheTool.cc | 2 ++ 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/plugins/experimental/stale_response/stale_response.cc b/plugins/experimental/stale_response/stale_response.cc index f687e7a6c5e..28c78d33fe5 100644 --- a/plugins/experimental/stale_response/stale_response.cc +++ b/plugins/experimental/stale_response/stale_response.cc @@ -1071,8 +1071,8 @@ parse_args(int argc, char const *argv[]) break; case 'd': // The option may be repeated; release the previously duplicated name first. - free(plugin_config->log_info.filename); - plugin_config->log_info.filename = strdup(optarg); + TSfree(plugin_config->log_info.filename); + plugin_config->log_info.filename = TSstrdup(optarg); break; case 'e': diff --git a/plugins/experimental/stale_response/stale_response.h b/plugins/experimental/stale_response/stale_response.h index bab94832620..c138cc7c99b 100644 --- a/plugins/experimental/stale_response/stale_response.h +++ b/plugins/experimental/stale_response/stale_response.h @@ -65,7 +65,7 @@ struct ConfigInfo { if (this->body_data_mutex) { TSMutexDestroy(this->body_data_mutex); } - free(this->log_info.filename); + TSfree(this->log_info.filename); } UintBodyMap *body_data = nullptr; TSMutex body_data_mutex; diff --git a/src/proxy/http/remap/RemapYamlConfig.cc b/src/proxy/http/remap/RemapYamlConfig.cc index c515a6970d4..d31fa23c4d7 100644 --- a/src/proxy/http/remap/RemapYamlConfig.cc +++ b/src/proxy/http/remap/RemapYamlConfig.cc @@ -389,10 +389,9 @@ parse_map_referer(const YAML::Node &node, url_mapping *url_mapping) !strcasecmp(url.c_str(), "") || !strcasecmp(url.c_str(), "default_redirect_url")) { url_mapping->default_redirect_url = true; } - // parse_format_redirect_url() nul terminates each chunk in place before copying it out, and for a - // url with no format specifier that write lands on the terminating nul, which std::string does not - // allow a caller to assign. Give it a buffer we own instead, and release it once it returns; the - // chunk list holds copies. + // parse_format_redirect_url() nul terminates each chunk in place before copying it out, so it + // needs a mutable buffer. It keeps no pointer into that buffer, only ats_strdup copies, so the + // duplicate can be released as soon as it returns. Previously nothing owned it and it leaked. ats_scoped_str redirect_url(ats_strdup(url.c_str())); url_mapping->redir_chunk_list = redirect_tag_str::parse_format_redirect_url(redirect_url.get()); diff --git a/src/traffic_cache_tool/CacheTool.cc b/src/traffic_cache_tool/CacheTool.cc index 8da841c4d2b..3f3ea0d1901 100644 --- a/src/traffic_cache_tool/CacheTool.cc +++ b/src/traffic_cache_tool/CacheTool.cc @@ -1014,6 +1014,8 @@ Cache::build_stripe_hash_table() for (int i = 0; i < num_stripes; i++) { printf("build_vol_hash_table index %d mapped to %d requested %d got %d\n", i, i, forvol[i], gotvol[i]); } + // The destructor owns this table, so release any table a previous call installed. + ats_free(stripes_hash_table); stripes_hash_table = ttable; ats_free(forvol); From 2b3c27c0ac29cb083636f4937dbd6250802ba480 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Tue, 18 Aug 2026 05:36:28 -0700 Subject: [PATCH 6/7] stale_response: hold the log filename override as a string A nullable owning char pointer put the burden on every caller to remember the PLUGIN_TAG fallback, and left the destructor responsible for a raw deallocation. Holding it as a std::string states the intent instead: empty means the default tag, and the effective name is computed where it is used. This also makes the defect that started this impossible rather than fixed. Repeating the option previously leaked the earlier duplicate; assigning to a string releases it, so there is no ownership left to get wrong. --- plugins/experimental/stale_response/stale_response.cc | 8 ++++---- plugins/experimental/stale_response/stale_response.h | 5 +++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/plugins/experimental/stale_response/stale_response.cc b/plugins/experimental/stale_response/stale_response.cc index 28c78d33fe5..9611cd671e2 100644 --- a/plugins/experimental/stale_response/stale_response.cc +++ b/plugins/experimental/stale_response/stale_response.cc @@ -1070,9 +1070,8 @@ parse_args(int argc, char const *argv[]) plugin_config->log_info.stale_if_error = true; break; case 'd': - // The option may be repeated; release the previously duplicated name first. - TSfree(plugin_config->log_info.filename); - plugin_config->log_info.filename = TSstrdup(optarg); + // Assigning replaces any name from an earlier occurrence of this option. + plugin_config->log_info.filename_override = optarg; break; case 'e': @@ -1110,7 +1109,8 @@ parse_args(int argc, char const *argv[]) } if (plugin_config->log_info.all || plugin_config->log_info.stale_while_revalidate || plugin_config->log_info.stale_if_error) { - char const *const log_filename = plugin_config->log_info.filename ? plugin_config->log_info.filename : PLUGIN_TAG; + char const *const log_filename = + plugin_config->log_info.filename_override.empty() ? PLUGIN_TAG : plugin_config->log_info.filename_override.c_str(); SRDBG(TAG, "[%s] Logging to %s", __FUNCTION__, log_filename); TSTextLogObjectCreate(log_filename, TS_LOG_MODE_ADD_TIMESTAMP, &(plugin_config->log_info.object)); } diff --git a/plugins/experimental/stale_response/stale_response.h b/plugins/experimental/stale_response/stale_response.h index c138cc7c99b..704ed5fcbde 100644 --- a/plugins/experimental/stale_response/stale_response.h +++ b/plugins/experimental/stale_response/stale_response.h @@ -31,6 +31,7 @@ #include "BodyData.h" #include +#include #include struct BodyData; @@ -49,7 +50,8 @@ struct LogInfo { bool all = false; bool stale_if_error = false; bool stale_while_revalidate = false; - char *filename = nullptr; + // Empty means log to PLUGIN_TAG; see the effective name computed in parse_args(). + std::string filename_override; }; struct ConfigInfo { @@ -65,7 +67,6 @@ struct ConfigInfo { if (this->body_data_mutex) { TSMutexDestroy(this->body_data_mutex); } - TSfree(this->log_info.filename); } UintBodyMap *body_data = nullptr; TSMutex body_data_mutex; From 639cea0a7972b9da3c900fdeb59a1a57c7436b76 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Tue, 18 Aug 2026 05:59:55 -0700 Subject: [PATCH 7/7] Express stripe hash table ownership in its type Holding the stripe hash table in ats_scoped_mem removes both manual frees: the destructor no longer releases it, and installing a new table releases the previous one as part of the assignment rather than relying on a caller to remember. Also replaces a malloc and strcpy pair for the issuer id with strdup, which pairs with the free already in config_delete(), and corrects the spelling of null-terminates in the redirect URL comment to match the rest of the tree. --- plugins/experimental/uri_signing/config.cc | 3 +-- src/proxy/http/remap/RemapYamlConfig.cc | 2 +- src/traffic_cache_tool/CacheTool.cc | 8 +++----- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/plugins/experimental/uri_signing/config.cc b/plugins/experimental/uri_signing/config.cc index 27372790d56..f489ff12832 100644 --- a/plugins/experimental/uri_signing/config.cc +++ b/plugins/experimental/uri_signing/config.cc @@ -284,8 +284,7 @@ read_config_from_json(json_t *const issuer_json) if (id) { /* An earlier issuer may have set an id; free it so it is not leaked. Last issuer wins. */ free(cfg->id); - cfg->id = static_cast(malloc(strlen(id) + 1)); - strcpy(cfg->id, id); + cfg->id = strdup(id); PluginDebug("Found Id in the config: %s", cfg->id); } } diff --git a/src/proxy/http/remap/RemapYamlConfig.cc b/src/proxy/http/remap/RemapYamlConfig.cc index d31fa23c4d7..1016c468c10 100644 --- a/src/proxy/http/remap/RemapYamlConfig.cc +++ b/src/proxy/http/remap/RemapYamlConfig.cc @@ -389,7 +389,7 @@ parse_map_referer(const YAML::Node &node, url_mapping *url_mapping) !strcasecmp(url.c_str(), "") || !strcasecmp(url.c_str(), "default_redirect_url")) { url_mapping->default_redirect_url = true; } - // parse_format_redirect_url() nul terminates each chunk in place before copying it out, so it + // parse_format_redirect_url() null-terminates each chunk in place before copying it out, so it // needs a mutable buffer. It keeps no pointer into that buffer, only ats_strdup copies, so the // duplicate can be released as soon as it returns. Previously nothing owned it and it leaked. ats_scoped_str redirect_url(ats_strdup(url.c_str())); diff --git a/src/traffic_cache_tool/CacheTool.cc b/src/traffic_cache_tool/CacheTool.cc index 3f3ea0d1901..2d1e9187108 100644 --- a/src/traffic_cache_tool/CacheTool.cc +++ b/src/traffic_cache_tool/CacheTool.cc @@ -208,7 +208,7 @@ struct Cache { std::map _volumes; std::vector globalVec_stripe; std::unordered_set URLset; - unsigned short *stripes_hash_table = nullptr; + ats_scoped_mem stripes_hash_table; }; Errata @@ -687,11 +687,10 @@ Cache::calcTotalSpanPhysicalSize() Cache::~Cache() { - // The URL set and the stripe hash table are owned solely by this instance. + // The URL set is owned solely by this instance; the stripe hash table owns itself. for (auto *url : URLset) { delete url; } - ats_free(stripes_hash_table); } Errata @@ -1014,8 +1013,7 @@ Cache::build_stripe_hash_table() for (int i = 0; i < num_stripes; i++) { printf("build_vol_hash_table index %d mapped to %d requested %d got %d\n", i, i, forvol[i], gotvol[i]); } - // The destructor owns this table, so release any table a previous call installed. - ats_free(stripes_hash_table); + // Assigning releases any table a previous call installed. stripes_hash_table = ttable; ats_free(forvol);