Potentially problematic functions LogAccess::unmarshal_int_to_netscape|date|time_str

[freak82]

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

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 the function 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 subtle. The LogUtils::timestamp_to_netscape|date|time_str functions are not thread safe. They have 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 unmarshal functions (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.
One last note: these functions have internal optimization to call localtime_r only if the given timestamp is different than the last one. I've encountered myself, in other code bases, that the calls to localtime_r can be show up in the profiler in some situations. However, I'm not sure how often the current optimization actually kicks in. Using static thread_local will preserve it per thread.