This is the mail archive of the libc-help@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: strftime segfault vs return error code


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.

Also when you return 0 you encounter bug thats described in manpage as
you couldn't distinguish zero output condition from too small buffer.
This could cause problem if one writes strftime as

while (strftime (buf,size,...) == 0)
  {
    size = 2 * size;
    buf = realloc (buf, size);
  }

> (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.
 
> Adam: could you file a bug in our bugzilla with details & your test case ?
> 	https://sourceware.org/bugzilla/
> -mike


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]