This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [patch] Fix BZ 18985 out of bounds access in strftime
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Paul Pluzhnikov <ppluzhnikov at google dot com>, Andreas Schwab <schwab at linux-m68k dot org>
- Cc: Paul Eggert <eggert at cs dot ucla dot edu>, GLIBC Devel <libc-alpha at sourceware dot org>
- Date: Fri, 25 Sep 2015 15:26:59 -0400
- Subject: Re: [patch] Fix BZ 18985 out of bounds access in strftime
- Authentication-results: sourceware.org; auth=none
- References: <CAPC3xao-5YF_icBWE5yYbaYiUmiAvmb7w9s_G-dqawsx7eoTkQ at mail dot gmail dot com> <55FE5473 dot 7030305 at cs dot ucla dot edu> <CALoOobOWfPWuwtw_XgcTKx2yn=p3YbB04_B965zKRCkC1qsPjQ at mail dot gmail dot com> <55FEFD47 dot 4090401 at cs dot ucla dot edu> <CALoOobMOFtQVcCK4bR3VpeLMoj4bXVa12GAk0CjP6MMxuQU+Fw at mail dot gmail dot com> <CALoOobP2BQhWp_2NAvFyXfo9UVwr8mQCKbyL-8SJF8U3VoQRsA at mail dot gmail dot com> <87eghsvjrn dot fsf at igel dot home> <CALoOobNVmgx8+4aMpdN4t4seC-UAu9WFx8V5XCSk03hYM0GUtQ at mail dot gmail dot com>
Paul,
The patch looks great, thanks for the fix.
One quibble about the test case.
On 09/20/2015 06:50 PM, Paul Pluzhnikov wrote:
> diff --git a/time/tst-strftime.c b/time/tst-strftime.c
> index 374fba4..5a592c2 100644
> --- a/time/tst-strftime.c
> +++ b/time/tst-strftime.c
> @@ -4,6 +4,24 @@
> #include <time.h>
>
>
> +static int
> +do_bz18985 (void)
> +{
> + char buf[1000];
> + struct tm ttm;
> +
> + memset (&ttm, 1, sizeof (ttm));
> + ttm.tm_zone = NULL; /* Dereferenced directly if non-NULL. */
> + strftime (buf, sizeof (buf), "%a %A %b %B %c %z %Z", &ttm);
> +
> + /* Check negative values as well. */
> + memset (&ttm, 0xFF, sizeof (ttm));
> + ttm.tm_zone = NULL; /* Dereferenced directly if non-NULL. */
> + strftime (buf, sizeof (buf), "%a %A %b %B %c %z %Z", &ttm);
This test is insufficient.
We need comments about exactly what we are testing, and we need to
validate that the result is as expected. We should verify that the
resulting output has a `?' in the expected position. If we can't do that
then we've only validated half your change, namely that it doesn't
crash, but what if it starts returning valid values instead of `?'?
OK with that fixup.
> +
> + return 0;
> +}
> +
Cheers,
Carlos.