[tz] tzcode error handling (patch 2 of 4)
Steve Summit
scs at eskimo.com
Mon Dec 11 12:36:20 UTC 2017
This is Patch 2, which attempts to improve localtime's error
handling further. It will tell you if it couldn't open a
zoneinfo file (explicitly stating which one and why); it will
tell you if it had trouble parsing a zoneinfo file; it will
tell you if it had trouble parsing a Posix timezone string not
referencing a file. That's all well and good, but the drawbacks
are that this ends up requiring rather a lot of changes, and the
implementation is significantly imperfect in that it uses some
errno-style static global data which renders it non thread safe
in the error handling case.
As a user, patch 1's error messages (though arguably better than
nothing) would have me putting my fist through the screen ("Come
on, tell me *which* *file* you couldn't open!"). But Patch 2's
nonreentrancy is likely to be a fatal flaw in some people's eyes,
and rightly so.
For those looking at the code: the error handling variables
in question are tzlb_reason and tzlb_filename. The obvious
non-statically-global place to stash them, for better thread
safety, would be in an instance of union local_storage,
but those guys don't stick around long enough under all
circumstances. Addressing that will be the subject of
Patch 3, at the cost of yet more code rearrangement.
This patch is with respect to 2017c, plus Patch 1.
Steve Summit
scs at eskimo.com
-------------- next part --------------
--- a/localtime.c 2017-12-06 19:24:38.000000000 -0500
+++ b/localtime.c 2017-12-06 19:27:49.000000000 -0500
@@ -152,6 +152,8 @@
int_fast32_t r_time; /* transition time of rule */
};
+enum tzparseret { TZP_SUCCESS, TZP_PARSE_ERR, TZP_NONE };
+
static struct tm *gmtsub(struct state const *, time_t const *, int_fast32_t,
struct tm *);
static bool increment_overflow(int *, int);
@@ -160,7 +162,7 @@
static struct tm *timesub(time_t const *, int_fast32_t, struct state const *,
struct tm *);
static bool typesequiv(struct state const *, int, int);
-static bool tzparse(char const *, struct state *, bool);
+static enum tzparseret tzparse(char const *, struct state *, bool);
#ifdef ALL_STATE
static struct state * lclptr;
@@ -209,6 +211,10 @@
static void (*wrnfunc)(char const *, ...) = NULL;
+/* unfortunate statics; these will hinder reentrancy */
+static char const *tzlb_reason = NULL;
+static char tzlb_filename[FILENAME_MAX + 1];
+
/* 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)
@@ -382,7 +388,11 @@
/* Load tz data from the file named NAME into *SP. Read extended
format if DOEXTEND. Use *LSP for temporary storage. Return 0 on
- success, an errno value on failure. */
+ success, an errno value on failure. Also on failure,
+ tzlb_filename is the full pathname of the tzdatafile, and
+ tzlb_reason is a reason for the failure not necessarily
+ reflected by errno. (Beware: these error-reporting aspects
+ are not thread-safe.) */
static int
tzloadbody(char const *name, struct state *sp, bool doextend,
union local_storage *lsp)
@@ -397,10 +407,15 @@
sp->goback = sp->goahead = false;
+ tzlb_reason = NULL;
+ *tzlb_filename = '\0';
+
if (! name) {
name = TZDEFAULT;
- if (! name)
+ if (! name) {
+ tzlb_reason = "missing zone name";
return EINVAL;
+ }
}
if (name[0] == ':')
@@ -422,6 +437,8 @@
doaccess = true;
name = lsp->fullname;
}
+ *tzlb_filename = '\0'; /* use strncat as safe strcpy */
+ strncat(tzlb_filename, name, sizeof(tzlb_filename)-1);
if (doaccess && access(name, R_OK) != 0)
return errno;
fid = open(name, OPEN_MODE);
@@ -430,7 +447,13 @@
nread = read(fid, up->buf, sizeof up->buf);
if (nread < tzheadsize) {
- int err = nread < 0 ? errno : EINVAL;
+ int err;
+ if(nread < 0)
+ err = errno;
+ else {
+ err = EINVAL;
+ tzlb_reason = "truncated header";
+ }
close(fid);
return err;
}
@@ -451,8 +474,10 @@
&& 0 <= timecnt && timecnt < TZ_MAX_TIMES
&& 0 <= charcnt && charcnt < TZ_MAX_CHARS
&& (ttisstdcnt == typecnt || ttisstdcnt == 0)
- && (ttisgmtcnt == typecnt || ttisgmtcnt == 0)))
+ && (ttisgmtcnt == typecnt || ttisgmtcnt == 0))) {
+ tzlb_reason = "invalid counts";
return EINVAL;
+ }
if (nread
< (tzheadsize /* struct tzhead */
+ timecnt * stored /* ats */
@@ -461,8 +486,10 @@
+ charcnt /* chars */
+ leapcnt * (stored + 4) /* lsinfos */
+ ttisstdcnt /* ttisstds */
- + ttisgmtcnt)) /* ttisgmts */
+ + ttisgmtcnt)) { /* ttisgmts */
+ tzlb_reason = "insufficient data";
return EINVAL;
+ }
sp->leapcnt = leapcnt;
sp->timecnt = timecnt;
sp->typecnt = typecnt;
@@ -481,8 +508,10 @@
= ((TYPE_SIGNED(time_t) ? at < TIME_T_MIN : at < 0)
? TIME_T_MIN : at);
if (timecnt && attime <= sp->ats[timecnt - 1]) {
- if (attime < sp->ats[timecnt - 1])
+ if (attime < sp->ats[timecnt - 1]) {
+ tzlb_reason = "timecnt err";
return EINVAL;
+ }
sp->types[i - 1] = 0;
timecnt--;
}
@@ -494,8 +523,10 @@
timecnt = 0;
for (i = 0; i < sp->timecnt; ++i) {
unsigned char typ = *p++;
- if (sp->typecnt <= typ)
+ if (sp->typecnt <= typ) {
+ tzlb_reason = "timecnt err 2";
return EINVAL;
+ }
if (sp->types[i])
sp->types[timecnt++] = typ;
}
@@ -508,12 +539,16 @@
ttisp->tt_gmtoff = detzcode(p);
p += 4;
isdst = *p++;
- if (! (isdst < 2))
+ if (! (isdst < 2)) {
+ tzlb_reason = "invalid isdst val";
return EINVAL;
+ }
ttisp->tt_isdst = isdst;
abbrind = *p++;
- if (! (abbrind < sp->charcnt))
+ if (! (abbrind < sp->charcnt)) {
+ tzlb_reason = "invalid abbrind";
return EINVAL;
+ }
ttisp->tt_abbrind = abbrind;
}
for (i = 0; i < sp->charcnt; ++i)
@@ -536,8 +571,10 @@
correction must differ from the previous one's by 1
second. */
if (tr - prevtr < 28 * SECSPERDAY - 1
- || (corr != prevcorr - 1 && corr != prevcorr + 1))
+ || (corr != prevcorr - 1 && corr != prevcorr + 1)) {
+ tzlb_reason = "invalid leapsec data";
return EINVAL;
+ }
sp->lsis[leapcnt].ls_trans = prevtr = tr;
sp->lsis[leapcnt].ls_corr = prevcorr = corr;
leapcnt++;
@@ -552,8 +589,10 @@
if (ttisstdcnt == 0)
ttisp->tt_ttisstd = false;
else {
- if (*p != true && *p != false)
+ if (*p != true && *p != false) {
+ tzlb_reason = "invalid ttisstd val";
return EINVAL;
+ }
ttisp->tt_ttisstd = *p++;
}
}
@@ -564,8 +603,10 @@
if (ttisgmtcnt == 0)
ttisp->tt_ttisgmt = false;
else {
- if (*p != true && *p != false)
- return EINVAL;
+ if (*p != true && *p != false) {
+ tzlb_reason = "invalid ttisgmt val";
+ return EINVAL;
+ }
ttisp->tt_ttisgmt = *p++;
}
}
@@ -583,7 +624,7 @@
struct state *ts = &lsp->u.st;
up->buf[nread - 1] = '\0';
- if (tzparse(&up->buf[1], ts, false)
+ if (tzparse(&up->buf[1], ts, false) == TZP_SUCCESS
&& ts->typecnt == 2) {
/* Attempt to reuse existing abbreviations.
@@ -1021,10 +1062,12 @@
/*
** Given a POSIX section 8-style TZ string, fill in the rule tables as
-** appropriate.
+** appropriate. Return TZP_SUCCESS if successful, TZP_PARSE_ERR on
+** parse error, TZP_NONE if it doesn't even appear to be an attempt
+** at a POSIX-style string.
*/
-static bool
+static enum tzparseret
tzparse(const char *name, struct state *sp, bool lastditch)
{
const char * stdname;
@@ -1035,6 +1078,7 @@
int_fast32_t stdoffset;
int_fast32_t dstoffset;
register char * cp;
+ int err;
register bool load_ok;
stdname = name;
@@ -1048,7 +1092,7 @@
stdname = name;
name = getqzname(name, '>');
if (*name != '>')
- return false;
+ return TZP_PARSE_ERR;
stdlen = name - stdname;
name++;
} else {
@@ -1056,18 +1100,27 @@
stdlen = name - stdname;
}
if (!stdlen)
- return false;
+ return TZP_NONE;
name = getoffset(name, &stdoffset);
if (name == NULL)
- return false;
+ return TZP_NONE;
}
charcnt = stdlen + 1;
if (sizeof sp->chars < charcnt)
- return false;
- load_ok = tzload(TZDEFRULES, sp, false) == 0;
+ return TZP_PARSE_ERR;
+ err = tzload(TZDEFRULES, sp, false);
+ load_ok = (err == 0);
if (!load_ok) {
- if(wrnfunc) (*wrnfunc)("can't load default rules %s",
- TZDEFRULES);
+ if(wrnfunc) {
+ (*wrnfunc)("can't load default rules %s", TZDEFRULES);
+ if(*tzlb_filename) {
+ if(tzlb_reason)
+ (*wrnfunc)("%s: %s", tzlb_filename, tzlb_reason);
+ if(!tzlb_reason || err != EINVAL)
+ (*wrnfunc)("error opening/reading %s: %s",
+ tzlb_filename, strerror(err));
+ }
+ }
sp->leapcnt = 0; /* so, we're off a little */
}
if (*name != '\0') {
@@ -1075,7 +1128,7 @@
dstname = ++name;
name = getqzname(name, '>');
if (*name != '>')
- return false;
+ return TZP_PARSE_ERR;
dstlen = name - dstname;
name++;
} else {
@@ -1084,14 +1137,14 @@
dstlen = name - dstname; /* length of DST zone name */
}
if (!dstlen)
- return false;
+ return TZP_PARSE_ERR;
charcnt += dstlen + 1;
if (sizeof sp->chars < charcnt)
- return false;
+ return TZP_PARSE_ERR;
if (*name != '\0' && *name != ',' && *name != ';') {
name = getoffset(name, &dstoffset);
if (name == NULL)
- return false;
+ return TZP_PARSE_ERR;
} else dstoffset = stdoffset - SECSPERHOUR;
if (*name == '\0' && !load_ok)
name = TZDEFRULESTRING;
@@ -1107,13 +1160,13 @@
++name;
if ((name = getrule(name, &start)) == NULL)
- return false;
+ return TZP_PARSE_ERR;
if (*name++ != ',')
- return false;
+ return TZP_PARSE_ERR;
if ((name = getrule(name, &end)) == NULL)
- return false;
+ return TZP_PARSE_ERR;
if (*name != '\0')
- return false;
+ return TZP_PARSE_ERR;
sp->typecnt = 2; /* standard time and DST */
/*
** Two transitions per year, from EPOCH_YEAR forward.
@@ -1191,7 +1244,7 @@
register int j;
if (*name != '\0')
- return false;
+ return TZP_PARSE_ERR;
/*
** Initial values of theirstdoffset and theirdstoffset.
*/
@@ -1279,15 +1332,29 @@
memcpy(cp, dstname, dstlen);
*(cp + dstlen) = '\0';
}
- return true;
+ return TZP_SUCCESS;
}
static void
gmtload(struct state *const sp)
{
- if (tzload(gmt, sp, true) != 0) {
- if(!tzparse(gmt, sp, true)) {
- if(wrnfunc) (*wrnfunc)("can't load GMT rules %s", gmt);
+ int err;
+ enum tzparseret parse_err;
+ if ((err = tzload(gmt, sp, true)) != 0) {
+ if((parse_err = tzparse(gmt, sp, true)) != TZP_SUCCESS) {
+ if(wrnfunc) {
+ if(parse_err == TZP_PARSE_ERR)
+ (*wrnfunc)("error parsing zone string");
+ else if(*tzlb_filename) {
+ if(tzlb_reason)
+ (*wrnfunc)("%s: %s", tzlb_filename, tzlb_reason);
+ if(!tzlb_reason || err != EINVAL)
+ (*wrnfunc)("error opening/reading %s: %s",
+ tzlb_filename, strerror(err));
+ }
+
+ (*wrnfunc)("can't load GMT rules %s", gmt);
+ }
}
}
}
@@ -1312,12 +1379,27 @@
return 0;
} else {
int err = tzload(name, sp, true);
- if (err != 0 && name && name[0] != ':' && tzparse(name, sp, false))
+ int parse_err;
+ if (err != 0 && name && name[0] != ':' &&
+ (parse_err = tzparse(name, sp, false)) == TZP_SUCCESS)
err = 0;
if (err == 0)
scrub_abbrs(sp);
if(err) {
- if(wrnfunc) (*wrnfunc)("can't load zone %s (using GMT)", name);
+ if(wrnfunc) {
+ if(parse_err == TZP_PARSE_ERR)
+ (*wrnfunc)("error parsing zone string");
+ else if(*tzlb_filename) {
+ if(tzlb_reason)
+ (*wrnfunc)("%s: %s", tzlb_filename, tzlb_reason);
+ if(!tzlb_reason || err != EINVAL)
+ (*wrnfunc)("error opening/reading %s: %s",
+ tzlb_filename, strerror(err));
+ } else if(tzlb_reason) {
+ (*wrnfunc)("%s", tzlb_reason);
+ }
+ (*wrnfunc)("can't load zone %s (using GMT instead)", name);
+ }
}
return err;
}
More information about the tz
mailing list