On Sun, Feb 24, 2019 at 01:11:39PM +0100, Hiltjo Posthuma wrote: > Hi, > > I noticed some things in the strptime(3) implementing when parsing timezone > strings using the %z format string. > > 1. I noticed the tm_gmtoff value is not correctly set in some cases. It should > set tm_gmtoff to the offset from UTC in seconds (as described in mktime(3)). > > 2. The military/nautical UTC offsets are also reversed. This was also actually > a bug in RFC822: > > RFC5322 (referenced in strptime(3) man page): > https://tools.ietf.org/html/rfc5322 > Section 4.3, page 34 says: > " > The 1 character military time zones were defined in a non-standard > way in [RFC0822] and are therefore unpredictable in their meaning. > The original definitions of the military zones "A" through "I" are > equivalent to "+0100" through "+0900", respectively; "K", "L", and > "M" are equivalent to "+1000", "+1100", and "+1200", respectively; > "N" through "Y" are equivalent to "-0100" through "-1200". > respectively; and "Z" is equivalent to "+0000". However, because of > the error in [RFC0822], they SHOULD all be considered equivalent to > "-0000" unless there is out-of-band information confirming their > meaning. > " > > While comparing the other BSD and Linux code-bases I noticed NetBSD has fixed > this on 2017-08-24. It is not in NetBSD 8 stable yet, but in NetBSD-current: > http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libc/time/strptime.c > > 3. The military zone J is also ambiguous. Some standards define it as unused, > while others define it as "local observed time". NetBSD handles it as local > observed time, but OpenBSD returns NULL in strptime(3). I left this as is. > > 4. While at it I also fixed some trailing whitespaces. > > Thanks for not falling asleep and reading the long text :) > > Patch and test program below: > > diff --git lib/libc/time/strptime.c lib/libc/time/strptime.c > index eaf182dc773..86a0d5353ee 100644 > --- lib/libc/time/strptime.c > +++ lib/libc/time/strptime.c > @@ -116,7 +116,7 @@ _strptime(const char *buf, const char *fmt, struct tm > *tm, int initialize) > fmt++; > continue; > } > - > + > if ((c = *fmt++) != '%') > goto literal; > > @@ -142,7 +142,7 @@ literal: > _LEGAL_ALT(0); > alt_format |= _ALT_O; > goto again; > - > + > /* > * "Complex" conversion rules, implemented through recursion. > */ > @@ -380,7 +380,7 @@ literal: > * number but without the century. > */ > if (!(_conv_num(&bp, &i, 0, 99))) > - return (NULL); > + return (NULL); > continue; > > case 'G': /* The year corresponding to the ISO week > @@ -500,7 +500,7 @@ literal: > ep = _find_string(bp, &i, nast, NULL, 4); > if (ep != NULL) { > #ifdef TM_GMTOFF > - tm->TM_GMTOFF = -5 - i; > + tm->TM_GMTOFF = (-5 - i) * SECSPERHOUR; > #endif > #ifdef TM_ZONE > tm->TM_ZONE = (char *)nast[i]; > @@ -512,7 +512,7 @@ literal: > if (ep != NULL) { > tm->tm_isdst = 1; > #ifdef TM_GMTOFF > - tm->TM_GMTOFF = -4 - i; > + tm->TM_GMTOFF = (-4 - i) * SECSPERHOUR; > #endif > #ifdef TM_ZONE > tm->TM_ZONE = (char *)nadt[i]; > @@ -527,11 +527,12 @@ literal: > /* Argh! No 'J'! */ > if (*bp >= 'A' && *bp <= 'I') > tm->TM_GMTOFF = > - ('A' - 1) - (int)*bp; > + (int)*bp - ('A' - 1); > else if (*bp >= 'L' && *bp <= 'M') > - tm->TM_GMTOFF = 'A' - (int)*bp; > + tm->TM_GMTOFF = (int)*bp - 'A'; > else if (*bp >= 'N' && *bp <= 'Y') > - tm->TM_GMTOFF = (int)*bp - 'M'; > + tm->TM_GMTOFF = 'M' - (int)*bp; > + tm->TM_GMTOFF *= SECSPERHOUR; > #endif > #ifdef TM_ZONE > tm->TM_ZONE = NULL; /* XXX */ > @@ -556,14 +557,15 @@ literal: > } > switch (i) { > case 2: > - offs *= 100; > + offs *= SECSPERHOUR; > break; > case 4: > i = offs % 100; > - if (i >= 60) > + offs /= 100; > + if (i >= SECSPERMIN) > return NULL; > /* Convert minutes into decimal */ > - offs = (offs / 100) * 100 + (i * 50) / 30; > + offs = offs * SECSPERHOUR + i * SECSPERMIN; > break; > default: > return NULL; > @@ -719,7 +721,7 @@ _find_string(const u_char *bp, int *tgt, const char * > const *n1, > return NULL; > } > > -static int > +static int > leaps_thru_end_of(const int y) > { > return (y >= 0) ? (y / 4 - y / 100 + y / 400) : > > > Quick & dirty test program below: > > > #include <sys/types.h> > > #include <err.h> > #include <stdio.h> > #include <stdlib.h> > #include <string.h> > #include <time.h> > > void > testtime(const char *s) > { > struct tm tm; > > memset(&tm, 0, sizeof(tm)); > if (strptime(s, "%Y-%m-%d %H:%M:%S %z", &tm) == NULL) > errx(1, "strptime: s=%s", s); > > printf("s=%s, gmtoff=%ld, zone=%s\n", s, tm.tm_gmtoff, > tm.tm_zone ? tm.tm_zone : "(null)"); > } > > int > main(void) > { > testtime("2000-01-01 00:00:01 Z"); > testtime("2000-01-01 00:00:01 UT"); > testtime("2000-01-01 00:00:01 GMT"); > testtime("2000-01-01 00:00:01 -04:00"); > testtime("2000-01-01 00:00:01 -0400"); > testtime("2000-01-01 00:00:01 +04:00"); > testtime("2000-01-01 00:00:01 +0400"); > testtime("2000-01-01 00:00:01 EST"); > testtime("2000-01-01 00:00:01 EDT"); > testtime("2000-01-01 00:00:01 A"); > testtime("2000-01-01 00:00:01 B"); > testtime("2000-01-01 00:00:01 C"); > //testtime("2000-01-01 00:00:01 J"); // strptime returns NULL. > testtime("2000-01-01 00:00:01 Q"); > testtime("2000-01-01 00:00:01 R"); > > return 0; > } >
Last weekly bump, Any further comments or OK's to get this in or reject it? As discussed previously for point 2 I think it is fine to either return NULL and not handle military zones like most libc's do or handle them properly like NetBSD (reversed offsets). Thanks, -- Kind regards, Hiltjo
