From 2824b243a97bc5f6056b0107d250313259ce01e9 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Fri, 14 Aug 2026 11:21:40 -0700 Subject: [PATCH 1/3] 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/3] 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/3] 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");