Re: strftime segfault vs return error code

[Mike Frysinger <vapier at gentoo dot org>]

On 28 Jul 2015 09:24, OndÅej BÃlka wrote:
> On Mon, Jul 27, 2015 at 11:37:25PM -0400, Mike Frysinger wrote:
> > On 12 Jul 2015 09:07, OndÅej BÃlka wrote:
> > > On Sun, Jul 12, 2015 at 04:34:23PM +1000, Adam Nielsen wrote:
> > > > I'm debugging a long standing problem with the GKrellM app, and I have
> > > > found that it is triggered by a bug that passes out-of-range data to
> > > > the strftime() function.
> > > > 
> > > > This results in a segfault (in strlen(), which must be called
> > > > internally by strftime) so I am wondering whether this is the ideal
> > > > behaviour.  I would have expected out-of-range values to cause strftime
> > > > to return an error (or an empty string) rather than crash.
> > > > 
> > > > You can reproduce this error by setting an out-of-range value for the
> > > > month, and then supplying a format specifier for the month name.  Here
> > > > is an example:
> > > > 
> > > >   #include <time.h>
> > > >   #include <stdio.h>
> > > >   #include <stdlib.h>
> > > >   #include <string.h>
> > > > 
> > > >   int main(int argc, char *argv[]) {
> > > >     char outstr[200];
> > > >     struct tm tmp;
> > > >     memset(&tmp, 0, sizeof(tmp));
> > > >     tmp.tm_mon = 1000;
> > > > 
> > > >     if (strftime(outstr, sizeof(outstr), "%b", &tmp) == 0) {
> > > >       fprintf(stderr, "strftime returned 0");
> > > >       exit(EXIT_FAILURE);
> > > >     }
> > > > 
> > > >     printf("Result string is \"%s\"\n", outstr);
> > > >     exit(EXIT_SUCCESS);
> > > >   }
> > > > 
> > > > Wouldn't it be better in this case for strftime() to return 0, rather
> > > > than crashing?  I'm not sure if there are any security implications in
> > > > this current behaviour.
> > >
> > > From practical perspective crashing/abort tends to be best. Users
> > > typically don't check return value and its better fail early and loudly
> > > than silently corrupting data. As timespec is constructed by programmer
> > > he wrote underlying bug that caused it, strptime doesn't set invalid
> > > months.
> > > 
> > > You could write patch to add asserts in strftime to make debugging
> > > easier. 
> > 
> > when dealing with invalid pointers, i would certainly agree.  but that isn't 
> > really what we're discussing here -- we're looking at out-of-range numbers.
> > 
> > afaict, POSIX really only has this to say on the matter:
> > http://pubs.opengroup.org/onlinepubs/9699919799/functions/strftime.html
> > DESCRIPTION
> > 	If any of the specified values are outside the normal range, the characters 
> > 	stored are unspecified.
> > RETURN VALUE
> > 	If the total number of resulting bytes including the terminating null byte 
> > 	is not more than maxsize, these functions shall return the number of bytes 
> > 	placed into the array pointed to by s, not including the terminating NUL 
> > 	character. Otherwise, 0 shall be returned and the contents of the array 
> > 	are unspecified.
> > ERRORS
> > 	No errors are defined.
> > 
> > the strftime man page also defines no possible error values.  considering its 
> > API format, i'm guessing most people don't really do error checking here beyond
> > NUL terminating the buffer after the call.
> > 
> > imo we should clamp the possible values when we go indexing arrays because:
> > (1) it's cheap
>
> Thats possible but still problematic. As users don't check result most
> of time it would cause invalid inputs to be ignored. Then it won't print
> time and corrupt output file format. I still think that aborting is
> sanest way.

fundamentally, i disagree.  further, this code isn't guaranteed to crash ...
it's directly adding the value to a data pointer and then treating it like a
NUL terminated C string:
# define a_wkday \
  ((const CHAR_T *) _NL_CURRENT (LC_TIME, NLW(ABDAY_1) + tp->tm_wday))
# define f_wkday \
  ((const CHAR_T *) _NL_CURRENT (LC_TIME, NLW(DAY_1) + tp->tm_wday))
# define a_month \
  ((const CHAR_T *) _NL_CURRENT (LC_TIME, NLW(ABMON_1) + tp->tm_mon))
# define f_month \
  ((const CHAR_T *) _NL_CURRENT (LC_TIME, NLW(MON_1) + tp->tm_mon))

so you're just as likely to get binary garbage in your output as you are a
crash, nor can you guarantee that the output isn't written incrementally (a
common idiom) which means the output is still corrupt while crashing.

i get the feeling a lot of people treat this as a "harmless" func that, as
long as the struct is valid memory, it shouldn't crash.  especially
considering how many applications expose access to it fairly directly (e.g.
php's strftime calls directly into the C library, albeit taking an epoch
timestamp rather than struct tm).  it's in the same class as the other
printf functions -- people don't expect them to crash when the memory is
valid.

also, no where in the manual does it say "if you set this integer to 12
instead of 11, then your code might crash", nor the man page.  if anything,
the manual makes it read like a fairly safe function to leverage.

> Also when you return 0 you encounter bug thats described in manpage as
> you couldn't distinguish zero output condition from too small buffer.

the glibc manual already explains how to handle this case.  not that i'm
advocating for returning 0 -- we can just as easily write out something
like INVALID<%i>.

> > (2) this seems like a func that gets called fairly directly with external data
> 
> As external data it shouldn't matter as they also should be created with
> strptime calls so these should be valid.

i don't think you can rely on that.  there's a dirth of timedelta APIs which
means people have to resort to doing things themselves.  nor does strftime
say you must use strptime (and if anything, the manual talks about using
strptime to convert the output of strftime back into binary data).  nor is
strptime the only function that returns a struct tm.
-mike

Attachment: signature.asc
Description: Digital signature