diff mbox

Fix BZ 18985 out of bounds access in strftime

Message ID CALoOobNVmgx8+4aMpdN4t4seC-UAu9WFx8V5XCSk03hYM0GUtQ@mail.gmail.com
State Committed
Headers

Commit Message

Paul Pluzhnikov Sept. 20, 2015, 10:50 p.m. UTC 
  On Sun, Sep 20, 2015 at 2:54 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:

> Line break before operator, not after.

OK.

2015-09-20  Paul Pluzhnikov  <ppluzhnikov@google.com>

        [BZ #18985]
        * time/strftime_l.c (a_wkday, f_wkday, a_month, f_month): Range check.
        (__strftime_internal): Likewise.
        * time/tst-strftime.c (do_bz18985): New test.
        (do_test): Call it.

Comments

Mike Frysinger Sept. 25, 2015, 2:12 p.m. UTC | #1 
On 20 Sep 2015 15:50, Paul Pluzhnikov wrote:
> On Sun, Sep 20, 2015 at 2:54 PM, Andreas Schwab wrote:
> > Line break before operator, not after.
> 
> OK.

this looks fine to me.  i think we can do better, but that doesn't
mean we can't land this incremental improvement in the mean time.
-mike
Carlos O'Donell Sept. 25, 2015, 7:26 p.m. UTC | #2 
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.
diff mbox

Patch

diff --git a/time/strftime_l.c b/time/strftime_l.c
index b48ef34..4eb647c 100644
--- a/time/strftime_l.c
+++ b/time/strftime_l.c
@@ -510,13 +510,17 @@  __strftime_internal (s, maxsize, format, tp, tzset_called ut_argument
      only a few elements.  Dereference the pointers only if the format
      requires this.  Then it is ok to fail if the pointers are invalid.  */
 # define a_wkday \
-  ((const CHAR_T *) _NL_CURRENT (LC_TIME, NLW(ABDAY_1) + tp->tm_wday))
+  ((const CHAR_T *) (tp->tm_wday < 0 || tp->tm_wday > 6			     \
+		     ? "?" : _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))
+  ((const CHAR_T *) (tp->tm_wday < 0 || tp->tm_wday > 6			     \
+		     ? "?" : _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))
+  ((const CHAR_T *) (tp->tm_mon < 0 || tp->tm_mon > 11			     \
+		     ? "?" : _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))
+  ((const CHAR_T *) (tp->tm_mon < 0 || tp->tm_mon > 11			     \
+		     ? "?" : _NL_CURRENT (LC_TIME, NLW(MON_1) + tp->tm_mon)))
 # define ampm \
   ((const CHAR_T *) _NL_CURRENT (LC_TIME, tp->tm_hour > 11		      \
 				 ? NLW(PM_STR) : NLW(AM_STR)))
@@ -526,8 +530,10 @@  __strftime_internal (s, maxsize, format, tp, tzset_called ut_argument
 # define ap_len STRLEN (ampm)
 #else
 # if !HAVE_STRFTIME
-#  define f_wkday (weekday_name[tp->tm_wday])
-#  define f_month (month_name[tp->tm_mon])
+#  define f_wkday (tp->tm_wday < 0 || tp->tm_wday > 6	\
+		   ? "?" : weekday_name[tp->tm_wday])
+#  define f_month (tp->tm_mon < 0 || tp->tm_mon > 11	\
+		   ? "?" : month_name[tp->tm_mon])
 #  define a_wkday f_wkday
 #  define a_month f_month
 #  define ampm (L_("AMPM") + 2 * (tp->tm_hour > 11))
@@ -1321,7 +1327,7 @@  __strftime_internal (s, maxsize, format, tp, tzset_called ut_argument
 		  *tzset_called = true;
 		}
 # endif
-	      zone = tzname[tp->tm_isdst];
+	      zone = tp->tm_isdst <= 1 ? tzname[tp->tm_isdst] : "?";
 	    }
 #endif
 	  if (! zone)
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);
+
+  return 0;
+}
+
 static struct
 {
   const char *fmt;
@@ -104,7 +122,7 @@  do_test (void)
 	}
     }
 
-  return result;
+  return result + do_bz18985 ();
 }
 
 #define TEST_FUNCTION do_test ()