bryancall commented on code in PR #13552:
URL: https://github.com/apache/trafficserver/pull/13552#discussion_r3788637828
##########
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(), "<default_redirect_url>") ||
!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());
Review Comment:
You are right, and this is a regression from the change rather than
something pre-existing. Fixed in bc46c50.
The original code here passed `ats_strdup(url.c_str())`, so the temporary
nul landed inside a malloc allocation of `strlen + 1` and was fine; it just
leaked, which is what I was trying to fix. Swapping in `url.data()` removed the
leak and moved that write onto `std::string`, where the reference at index
`size()` may not be assigned through.
Worth noting the trigger is the common case, not an edge case: the inner
scan only stops early on `%r`, `%f`, `%t` or `%o`, so any redirect URL with no
format specifier runs to the terminating nul and takes that path.
It now hands the parser an `ats_scoped_str` it owns and releases once the
call returns. The chunk list keeps its own copies via `ats_strdup`, so nothing
outlives the buffer, and the leak stays fixed. Clean build with no new warnings
and 137/137 unit tests on Fedora, GCC 16.1.1.
--
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]
