trafficserver
trafficserver copied to clipboard
apache
Don't assume LogAccess::m_client_req_unmapped_url_canon_str is null terminated.
In this code:
https://github.com/apache/trafficserver/blob/ff100f4f5ce69a00e3f3093f202d5f1aa9bb2ee5/src/proxy/logging/LogAccess.cc#L1591
if the unmapped URL has nothing to escape, m_client_req_unmapped_url_canon_str will retain the value returned by string_get_ref().
string_get_ref() is a wrapper for url_string_get_ref():
https://github.com/apache/trafficserver/blob/ff100f4f5ce69a00e3f3093f202d5f1aa9bb2ee5/include/proxy/hdrs/URL.h#L468
In this case, there is no apparent null termination:
https://github.com/apache/trafficserver/blob/ff100f4f5ce69a00e3f3093f202d5f1aa9bb2ee5/src/proxy/hdrs/URL.cc#L631
It looks like this is how the terminal null can be lost on m_ptr_printed_string:
https://github.com/apache/trafficserver/blob/ff100f4f5ce69a00e3f3093f202d5f1aa9bb2ee5/src/proxy/hdrs/URL.cc#L360 https://github.com/apache/trafficserver/blob/e0620eb941eab2603b2c230366e0fae5eeb6b57d/include/proxy/hdrs/HdrHeap.h#L255
This change has been running in Yahoo prod for about two weeks. It stopped memory corruption that was previously causing frequent crashes.
Other functions around this function use ink_strlcpy. Doing a different thing just for this case doesn't look like a right way. Can the other functions have similar issue and need similar changes? Did you consider ensuring the string is null terminated?
Other functions around this function use
ink_strlcpy. Doing a different thing just for this case doesn't look like a right way. Can the other functions have similar issue and need similar changes? Did you consider ensuring the string is null terminated?
This is a one-line, low-risk fix to a crash. If there is a better fix, it can be put in later. How does it make sense to put out release 10 with a bug, because it's possible there is a better fix for the bug? The clean fix is to consistently use null terminated strings, or strring_view. I don' t have time to do that currently, especially if we don't want to delay release 10.
If you think the clean fix is different from this change, why don't you leave a note? How could I know this is a compromise?
Also, I don't think we will have this on 9.1.x.
The code is quite conflicted about whether this string should be null-terminated or not. There are places where the length from url_string_get_ref is ignored, and others where the null terminator is ignored, like HDR_MOVE_STR. For now I think this PR is doing the safer thing.
