| Message ID | CAPC3xao-5YF_icBWE5yYbaYiUmiAvmb7w9s_G-dqawsx7eoTkQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Paul Pluzhnikov wrote: > + /* Do range checks first. BZ #18985 */ > + if (tp->tm_sec < 0 || tp->tm_sec > 60 > + || tp->tm_min < 0 || tp->tm_min > 59 > + || tp->tm_hour < 0 || tp->tm_hour > 23 > + || tp->tm_mday < 0 || tp->tm_mday > 31 > + || tp->tm_mon < 0 || tp->tm_mon > 11 > + || tp->tm_wday < 0 || tp->tm_wday > 6 > + || tp->tm_yday < 0 || tp->tm_yday > 365 > + || tp->tm_isdst < 0 || tp->tm_isdst > 1) > + { > + __set_errno (EINVAL); > + return 0; > + } That doesn't look right. If the values are out of range, C11 and POSIX say strftime is supposed to store an unspecified string, not return 0. tzcode strftime stores "?" for out-of-range tm_mon and tm_wday, and looks only at the sign of tm_isdst; this conforms to the standard. FreeBSD strftime is similar. glibc could do the same; that should be both easy and compatible.
On Sat, Sep 19, 2015 at 11:38 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:
> tzcode strftime stores "?" for out-of-range tm_mon and tm_wday
... but formats the other values, e.g. it can print something like
memset (tp, 0, sizeof (*tp));
tp->tm_hour = 1024;
strftime(..., "%H %I", tp); // produces "1024 04"
that doesn't seem very desirable.
Anyway, I'll send a patch to eliminate just the crashes on OOB tm_mon
and tm_wday.
On 09/20/2015 12:38 AM, Paul Eggert wrote: > Paul Pluzhnikov wrote: >> + /* Do range checks first. BZ #18985 */ >> + if (tp->tm_sec < 0 || tp->tm_sec > 60 >> + || tp->tm_min < 0 || tp->tm_min > 59 >> + || tp->tm_hour < 0 || tp->tm_hour > 23 >> + || tp->tm_mday < 0 || tp->tm_mday > 31 >> + || tp->tm_mon < 0 || tp->tm_mon > 11 >> + || tp->tm_wday < 0 || tp->tm_wday > 6 >> + || tp->tm_yday < 0 || tp->tm_yday > 365 >> + || tp->tm_isdst < 0 || tp->tm_isdst > 1) >> + { >> + __set_errno (EINVAL); >> + return 0; >> + } > > That doesn't look right. If the values are out of range, C11 and POSIX > say strftime is supposed to store an unspecified string, not return 0. > > tzcode strftime stores "?" for out-of-range tm_mon and tm_wday, and > looks only at the sign of tm_isdst; this conforms to the standard. > FreeBSD strftime is similar. glibc could do the same; that should be > both easy and compatible. I tend to agree, although while I suspect it wasn't the intent, an argument could be made that the patch is strictly conforming on the basis that storing the unspecified characters would produce a string that's longer than maxsize characters, in which case the function is supposed to return 0. Martin
Paul Pluzhnikov wrote: > ... but formats the other values, e.g. it can print something like > > memset (tp, 0, sizeof (*tp)); > tp->tm_hour = 1024; > > strftime(..., "%H %I", tp); // produces "1024 04" > > that doesn't seem very desirable. That helps the programmer more than returning 0 would. I occasionally need to format hours outside the 00-23 range, e.g., the "26" in the POSIX-style TZ string "IST-2IDT,M3.4.4/26,M10.5.0" for Israel's current daylight-saving rules. And even if the 1024 is a bug, one can debug it more easily by seeing the 1024 in the output than by puzzling over a 0 return code. Although as Martin Sebor mentions we could pretend that strftime is generating an infinitely-long string and use this pretence to return 0, that would be less useful not to mention less compatible with existing practice. > Anyway, I'll send a patch to eliminate just the crashes on OOB tm_mon > and tm_wday. Don't forget out-of-range tm_isdst: there's one place the code has undefined behavior if tm_isdst exceeds 1.
On Sun, Sep 20, 2015 at 11:39 AM, Paul Eggert <eggert@cs.ucla.edu> wrote: >> tp->tm_hour = 1024; >> strftime(..., "%H %I", tp); // produces "1024 04" >> >> that doesn't seem very desirable. > > That helps the programmer more than returning 0 would. Yes, you convinced me that returning 0 is the wrong thing to do. The remaining question was whether tm_hour==1024 should map to "1024" or to "?". Your other example suggests that "1024" is more useful. > Don't forget out-of-range tm_isdst: there's one place the code has undefined > behavior if tm_isdst exceeds 1. Thanks!
On 20 Sep 2015 11:45, Paul Pluzhnikov wrote: > On Sun, Sep 20, 2015 at 11:39 AM, Paul Eggert wrote: > >> tp->tm_hour = 1024; > >> strftime(..., "%H %I", tp); // produces "1024 04" > >> > >> that doesn't seem very desirable. > > > > That helps the programmer more than returning 0 would. > > Yes, you convinced me that returning 0 is the wrong thing to do. > > The remaining question was whether tm_hour==1024 should map to "1024" > or to "?". Your other example suggests that "1024" is more useful. as i mentioned in the previous thread, i think dumping more details would be nicer, and still permissible by the standard. using tm_hour=1024, we might have something like: $ cat test.c #include <stdio.h> #include <string.h> #include <time.h> int main() { char buf[1024]; struct tm tm; memset(&tm, 0, sizeof(tm)); tm.tm_hour = 1024; size_t ret = strftime(buf, sizeof(buf), "%H %I", &tm); printf("buf = %s\n", buf); } $ ./a.out buf = INVALID<H/tm_hour=1024> 1012 i'm not set on any particular structured format ... just including more details to make it overly obvious where the problem lies. -mike
On Sun, Sep 20, 2015 at 4:10 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> buf = INVALID<H/tm_hour=1024> 1012
It's fairly trivial to modify my patch to show "INVALID tm_wday" instead of "?".
Doing that for tm_hour is just a bit more involved, though not that
much. But it seems that that would be something Paul E. *doesn't* want
in his "IST-2IDT,M3.4.4/26,M10.5.0" example.
On 20 Sep 2015 16:16, Paul Pluzhnikov wrote: > On Sun, Sep 20, 2015 at 4:10 PM, Mike Frysinger wrote: > > buf = INVALID<H/tm_hour=1024> 1012 > > It's fairly trivial to modify my patch to show "INVALID tm_wday" instead of "?". using a const string is fine. better than just "?" imo. > Doing that for tm_hour is just a bit more involved, though not that > much. But it seems that that would be something Paul E. *doesn't* want > in his "IST-2IDT,M3.4.4/26,M10.5.0" example. i'm not sure that's readily obvious that there's a failure. seems like it's easy to pass on to other layers which also might not notice until it's much too late. -mike
On 09/20/2015 05:10 PM, Mike Frysinger wrote: > On 20 Sep 2015 11:45, Paul Pluzhnikov wrote: >> On Sun, Sep 20, 2015 at 11:39 AM, Paul Eggert wrote: >>>> tp->tm_hour = 1024; >>>> strftime(..., "%H %I", tp); // produces "1024 04" >>>> >>>> that doesn't seem very desirable. >>> >>> That helps the programmer more than returning 0 would. >> >> Yes, you convinced me that returning 0 is the wrong thing to do. >> >> The remaining question was whether tm_hour==1024 should map to "1024" >> or to "?". Your other example suggests that "1024" is more useful. > > as i mentioned in the previous thread, i think dumping more details would > be nicer, and still permissible by the standard. using tm_hour=1024, we > might have something like: > > $ cat test.c > #include <stdio.h> > #include <string.h> > #include <time.h> > int main() > { > char buf[1024]; > struct tm tm; > memset(&tm, 0, sizeof(tm)); > tm.tm_hour = 1024; > size_t ret = strftime(buf, sizeof(buf), "%H %I", &tm); > printf("buf = %s\n", buf); > } > > $ ./a.out > buf = INVALID<H/tm_hour=1024> 1012 This approach will cause the function to exceed the size of the destination buffer when it's sized just right for valid input. This will then lead to two failure modes for the same problem: strftime producing "<INVALID>" for invalid input, and returning zero for the same invalid input, depending on the size of the destination buffer. Martin > > i'm not set on any particular structured format ... just including more > details to make it overly obvious where the problem lies. > -mike >
Mike Frysinger <vapier@gentoo.org> writes: > $ ./a.out > buf = INVALID<H/tm_hour=1024> 1012 I don't think that makes any sense. strftime should do nothing special for numeric formats. Andreas.
On 21 Sep 2015 09:30, Andreas Schwab wrote: > Mike Frysinger <vapier@gentoo.org> writes: > > $ ./a.out > > buf = INVALID<H/tm_hour=1024> 1012 > > I don't think that makes any sense. strftime should do nothing special > for numeric formats. i'm fine with outputting INVALID<fmt/field> for any invalid field. the =%i part doesn't need to be there. -mike
diff --git a/time/strftime_l.c b/time/strftime_l.c index b48ef34..6a64b8a 100644 --- a/time/strftime_l.c +++ b/time/strftime_l.c @@ -40,6 +40,7 @@ #endif #include <ctype.h> +#include <errno.h> #include <sys/types.h> /* Some systems define `time_t' here. */ #ifdef TIME_WITH_SYS_TIME @@ -497,6 +498,20 @@ __strftime_internal (s, maxsize, format, tp, tzset_called ut_argument ut_argument_spec LOCALE_PARAM_DECL { + /* Do range checks first. BZ #18985 */ + if (tp->tm_sec < 0 || tp->tm_sec > 60 + || tp->tm_min < 0 || tp->tm_min > 59 + || tp->tm_hour < 0 || tp->tm_hour > 23 + || tp->tm_mday < 0 || tp->tm_mday > 31 + || tp->tm_mon < 0 || tp->tm_mon > 11 + || tp->tm_wday < 0 || tp->tm_wday > 6 + || tp->tm_yday < 0 || tp->tm_yday > 365 + || tp->tm_isdst < 0 || tp->tm_isdst > 1) + { + __set_errno (EINVAL); + return 0; + } + #if defined _LIBC && defined USE_IN_EXTENDED_LOCALE_MODEL struct __locale_data *const current = loc->__locales[LC_TIME];