[tz] localtime.c patch
Paul Eggert
eggert at cs.ucla.edu
Wed Oct 8 05:11:16 UTC 2014
Thanks for reviewing the code; this found a real bug and prompted me to find
another. Plus, you found some other things worth sprucing up.
To my mind the most important fix in your patch is removing that stray
initialization in leapcorr; this fixes a bug in the implementation of the new
(2014g) functions time2posix_z and posix2time_z.
Christos Zoulas wrote:
> There is a bug fix there too; in one of the cases sp->ttis[1]
> is not initialized, and returns trash to the user.
Sorry, I don't see the bug there; can you explain? I assume you're talking
about either the code labeled "only standard time" or the code labeled "so,
we're off a little", and in both cases there is only one time type, so the rest
of the code should never access sp->ttis[1] and there should be no need to
initialize it.
Although C99-and-later guarantees that memset(..., 0, ...) sets the struct
ttinfo members (which are int, bool, and int_fast32_t) to zero, tzcode assumes
only C89 which as I understand it does not provide that guarantee, so to be safe
we should avoid memset here. It's easy enough to initialize the remaining
structure members by hand.
The change to use ssize_t won't fix bugs on modern POSIX platforms, since POSIX
nowadays requires int to be at least 32 bits, but the change is probably worth
doing for clarity anway, and also to port to ancient POSIX which allowed 16-bit
int. However, POSIX weirdly says that ssize_t might not be able to store values
less than -1, which means subtracting a value from ssize_t is a no-no if the
result might be less than -1, which means we need to redo a subtraction that
might compute such a result. Furthermore, the loop from 0 to nread would need
an index type other than 'int', but there it's clearer just to use memmove
(something that C89 does guarantee).
In trying out the code under 'valgrind' I found that the command "zdump ''"
tickled some other uninitialized-storage usages: the "so, we're off a little"
code doesn't initialize charcnt, goback, goahead, or defaulttype.
I hope the attached proposed patch fixes all these problems; please let us know
if it misses anything.
-------------- next part --------------
From 3bf56b926fcf655531ff47cff716e81df28a6800 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert at cs.ucla.edu>
Date: Tue, 7 Oct 2014 21:43:13 -0700
Subject: [PROPOSED PATCH] Fix some localtime.c problems noted by Christos
Zoulas
in: http://mm.icann.org/pipermail/tz/2014-October/021684.html
* localtime.c (tzload): Use ssize_t, not int. Redo comparison to
avoid the need for an ssize_t value less than -1, which POSIX does
not guarantee. Use memmove so that we needn't worry about an
ssize_t index.
(tzparse): Remove static always-zero var. Initialized fields
to zero by hand instead.
(zoneinit): Initialized more fields, to avoid undefined behavior
in tzalloc.
(leapcorr): Fix bug, a stray initialization of a local variable.
* Makefile: Add comment about ssize_t.
* NEWS: Document the above.
---
Makefile | 1 +
NEWS | 6 ++++++
localtime.c | 53 +++++++++++++++++++++++++----------------------------
3 files changed, 32 insertions(+), 28 deletions(-)
diff --git a/Makefile b/Makefile
index 5037336..178c850 100644
--- a/Makefile
+++ b/Makefile
@@ -129,6 +129,7 @@ LDLIBS=
# -DNO_RUN_TIME_WARNINGS_ABOUT_YEAR_2000_PROBLEMS_THANK_YOU=1
# if you do not want run time warnings about formats that may cause
# year 2000 grief
+# -Dssize_t=long on ancient hosts that lack ssize_t
# -DTHREAD_SAFE=1 to make localtime.c thread-safe, as POSIX requires;
# not needed by the main-program tz code, which is single-threaded.
# Append other compiler flags as needed, e.g., -pthread on GNU/Linux.
diff --git a/NEWS b/NEWS
index f4d7942..8b01e64 100644
--- a/NEWS
+++ b/NEWS
@@ -26,6 +26,12 @@ Unreleased, experimental changes
This change is a companion to the tzname change in 2014h, and is
designed to make timezone and altzone more compatible with tzname.
+ Some bugs associated with the new 2014g functions have been fixed.
+ This includes a bug that largely incapacitated the new functions
+ time2posix_z and posix2time_z. (Thanks to Christos Zoulas.)
+ It also includes some uses of uninitialized variables after tzalloc.
+ The new code uses the standard type 'ssize_t', which the Makefile
+ now gives porting advice about.
Release 2014h - 2014-09-25 18:59:03 -0700
diff --git a/localtime.c b/localtime.c
index 9ffdcda..d7022e2 100644
--- a/localtime.c
+++ b/localtime.c
@@ -200,6 +200,17 @@ int daylight = 0;
long altzone = 0;
#endif /* defined ALTZONE */
+/* Initialize *S to a value based on GMTOFF, ISDST, and ABBRIND. */
+static void
+init_ttinfo(struct ttinfo *s, int_fast32_t gmtoff, bool isdst, int abbrind)
+{
+ s->tt_gmtoff = gmtoff;
+ s->tt_isdst = isdst;
+ s->tt_abbrind = abbrind;
+ s->tt_ttisstd = false;
+ s->tt_ttisgmt = false;
+}
+
static int_fast32_t
detzcode(const char *const codep)
{
@@ -304,7 +315,7 @@ tzload(register const char *name, register struct state *const sp,
register int i;
register int fid;
register int stored;
- register int nread;
+ register ssize_t nread;
typedef union {
struct tzhead tzhead;
char buf[2 * sizeof(struct tzhead) +
@@ -389,8 +400,9 @@ tzload(register const char *name, register struct state *const sp,
&& (ttisstdcnt == typecnt || ttisstdcnt == 0)
&& (ttisgmtcnt == typecnt || ttisgmtcnt == 0)))
goto oops;
- if (nread - (p - up->buf)
- < (timecnt * stored /* ats */
+ if (nread
+ < (p - up->buf /* struct tzhead */
+ + timecnt * stored /* ats */
+ timecnt /* types */
+ typecnt * 6 /* ttinfos */
+ charcnt /* chars */
@@ -507,8 +519,7 @@ tzload(register const char *name, register struct state *const sp,
if (up->tzhead.tzh_version[0] == '\0')
break;
nread -= p - up->buf;
- for (i = 0; i < nread; ++i)
- up->buf[i] = p[i];
+ memmove(up->buf, p, nread);
/*
** If this is a signed narrow time_t system, we're done.
*/
@@ -932,7 +943,6 @@ tzparse(const char *name, register struct state *const sp,
int_fast32_t dstoffset;
register char * cp;
register bool load_ok;
- static struct ttinfo zttinfo;
stdname = name;
if (lastditch) {
@@ -1004,13 +1014,8 @@ tzparse(const char *name, register struct state *const sp,
/*
** Two transitions per year, from EPOCH_YEAR forward.
*/
- sp->ttis[0] = sp->ttis[1] = zttinfo;
- sp->ttis[0].tt_gmtoff = -dstoffset;
- sp->ttis[0].tt_isdst = true;
- sp->ttis[0].tt_abbrind = stdlen + 1;
- sp->ttis[1].tt_gmtoff = -stdoffset;
- sp->ttis[1].tt_isdst = false;
- sp->ttis[1].tt_abbrind = 0;
+ init_ttinfo(&sp->ttis[0], -dstoffset, true, stdlen + 1);
+ init_ttinfo(&sp->ttis[1], -stdoffset, false, 0);
sp->defaulttype = 0;
timecnt = 0;
janfirst = 0;
@@ -1129,13 +1134,8 @@ tzparse(const char *name, register struct state *const sp,
/*
** Finally, fill in ttis.
*/
- sp->ttis[0] = sp->ttis[1] = zttinfo;
- sp->ttis[0].tt_gmtoff = -stdoffset;
- sp->ttis[0].tt_isdst = false;
- sp->ttis[0].tt_abbrind = 0;
- sp->ttis[1].tt_gmtoff = -dstoffset;
- sp->ttis[1].tt_isdst = true;
- sp->ttis[1].tt_abbrind = stdlen + 1;
+ init_ttinfo(&sp->ttis[0], -stdoffset, false, 0);
+ init_ttinfo(&sp->ttis[1], -dstoffset, true, stdlen + 1);
sp->typecnt = 2;
sp->defaulttype = 0;
}
@@ -1143,10 +1143,7 @@ tzparse(const char *name, register struct state *const sp,
dstlen = 0;
sp->typecnt = 1; /* only standard time */
sp->timecnt = 0;
- sp->ttis[0] = zttinfo;
- sp->ttis[0].tt_gmtoff = -stdoffset;
- sp->ttis[0].tt_isdst = false;
- sp->ttis[0].tt_abbrind = 0;
+ init_ttinfo(&sp->ttis[0], -stdoffset, false, 0);
sp->defaulttype = 0;
}
sp->charcnt = stdlen + 1;
@@ -1183,10 +1180,11 @@ zoneinit(struct state *sp, char const *name)
sp->leapcnt = 0; /* so, we're off a little */
sp->timecnt = 0;
sp->typecnt = 0;
- sp->ttis[0].tt_isdst = 0;
- sp->ttis[0].tt_gmtoff = 0;
- sp->ttis[0].tt_abbrind = 0;
+ sp->charcnt = 0;
+ sp->goback = sp->goahead = false;
+ init_ttinfo(&sp->ttis[0], 0, false, 0);
strcpy(sp->chars, gmt);
+ sp->defaulttype = 0;
} else if (! (tzload(name, sp, true)
|| (name && name[0] != ':' && tzparse(name, sp, false))))
return NULL;
@@ -2121,7 +2119,6 @@ leapcorr(struct state const *sp, time_t t)
register struct lsinfo const * lp;
register int i;
- sp = lclptr;
i = sp->leapcnt;
while (--i >= 0) {
lp = &sp->lsis[i];
--
1.9.3
More information about the tz
mailing list