[Bug 54363] New: make time conversion caching functions thread-safe in util_time.c and mod_log_config.c

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=54363

            Bug ID: 54363
           Summary: make time conversion caching functions thread-safe in
                    util_time.c and mod_log_config.c
           Product: Apache httpd-2
           Version: 2.4.3
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P2
         Component: Core
          Assignee: bugs@httpd.apache.org
          Reporter: daniel.lescohier@cnet.com
    Classification: Unclassified

Created attachment 29803
  --> https://issues.apache.org/bugzilla/attachment.cgi?id=29803&action=edit
server/util_time.c patch

Though the time conversion caching functions in server/util_time.c and
modules/loggers/mod_log_config.c attempt to be thread-safe, the current
implementations are not thread-safe for the following reasons:

 1] Access to the cache_element is not marked volatile, so the compiler may
reorder memory access, making the algorithm invalid.  E.g., because it's not
marked volatile, the compiler might schedule to load t_validate into a register
before it copies the whole cache_element.

 2] Memory barriers are not used, so the CPUs are not guaranteeing total memory
ordering.  This can be solved by using Compare-And-Swap operations as a memory
barrier.

 3] The algorithm is subject to the "ABA problem."  This can be solved by only
updating the cache with newer time values, so that one never goes from B->A.
This fix also will improve the cache hit ratio, because an outlying old time
value will not update the cache element.

I am attaching patches based off of the 2.4.3 source distribution. I will also
attach the new versions of the .c files, so it's readable from this bug. The
comments in the code explain the new algorithm.

Other changes included in the patches:

 * util_time.c:cached_explode(): the common "is cached" path avoids an extra
memory copy of the exploded time structure.

 * Instead of passing a use_gmt flag, pass a function pointer to the explode
function, which avoids a branch in the function.

 * The exploded_time_cache_element struct size drops from 60 bytes to 48 bytes,
meaning the cache array is 192 bytes smaller, to a total cache array size of
768 bytes. Smaller means it's more likely to stay in the processor caches.

 * mod_log_config.c: Added a %g handler that is the same as %t except it's GMT
instead of localtime. This only added a few lines of code, using the same
technique mentioned above of passing the pointer to the explode function. This
is a useful addition: at my company we've been using %g for years by having the
logitem handler in our own custom module and registering the optional function
with mod_log_config.

 * mod_log_config.c:log_request_time_custom(): It's possible for apr_strftime
to return an empty string, but empty string return doesn't follow the logitem
API, one is supposed to return NULL instead, which the caller replaces with
"-". So, instead, check the retcode from apr_strtime, and return NULL on a
zero-length. Also, remove the double-copy of the string, and don't
unnecessarily expand the stack by 8kB.

I don't have the changes tested, I don't have a test environment setup, so the
only thing I've done is compiled these two objects and read the assembly to see
if it looks correct. But I thought I should open the bug as soon as possible.

-- 
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@httpd.apache.org
For additional commands, e-mail: bugs-help@httpd.apache.org

[Bug 54363] make time conversion caching functions thread-safe in util_time.c and mod_log_config.c

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=54363

--- Comment #3 from Daniel Lescohier <da...@cnet.com> ---
Created attachment 29806
  --> https://issues.apache.org/bugzilla/attachment.cgi?id=29806&action=edit
modules/loggers/mod_log_config.c (patched)

-- 
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@httpd.apache.org
For additional commands, e-mail: bugs-help@httpd.apache.org

[Bug 54363] make time conversion caching functions thread-safe in util_time.c and mod_log_config.c

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=54363

--- Comment #2 from Daniel Lescohier <da...@cnet.com> ---
Created attachment 29805
  --> https://issues.apache.org/bugzilla/attachment.cgi?id=29805&action=edit
server/util_time.c (patched)

-- 
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@httpd.apache.org
For additional commands, e-mail: bugs-help@httpd.apache.org

[Bug 54363] make time conversion caching functions thread-safe in util_time.c and mod_log_config.c

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=54363

--- Comment #1 from Daniel Lescohier <da...@cnet.com> ---
Created attachment 29804
  --> https://issues.apache.org/bugzilla/attachment.cgi?id=29804&action=edit
modules/loggers/mod_log_config.c patch

-- 
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@httpd.apache.org
For additional commands, e-mail: bugs-help@httpd.apache.org