Re: strftime segfault vs return error code

[Mike Frysinger <vapier at gentoo dot org>]

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
(2) this seems like a func that gets called fairly directly with external data

Adam: could you file a bug in our bugzilla with details & your test case ?
	https://sourceware.org/bugzilla/
-mike

Attachment: signature.asc
Description: Digital signature