<div dir="ltr">i recently upgraded Android's copy of tzcode to be closer to tzcode2013d. with Android's signed 32-bit time_t i'm seeing a change in behavior for out-of-range dates. previously given something like this, mktime would return an error, but now it hangs in the binary search in time2sub.<div>
<br clear="all"><div><div> struct tm t;</div><div> memset(&t, 0, sizeof(tm));</div><div> t.tm_year = 200;</div><div> t.tm_mon = 2;</div><div> t.tm_mday = 10;</div></div><div><br></div><div>changing the </div><div>
<br></div><div><div> if (t == lo) {</div><div> ++t;</div><div> if (t <= lo)</div><div> return WRONG;</div><div> ++lo;</div><div> } else if (t == hi) {</div>
<div> --t;</div><div> if (t >= hi)</div><div> return WRONG;</div><div> --hi;</div><div> }</div></div><div><br></div><div>back to</div><div><br>
</div><div><div> if (t == lo) {</div><div> if (t == TIME_T_MAX)</div><div> return WRONG;</div><div> ++t;</div><div> ++lo;</div><div> } else if (t == hi) {</div>
<div> if (t == TIME_T_MIN)</div><div> return WRONG;</div><div> --t;</div><div> --hi;</div><div> }</div></div><div><br></div><div>fixes things. looking at the git history in <a href="https://github.com/eggert/tz.git">https://github.com/eggert/tz.git</a> it looks like the old code, found in Android up to and including jb-mr2 (but no longer in AOSP ToT), was never in upstream tzcode. here's where the upstream tzcode's overflow checks were introduced:</div>
<div><br></div><div><div>commit 46fa7e98177cae664f673c6034c9ac74d0ccd0ec</div><div>Author: Arthur David Olson <olsona@elsie></div><div>Date: Tue Aug 9 09:55:06 2005 -0400</div><div><br></div><div> overflow work</div>
<div> </div><div> SCCS-file: localtime.c</div><div> SCCS-SID: 7.96</div><div><br></div><div>diff --git a/localtime.c b/localtime.c</div><div>index 1c3e9b0..d3edb5a 100644</div><div>--- a/localtime.c</div><div>+++ b/localtime.c</div>
<div>@@ -1616,9 +1616,13 @@ const int do_norm_secs;</div><div> if (dir != 0) {</div><div> if (t == lo) {</div><div> ++t;</div><div>+ if (t <= lo)</div>
<div>+ return WRONG;</div><div> ++lo;</div><div> } else if (t == hi) {</div><div> --t;</div><div>
+ if (t >= hi)</div>
<div>+ return WRONG;</div><div> --hi;</div><div> }</div><div> if (lo > hi)</div></div><div><br></div><div>
looking at the Android AOSP git history, it looks like we found and fixed this bug years ago but never talked to upstream about it:</div><div><br></div><div><div>commit 2093d350be21ff086f9e145404877941b9a42c5c</div><div>
Author: David 'Digit' Turner <<a href="mailto:digit@google.com">digit@google.com</a>></div>
<div>Date: Wed Sep 9 17:41:59 2009 -0700</div><div><br></div><div> Fix an infinite loop in time2sub.</div><div> </div><div> The problem is that time_t is signed, and the original code relied on the</div><div> fact that (X + c < X) in case of overflow for c >= 0. Unfortunately, this</div>
<div> condition is only guaranteed by the standard for unsigned arithmetic, and</div><div> the gcc 4.4.0 optimizer did completely remove the corresponding test from</div><div> the code. This resulted in a missing boundary check, and an infinite loop.</div>
<div> </div><div> The problem is solved by testing explicitely for TIME_T_MIN and TIME_T_MAX</div><div> in the loop that uses this.</div><div> </div><div> Also fix increment_overflow and long_increment_overflow which were buggy</div>
<div> for exactly the same reasons.</div><div> </div><div> Note: a similar fix is needed for system/core/libcutils</div><div><br></div><div>diff --git a/libc/tzcode/localtime.c b/libc/tzcode/localtime.c</div><div>
index 9c5f218..83c1011 100644</div><div>--- a/libc/tzcode/localtime.c</div><div>+++ b/libc/tzcode/localtime.c</div><div>@@ -75,6 +75,31 @@ static __inline__ void _tzUnlock(void)</div><div> pthread_mutex_unlock(&_tzMutex);</div>
<div> }</div><div> </div><div>+/* Complex computations to determine the min/max of time_t depending</div><div>+ * on TYPE_BIT / TYPE_SIGNED / TYPE_INTEGRAL.</div><div>+ * These macros cannot be used in pre-processor directives, so we</div>
<div>+ * let the C compiler do the work, which makes things a bit funky.</div><div>+ */</div><div>+static const time_t TIME_T_MAX =</div><div>+ TYPE_INTEGRAL(time_t) ?</div><div>+ ( TYPE_SIGNED(time_t) ?</div><div>
+ ~((time_t)1 << (TYPE_BIT(time_t)-1))</div><div>+ :</div><div>+ ~(time_t)0</div><div>+ )</div><div>+ : /* if time_t is a floating point number */</div><div>+ ( sizeof(time_t) > sizeof(float) ? (time_t)DBL_MAX : (time_t)FLT_MAX );</div>
<div>+</div><div>+static const time_t TIME_T_MIN =</div><div>+ TYPE_INTEGRAL(time_t) ?</div><div>+ ( TYPE_SIGNED(time_t) ?</div><div>+ ((time_t)1 << (TYPE_BIT(time_t)-1))</div><div>+ :</div>
<div>+ 0</div><div>+ )</div><div>+ :</div><div>+ ( sizeof(time_t) > sizeof(float) ? (time_t)DBL_MIN : (time_t)FLT_MIN );</div><div>+</div><div> #ifndef WILDABBR</div><div> /*</div><div> ** Someone might make incorrect use of a time zone abbreviation:</div>
<div>@@ -1683,11 +1708,16 @@ increment_overflow(number, delta)</div><div> int * number;</div><div> int delta;</div><div> {</div><div>- int number0;</div><div>+ unsigned number0 = (unsigned)*number;</div><div>+ unsigned number1 = (unsigned)(number0 + delta);</div>
<div>+</div><div>+ *number = (int)number1;</div><div> </div><div>- number0 = *number;</div><div>- *number += delta;</div><div>- return (*number < number0) != (delta < 0);</div><div>+ if (delta >= 0) {</div>
<div>+ return ((int)number1 < (int)number0);</div><div>+ } else {</div><div>+ return ((int)number1 > (int)number0);</div><div>+ }</div><div> }</div><div> </div><div> static int</div><div>@@ -1695,11 +1725,16 @@ long_increment_overflow(number, delta)</div>
<div> long * number;</div><div> int delta;</div><div> {</div><div>- long number0;</div><div>+ unsigned long number0 = (unsigned long)*number;</div><div>+ unsigned long number1 = (unsigned long)(number0 + delta);</div>
<div>+</div><div>+ *number = (long)number1;</div><div> </div><div>- number0 = *number;</div><div>- *number += delta;</div><div>- return (*number < number0) != (delta < 0);</div><div>+ if (delta >= 0) {</div>
<div>+ return ((long)number1 < (long)number0);</div><div>+ } else {</div><div>+ return ((long)number1 > (long)number0);</div><div>+ }</div><div> }</div><div> </div><div> static int</div><div>@@ -1868,14 +1903,14 @@ const int do_norm_secs;</div>
<div> } else dir = tmcomp(&mytm, &yourtm);</div><div> if (dir != 0) {</div><div> if (t == lo) {</div><div>- ++t;</div><div>- if (t <= lo)</div><div>+ if (t == TIME_T_MAX)</div>
<div> return WRONG;</div><div>+ ++t;</div><div> ++lo;</div><div> } else if (t == hi) {</div><div>- --t;</div><div>- if (t >= hi)</div>
<div>+ if (t == TIME_T_MIN)</div><div> return WRONG;</div><div>+ --t;</div><div> --hi;</div><div> }</div><div> if (lo > hi)</div>
</div><div><br></div><div>right now i'm going to work around this by building the tzcode part of the C library with -fno-strict-overflow (because i don't know how many other places are relying on this unspecified behavior), but i'll apply whatever patch goes into upstream as soon as it's available.</div>
<div><br></div><div> --elliott</div>
</div></div>