freak82 opened a new issue, #11364: URL: https://github.com/apache/trafficserver/issues/11364
Hi there, I stumbled upon these functions and saw potential problems/bugs there. You'll correct me if I'm wrong. All of these functions are located in `src/proxy/logging/LogAccess.cc` and have the same pattern inside their body: For example `[ogAccess::unmarshal_int_to_date_str](https://github.com/apache/trafficserver/blob/master/src/proxy/logging/LogAccess.cc#L816) ``` int LogAccess::unmarshal_int_to_date_str(char **buf, char *dest, int len) { ink_assert(buf != nullptr); ink_assert(*buf != nullptr); ink_assert(dest != nullptr); int64_t value = unmarshal_int(buf); char *strval = LogUtils::timestamp_to_date_str(value); int strlen = static_cast<int>(::strlen(strval)); memcpy(dest, strval, strlen); return strlen; } ``` The most obvious problem is that the `len` of the `dest` is not taken into account and it just does `memcpy` with source `strlen`. Most likely the passed buffer is big enough but IMO this is fishy code which can lead to buffer overflow in some cases. I think it'll be better if this line: ``` int strlen = static_cast<int>(::strlen(strval)); ``` is replaced with such a line ``` int strlen = std::min<int>(::strlen(strval), len); ``` The second problem is more subtile. The `LogUtils::timestamp_to_netscape|date|time_str` functions are not thread safe. They have internally statically allocated buffer and another variable which they manipulate internally and return a pointer to this buffer. There is also a comment that these functions are not thread safe because they are supposed to be used only the logging thread. I checked the call-stack for these `unmarshal` functions and I'm not sure they are used by the logging thread currently. They are used under a lock but this lock is many levels up in the stack. Here is the callstack that I traced: ``` HttpBodyFactory::fabricate |- HttpBodyTemplate::build_instantiated_buffer |- resolve_logfield_string |- LogBuffer::resolve_custom_entry |- LogField::unmarshal |- m_unmarshal_func ``` and the `m_unmarshal_func` depending on the field type can be one of the above (they are set in `Log::init_fields()`). The `HttpBodyFactory::fabricate` seems to be always called under a lock so I assume it can be called by multiple threads but only one of them will be active at the time. So, assuming that I did not get confused in the callstack there is no possibility for race condition. However, I think it'll be a better and much more safer design if these low level functions `LogUtils::timestamp_to_netscape|date|time_str` behave like `LogUtils::timestamp_to_str` and get the destination buffer from outside. Or at lest they are changed to work with `static thread_local` buffer and return a pointer to it. -- 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]
