Fix a stack-buffer-overflow within approxidate.c

[dongahn]

I am creating a separate issue for a problem found in #694. AddressSanitizer produced the following report and this was confirmed to be a true positive.

=================================================================
==36257==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffffffcbf0 at pc 0x000000440715 bp 0x7fffffffca50 sp 0x7fffffffc210
WRITE of size 56 at 0x7fffffffcbf0 thread T0
    #0 0x440714 in unpoison_tm /collab/usr/global/tools/clang/chaos_5_x86_64_ib/clang-omp-3.5.0/src/llvm-3.5.0.src/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:573
    #1 0x440714 in __interceptor_mktime.part.68 /collab/usr/global/tools/clang/chaos_5_x86_64_ib/clang-omp-3.5.0/src/llvm-3.5.0.src/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:684
    #2 0x4ba1da in parse_date_basic /g/g0/dahn/workspace/Flux/flux-core/src/common/libutil/approxidate.c:581:22
    #3 0x4ba76c in approxidate /g/g0/dahn/workspace/Flux/flux-core/src/common/libutil/approxidate.c:919:7
    #4 0x4b75d5 in approxidate_tm /g/g0/dahn/workspace/Flux/flux-core/src/common/libutil/test/cronodate.c:15:9
    #5 0x4b75d5 in cronodate_check_match /g/g0/dahn/workspace/Flux/flux-core/src/common/libutil/test/cronodate.c:26
    #6 0x4b75d5 in main /g/g0/dahn/workspace/Flux/flux-core/src/common/libutil/test/cronodate.c:135
    #7 0x2aaaabeced1c in __libc_start_main /usr/src/debug/glibc-2.12-2-gc4ccff1/csu/libc-start.c:226
    #8 0x4b67bc in _start (/g/g0/dahn/workspace/Flux/flux-core/src/common/libutil/test_cronodate.t+0x4b67bc)

Address 0x7fffffffcbf0 is located in stack of thread T0 at offset 208 in frame
    #0 0x4b8a4f in parse_date_basic /g/g0/dahn/workspace/Flux/flux-core/src/common/libutil/approxidate.c:530

  This frame has 6 object(s):
    [32, 40) 'end.i147'
    [64, 72) 'end.i130'
    [96, 104) 'time.i'
    [128, 136) 'end.i'
    [160, 208) 'tm'
    [240, 244) 'dummy_offset' <== Memory access at offset 208 partially underflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow /collab/usr/global/tools/clang/chaos_5_x86_64_ib/clang-omp-3.5.0/src/llvm-3.5.0.src/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:573 unpoison_tm
Shadow bytes around the buggy address:
  0x10007fff7920: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10007fff7930: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10007fff7940: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10007fff7950: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10007fff7960: 00 00 00 00 f1 f1 f1 f1 00 f2 f2 f2 00 f2 f2 f2
=>0x10007fff7970: 00 f2 f2 f2 00 f2 f2 f2 00 00 00 00 00 00[f2]f2
  0x10007fff7980: f2 f2 04 f3 00 00 00 00 00 00 00 00 00 00 00 00
  0x10007fff7990: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10007fff79a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10007fff79b0: 00 00 00 00 f1 f1 f1 f1 00 f2 f2 f2 00 f2 f2 f2
  0x10007fff79c0: 00 f2 f2 f2 00 f2 f2 f2 00 f2 f2 f2 00 f2 f2 f2
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  ASan internal:           fe
==36257==ABORTING
FAIL: test_cronodate.t
ok 1 - wallclock_get_zulu() works: 2016-07-06T23:57:49.013747Z

From time.h:

struct tm
{
  int tm_sec;           /* Seconds. [0-60] (1 leap second) */
  int tm_min;           /* Minutes. [0-59] */
  int tm_hour;          /* Hours.   [0-23] */
  int tm_mday;          /* Day.     [1-31] */
  int tm_mon;           /* Month.   [0-11] */
  int tm_year;          /* Year - 1900.  */
  int tm_wday;          /* Day of week. [0-6] */
  int tm_yday;          /* Days in year.[0-365] */
  int tm_isdst;         /* DST.     [-1/0/1]*/

#ifdef  __USE_BSD
  long int tm_gmtoff;       /* Seconds east of UTC.  */
  __const char *tm_zone;    /* Timezone abbreviation.  */
#else
  long int __tm_gmtoff;     /* Seconds east of UTC.  */
  __const char *__tm_zone;  /* Timezone abbreviation.  */
#endif
};

So, struct tm may have more fields than what's explained in the man page:

       Broken-down time is stored in the structure tm which is defined in <time.h> as follows:

           struct tm {
               int tm_sec;         /* seconds */
               int tm_min;         /* minutes */
               int tm_hour;        /* hours */
               int tm_mday;        /* day of the month */
               int tm_mon;         /* month */
               int tm_year;        /* year */
               int tm_wday;        /* day of the week */
               int tm_yday;        /* day in the year */
               int tm_isdst;       /* daylight saving time */
           };

Given the augmented tm structure at here, mktemp actually appear to overflow the buffer. But it turned out when I fix it by adding these two hidden fields to the struct atm:

struct atm {
    int tm_sec;
    int tm_min;
    int tm_hour;
    int tm_mday;
    int tm_mon;
    int tm_year;
    int tm_wday;
    int tm_yday;
    int tm_isdst;
    long int __tm_gmtoff;
    __const char *__tm_zone;
    long tm_usec;
};

Interestingly enough, test_approxidate.t However, test_approxidate.t fails with a similar symptom reported in Issue #715.

not ok 34 - line 96: 1388620800 = 1388534400

#   Failed test 'line 96: 1388620800 = 1388534400
'
#   at test/approxidate.c line 96.
not ok 35 - line 99: 1388620800 = 1388534400

#   Failed test 'line 99: 1388620800 = 1388534400
'
#   at test/approxidate.c line 99.
not ok 36 - line 112: 1481328000 = 1481241600

#   Failed test 'line 112: 1481328000 = 1481241600
'
#   at test/approxidate.c line 112.
ok 37 - usec calculation for anonymous time is correct
1..37

[dongahn]

@grondo suggested:

Just as an experiment I wonder if this would quiet AddressSanitizer:

diff --git a/src/common/libutil/approxidate.c b/src/common/libutil/approxidate.c
index e2b2bd6..9d0e0d2 100644
--- a/src/common/libutil/approxidate.c
+++ b/src/common/libutil/approxidate.c
@@ -20,15 +20,7 @@
  * but adds a field for usec.
  */
 struct atm {
-       int tm_sec;
-       int tm_min;
-       int tm_hour;
-       int tm_mday;
-       int tm_mon;
-       int tm_year;
-       int tm_wday;
-       int tm_yday;
-       int tm_isdst;
+       struct tm;
        long tm_usec;
 };

With the version of GCC I'm using, for this to work requires CFLAGS=-fms-extensions, however anonymous structs in C do seem to be part of the ISO C11 standard, as stated in the GCC documentation

It would be interesting to know if this resolves the issue for ASan.

[grondo]

Thanks @dongahn, I was seeing failures in approxidate tests too (due to lazy use of timezone setting I think), and since we only use approxidate right now for cronodate tests, I'm going to submit a PR that adapts those tests and removes approxidate.