[PROPOSED 0/3] Leap second overflow bugs
This patch series fixes some bugs involving integer overflow in leap second calculations at runtime. The main problem is with the optional time2posix and posix2time functions; without the patches the functions can crash or loop indefinitely or return completely-wrong answers. With these patches the functions have new, documented behavior: they return (time_t) -1 and set errno to EOVERFLOW when the result is not representable, instead of behaving badly in tricky situations. Given the problems I've been seeing with the leap second runtime code, and given the worry that similar problems remain, and given that the leap second runtime code is rarely used in practice, I'm thinking it may be a good idea to add a compile-time option to disable localtime.c's support for leap seconds. Using the option would shrink the attack surface on tzcode. Paul Eggert (3): Fix theoretical -2**31 leapcorr bug Improve time2posix man page Fix overflow bugs in time2posix and posix2time NEWS | 13 ++++- localtime.c | 90 +++++++++++++++++++++------------- time2posix.3 | 134 ++++++++++++++++++++++++++++++++++----------------- 3 files changed, 156 insertions(+), 81 deletions(-) -- 2.51.0
Avoid undefined behavior if a contrived TZif file specifies a leap second correction equal to -2**31 on host where INT_FAST32_MIN == -2**31 + 1. ISO C before C23 allows such hosts, though I know of no practical examples. * NEWS: Mention this. * localtime.c (struct lsinfo.ls_corr, tzloadbody, timesub) (increment_overflow_time, leapcorr): Use int_fast32_2s instead of int_fast32_t when the value might equal -2**31. --- NEWS | 4 ++-- localtime.c | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/NEWS b/NEWS index d2aade51..539e5968 100644 --- a/NEWS +++ b/NEWS @@ -31,8 +31,8 @@ Unreleased, experimental changes (Undefined behavior reported by GitHub user Naveed8951.) Some other undefined behavior, triggered by TZif files containing - outlandish but conforming UT offsets, has also been fixed. - (Also reported by Naveed8951.) + outlandish but conforming UT offsets or leap second corrections, + has also been fixed. (Also reported by Naveed8951.) zic no longer generates a no-op transition when simultaneous Rule and Zone changes cancel each other out. diff --git a/localtime.c b/localtime.c index 9b7bbaa9..16eec790 100644 --- a/localtime.c +++ b/localtime.c @@ -480,7 +480,7 @@ struct ttinfo { /* time type information */ struct lsinfo { /* leap second information */ time_t ls_trans; /* transition time */ - int_fast32_t ls_corr; /* correction to apply */ + int_fast32_2s ls_corr; /* correction to apply */ }; /* This abbreviation means local time is unspecified. */ @@ -529,8 +529,8 @@ struct rule { static struct tm *gmtsub(struct state const *, time_t const *, int_fast32_t, struct tm *); static bool increment_overflow(int *, int); -static bool increment_overflow_time(time_t *, int_fast32_t); -static int_fast32_t leapcorr(struct state const *, time_t); +static bool increment_overflow_time(time_t *, int_fast32_2s); +static int_fast32_2s leapcorr(struct state const *, time_t); static struct tm *timesub(time_t const *, int_fast32_t, struct state const *, struct tm *); static bool tzparse(char const *, struct state *, struct state const *); @@ -1006,7 +1006,7 @@ tzloadbody(char const *name, struct state *sp, char tzloadflags, bool skip_datablock = stored == 4 && version; int_fast32_t datablock_size; int_fast64_t prevtr = -1; - int_fast32_t prevcorr; + int_fast32_2s prevcorr; int_fast32_2s ttisstdcnt = detzcode(up->tzhead.tzh_ttisstdcnt), ttisutcnt = detzcode(up->tzhead.tzh_ttisutcnt), @@ -1112,7 +1112,7 @@ tzloadbody(char const *name, struct state *sp, char tzloadflags, leapcnt = 0; for (i = 0; i < sp->leapcnt; ++i) { int_fast64_t tr = stored == 4 ? detzcode(p) : detzcode64(p); - int_fast32_t corr = detzcode(p + stored); + int_fast32_2s corr = detzcode(p + stored); p += stored + 4; /* Leap seconds cannot occur before the Epoch, @@ -2277,7 +2277,7 @@ timesub(const time_t *timep, int_fast32_t offset, register const struct lsinfo * lp; register time_t tdays; register const int * ip; - register int_fast32_t corr; + int_fast32_2s corr; register int i; int_fast32_t idays, rem, dayoff, dayrem; time_t y; @@ -2467,7 +2467,7 @@ increment_overflow_time_64(time_t *tp, int_fast64_t j) } static bool -increment_overflow_time(time_t *tp, int_fast32_t j) +increment_overflow_time(time_t *tp, int_fast32_2s j) { #ifdef ckd_add return ckd_add(tp, *tp, j); @@ -2945,7 +2945,7 @@ timegm(struct tm *tmp) } #endif -static int_fast32_t +static int_fast32_2s leapcorr(struct state const *sp, time_t t) { register struct lsinfo const * lp; -- 2.51.0
* time2posix.3: Improve formatting and wording. --- time2posix.3 | 113 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 68 insertions(+), 45 deletions(-) diff --git a/time2posix.3 b/time2posix.3 index 4a6969ed..adc6c65c 100644 --- a/time2posix.3 +++ b/time2posix.3 @@ -25,8 +25,9 @@ time2posix, posix2time \- convert seconds since the Epoch .. IEEE Standard 1003.1 (POSIX) -requires the time_t value 536457599 to stand for 1986-12-31 23:59:59 UTC. -This effectively implies that POSIX time_t values cannot include leap +says that +.B time_t +values cannot count leap seconds and, therefore, that the system time must be adjusted as each leap occurs. @@ -35,18 +36,22 @@ If the time package is configured with leap-second support enabled, however, no such adjustment is needed and -time_t values continue to increase over leap events +.B time_t +values continue to increase over leap events (as a true .q "seconds since...\&" value). This means that these values will differ from those required by POSIX by the net number of leap seconds inserted since the Epoch. .PP -Typically this is not a problem as the type time_t is intended -to be +For many C programs this is not a problem as the C standard says that +.B time_t +is (mostly) -opaque \*(en time_t values should only be obtained-from and -passed-to functions such as +opaque \*(en +.B time_t +values should be obtained from and +passed to functions such as .BR time(2) , .BR localtime(3) , .BR mktime(3) , @@ -54,11 +59,14 @@ and .BR difftime(3) . However, POSIX gives an arithmetic -expression for directly computing a time_t value from a given date/time, +expression for computing a +.B time_t +value directly from a given Universal date and time, and the same relationship is assumed by some -(usually older) applications. -Any programs creating/dissecting time_t values +Any programs creating/dissecting +.B time_t +values using such a relationship will typically not handle intervals over leap seconds correctly. .PP @@ -66,30 +74,31 @@ The .B time2posix and .B posix2time -functions are provided to address this time_t mismatch by converting -between local time_t values and their POSIX equivalents. +functions address this mismatch by converting +between local +.B time_t +values and their POSIX equivalents. This is done by accounting for the number of time-base changes that would have taken place on a POSIX system as leap seconds were inserted or deleted. -These converted values can then be used in lieu of correcting the older -applications, -or when communicating with POSIX-compliant systems. +These converted values can then be used +when communicating with POSIX-compliant systems. .PP The .B time2posix -function -is single-valued. -That is, -every local time_t -corresponds to a single POSIX time_t. +function converts a +.B time_t +value to its POSIX counterpart, if one exists. The .B posix2time -function +function does the reverse but is less well-behaved: for a positive leap second hit the result is not unique, and for a negative leap second hit the corresponding -POSIX time_t doesn't exist so an adjacent value is returned. -Both of these are good indicators of the inferiority of the +POSIX +.B time_t +doesn't exist so an adjacent value is returned. +Both of these are indicate problems with the POSIX representation. .PP The following table summarizes the relationship between a time @@ -97,35 +106,49 @@ T and its conversion to, and back from, the POSIX representation over the leap second inserted at the end of June, 1993. +In this table, X=time2posix(T), Y=posix2time(X), A=741484816, and B=A\-17 +because 17 positive leap seconds preceded this leap second. +.PP +.in +2 +.nf +.ta \w'1993-06-30\0'u +\w'23:59:59\0'u +\w'A+0\0'u +\w'B+0\0'u +DATE TIME T X Y +1993-06-30 23:59:59 A B A +1993-06-30 23:59:60 A+1 B+1 A+1 or A+2 +1993-07-01 00:00:00 A+2 B+1 A+1 or A+2 +1993-07-01 00:00:01 A+3 B+2 A+3 +.in +.fi +.PP +A leap second deletion would look like the following, and +posix2time(B+1) would return either A or A+1. +.PP +.in +2 .nf -.ta \w'93/06/30\0'u +\w'23:59:59\0'u +\w'A+0\0'u +\w'X=time2posix(T)\0'u -DATE TIME T X=time2posix(T) posix2time(X) -93/06/30 23:59:59 A+0 B+0 A+0 -93/06/30 23:59:60 A+1 B+1 A+1 or A+2 -93/07/01 00:00:00 A+2 B+1 A+1 or A+2 -93/07/01 00:00:01 A+3 B+2 A+3 - -A leap second deletion would look like... - -DATE TIME T X=time2posix(T) posix2time(X) -??/06/30 23:59:58 A+0 B+0 A+0 -??/07/01 00:00:00 A+1 B+2 A+1 -??/07/01 00:00:01 A+2 B+3 A+2 -.sp -.ce - [Note: posix2time(B+1) => A+0 or A+1] +DATE TIME T X Y +????-06-30 23:59:58 A B A +????-07-01 00:00:00 A+1 B+2 A+1 +????-07-01 00:00:01 A+2 B+3 A+2 .fi +.in .PP If leap-second support is not enabled, -local time_t and -POSIX time_t values are equivalent, +local +.B time_t +and +POSIX +.B time_t +values are equivalent, and both .B time2posix and .B posix2time degenerate to the identity function. +.SH "RETURN VALUE" +These functions return the resulting timestamp without modifying +.BR errno . .SH SEE ALSO -difftime(3), -localtime(3), -mktime(3), -time(2) +.BR difftime (3), +.BR localtime (3), +.BR mktime (3), +.BR time (2). -- 2.51.0
* NEWS, time2posix.3: Mention this. * localtime.c (decrement_overflow_time): New function. (time2posix_z): Use it. (time2posix_z, posix2time_z): If the result cannot be represented, set errno=EOVERFLOW and return -1 instead of having undefined behavior which in practice could involve inflooping and crashing. --- NEWS | 9 +++++++ localtime.c | 74 ++++++++++++++++++++++++++++++++++------------------ time2posix.3 | 23 +++++++++++++++- 3 files changed, 79 insertions(+), 27 deletions(-) diff --git a/NEWS b/NEWS index 539e5968..71bd91e4 100644 --- a/NEWS +++ b/NEWS @@ -4,6 +4,7 @@ Unreleased, experimental changes Briefly: The "right" TZif files are no longer installed by default. + Several integer overflow bugs have been fixed. Changes to build procedure @@ -30,6 +31,14 @@ Unreleased, experimental changes obsolete in release 2019b, and fixes some undefined behavior. (Undefined behavior reported by GitHub user Naveed8951.) + The posix2time, posix2time_z, time2posix, and time2posix_z + functions now set errno=EOVERFLOW and return ((time_t) -1) if the + result is not representable. Formerly they had undefined behavior + that could in practice result in crashing, looping indefinitely, + or returning an incorrect result. As before, these functions are + defined only when localtime.c is compiled with the -DSTD_INSPIRED + option. + Some other undefined behavior, triggered by TZif files containing outlandish but conforming UT offsets or leap second corrections, has also been fixed. (Also reported by Naveed8951.) diff --git a/localtime.c b/localtime.c index 16eec790..aa9b48ca 100644 --- a/localtime.c +++ b/localtime.c @@ -479,7 +479,7 @@ struct ttinfo { /* time type information */ }; struct lsinfo { /* leap second information */ - time_t ls_trans; /* transition time */ + time_t ls_trans; /* transition time (positive) */ int_fast32_2s ls_corr; /* correction to apply */ }; @@ -2967,6 +2967,21 @@ leapcorr(struct state const *sp, time_t t) #if !USE_TIMEX_T # if STD_INSPIRED +static bool +decrement_overflow_time(time_t *tp, int_fast32_2s j) +{ +#ifdef ckd_sub + return ckd_sub(tp, *tp, j); +#else + if (! (j < 0 + ? *tp <= TIME_T_MAX + j + : (TYPE_SIGNED(time_t) ? TIME_T_MIN + j <= *tp : j <= *tp))) + return true; + *tp -= j; + return false; +#endif +} + /* NETBSD_INSPIRED_EXTERN functions are exported to callers if NETBSD_INSPIRED is defined, and are private otherwise. */ # if NETBSD_INSPIRED @@ -2986,7 +3001,13 @@ leapcorr(struct state const *sp, time_t t) NETBSD_INSPIRED_EXTERN time_t time2posix_z(struct state *sp, time_t t) { - return t - leapcorr(sp, t); + if (decrement_overflow_time(&t, leapcorr(sp, t))) { + /* Overflow near maximum time_t value with negative correction. + This can happen with unrealistic-but-valid TZif files. */ + errno = EOVERFLOW; + return -1; + } + return t; } time_t @@ -3009,30 +3030,31 @@ time2posix(time_t t) NETBSD_INSPIRED_EXTERN time_t posix2time_z(struct state *sp, time_t t) { - time_t x; - time_t y; - /* - ** For a positive leap second hit, the result - ** is not unique. For a negative leap second - ** hit, the corresponding time doesn't exist, - ** so we return an adjacent second. - */ - x = t + leapcorr(sp, t); - y = x - leapcorr(sp, x); - if (y < t) { - do { - x++; - y = x - leapcorr(sp, x); - } while (y < t); - x -= y != t; - } else if (y > t) { - do { - --x; - y = x - leapcorr(sp, x); - } while (y > t); - x += y != t; - } - return x; + int i; + for (i = sp->leapcnt; 0 <= --i; ) { + struct lsinfo *lp = &sp->lsis[i]; + int_fast32_2s corr = lp->ls_corr; + time_t t_corr = t; + + if (increment_overflow_time(&t_corr, corr)) { + if (0 <= corr) { + /* Overflow near maximum time_t value with positive correction. + This can happen with ordinary TZif files with leap seconds. */ + errno = EOVERFLOW; + return -1; + } else { + /* A negative correction overflowed, so keep going. + This can happen with unrealistic-but-valid TZif files. */ + } + } else { + time_t trans = lp->ls_trans; + if (trans <= t_corr) + return (t_corr + - (trans == t_corr + && (i == 0 ? 0 : sp->lsis[i - 1].ls_corr) < corr)); + } + } + return t; } time_t diff --git a/time2posix.3 b/time2posix.3 index adc6c65c..adbff83a 100644 --- a/time2posix.3 +++ b/time2posix.3 @@ -145,8 +145,29 @@ and .B posix2time degenerate to the identity function. .SH "RETURN VALUE" -These functions return the resulting timestamp without modifying +If successful, these functions return the resulting timestamp without modifying .BR errno . +Otherwise, they set +.B errno +and return +.BR "((time_t) -1)" . +.SH ERRORS +These functions fail if: +.TP +[EOVERFLOW] +The resulting value cannot be represented. +This can happen for +.B posix2time +if its argument is close to the maximum +.B time_t +value. +In environments where the +.I TZ +environment variable names a TZif file, +overflow can happen for either function for an argument sufficiently +close to an extreme +.B time_t +value if the TZif file specifies unrealistic leap second corrections. .SH SEE ALSO .BR difftime (3), .BR localtime (3), -- 2.51.0
participants (1)
-
Paul Eggert