Bug 54363 - make time conversion caching functions thread-safe in util_time.c and mod_log_config.c
Summary: make time conversion caching functions thread-safe in util_time.c and mod_log...
Status: NEW
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: Core (show other bugs)
Version: 2.4.3
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
Depends on:
Reported: 2012-12-31 16:12 UTC by Daniel Lescohier
Modified: 2012-12-31 16:15 UTC (History)
0 users

server/util_time.c patch (8.61 KB, patch)
2012-12-31 16:12 UTC, Daniel Lescohier
Details | Diff
modules/loggers/mod_log_config.c patch (10.23 KB, patch)
2012-12-31 16:12 UTC, Daniel Lescohier
Details | Diff
server/util_time.c (patched) (10.54 KB, text/x-csrc)
2012-12-31 16:14 UTC, Daniel Lescohier
modules/loggers/mod_log_config.c (patched) (58.78 KB, text/x-csrc)
2012-12-31 16:15 UTC, Daniel Lescohier

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Lescohier 2012-12-31 16:12:00 UTC
Created attachment 29803 [details]
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.
Comment 1 Daniel Lescohier 2012-12-31 16:12:49 UTC
Created attachment 29804 [details]
modules/loggers/mod_log_config.c patch
Comment 2 Daniel Lescohier 2012-12-31 16:14:15 UTC
Created attachment 29805 [details]
server/util_time.c (patched)
Comment 3 Daniel Lescohier 2012-12-31 16:15:07 UTC
Created attachment 29806 [details]
modules/loggers/mod_log_config.c (patched)