New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't assume LogAccess::m_client_req_unmapped_url_canon_str is null terminated. #11305
base: master
Are you sure you want to change the base?
Conversation
…erminated. (apache#936) 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm: this has worked well for Yahoo and has fixed a pretty bad crash.
Other functions around this function use |
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. |
Why would we want to leave the crash in 9.x? |
Is 9.1.x no longer supported (with maintenance releases)? |
In this code:
trafficserver/src/proxy/logging/LogAccess.cc
Line 1591 in ff100f4
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():
trafficserver/include/proxy/hdrs/URL.h
Line 468 in ff100f4
In this case, there is no apparent null termination:
trafficserver/src/proxy/hdrs/URL.cc
Line 631 in ff100f4
It looks like this is how the terminal null can be lost on m_ptr_printed_string:
trafficserver/src/proxy/hdrs/URL.cc
Line 360 in ff100f4
trafficserver/include/proxy/hdrs/HdrHeap.h
Line 255 in e0620eb