Don't assume LogAccess::m_client_req_unmapped_url_canon_str is null terminated.

[ywkaras]

In this code:

trafficserver/src/proxy/logging/LogAccess.cc

Line 1591 in ff100f4

char *unmapped_url = m_http_sm->t_state.unmapped_url.string_get_ref(&unmapped_url_len);

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

URL::string_get_ref(int *length, unsigned normalization_flags) const

In this case, there is no apparent null termination:

trafficserver/src/proxy/hdrs/URL.cc

Line 631 in ff100f4

return const_cast<char *>(url->m_ptr_printed_string);

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

m_ptr_printed_string = new_heap->localize({m_ptr_printed_string, m_len_printed_string}).data();

trafficserver/include/proxy/hdrs/HdrHeap.h

Line 255 in e0620eb

char *new_str = this->allocate_str(length);

[ywkaras]

This change has been running in Yahoo prod for about two weeks. It stopped memory corruption that was previously causing frequent crashes.

[bneradt]

Just to confirm: this has worked well for Yahoo and has fixed a pretty bad crash.

[maskit]

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?

[ywkaras]

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.

[maskit]

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.

[ywkaras]

Why would we want to leave the crash in 9.x?

[maskit]

I said 9.1.x.

[ywkaras]

Is 9.1.x no longer supported (with maintenance releases)?

[moonchen]

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.

[cmcfarlen]

Cherry-picked to v10.0.x