Re: [bug-gnulib] PING: strftime bugs

[Paul Eggert]

Eric Blake <address@hidden> writes:

> It's been a while, any comments on this patch?

I think it's incomplete and many of the bugs will remain even after
it's applied.  Fixing this is on my to-do list.

Here's my current patch.  It is completely untested and unreviewed and
quite possibly wrong, but I thought I'd give you a snapshot to let you
know what sort of issues are involved.  Perhaps you can review it
and/or test it with your test cases?

> Should I also file it upstream with glibc?

I wouldn't bother.  Let's get it right first.  They have bought back
changes from gnulib before, and are likely to do so in the future.

--- strftime.c.~1.78.~  2004-11-10 21:32:43 -0800
+++ strftime.c  2005-02-16 11:51:30 -0800
@@ -1,4 +1,4 @@
-/* Copyright (C) 1991-1999, 2000, 2001, 2003, 2004 Free Software
+/* Copyright (C) 1991-1999, 2000, 2001, 2003, 2004, 2005 Free Software
    Foundation, Inc.
 
    NOTE: The canonical source of this file is maintained with the GNU C 
Library.
@@ -72,6 +72,7 @@ extern char *tzname[];
 #endif
 
 #include <limits.h>
+#include <stdbool.h>
 #include <stddef.h>
 #include <stdlib.h>
 #include <string.h>
@@ -131,13 +132,17 @@ extern char *tzname[];
 
 #define TM_YEAR_BASE 1900
 
-#ifndef __isleap
-/* Nonzero if YEAR is a leap year (every 4 years,
-   except every 100th isn't, and every 400th is).  */
-# define __isleap(year)        \
-  ((year) % 4 == 0 && ((year) % 100 != 0 || (year) % 400 == 0))
-#endif
-
+/* Return 1 if YEAR + TM_YEAR_BASE is a leap year.  */
+static inline bool
+leapyear (int year)
+{
+  /* Don't add YEAR to TM_YEAR_BASE, as that might overflow.
+     Also, work even if YEAR is negative.  */
+  return
+    ((year & 3) == 0
+     && (year % 100 != 0
+        || ((year / 100) & 3) == (- (TM_YEAR_BASE / 100) & 3)));
+}
 
 #ifdef _LIBC
 # define tzname __tzname
@@ -479,6 +484,7 @@ my_strftime (CHAR_T *s, size_t maxsize, 
       int modifier;            /* Field modifier ('E', 'O', or 0).  */
       int digits;              /* Max digits for numeric format.  */
       int number_value;                /* Numeric value to be printed.  */
+      unsigned int unsigned_number_value; /* (unsigned int) number_value.  */
       int negative_number;     /* 1 if the number is negative.  */
       const CHAR_T *subfmt;
       CHAR_T *bufp;
@@ -645,6 +651,10 @@ my_strftime (CHAR_T *s, size_t maxsize, 
 #define DO_NUMBER(d, v) \
          digits = d > width ? d : width;                                     \
          number_value = v; goto do_number
+#define DO_SIGNED_NUMBER(d, negative, v) \
+         digits = d > width ? d : width;                                     \
+         negative_number = negative;                                         \
+         unsigned_number_value = v; goto do_signed_number
 #define DO_NUMBER_SPACEPAD(d, v) \
          digits = d > width ? d : width;                                     \
          number_value = v; goto do_number_spacepad
@@ -807,8 +817,9 @@ my_strftime (CHAR_T *s, size_t maxsize, 
            }
 
          {
-           int year = tp->tm_year + TM_YEAR_BASE;
-           DO_NUMBER (1, year / 100 - (year % 100 < 0));
+           int century = tp->tm_year / 100 + TM_YEAR_BASE / 100;
+           century -= tp->tm_year % 100 < 0 && 0 < century;
+           DO_SIGNED_NUMBER (2, tp->tm_year < - TM_YEAR_BASE, century);
          }
 
        case L_('x'):
@@ -846,8 +857,9 @@ my_strftime (CHAR_T *s, size_t maxsize, 
 
          DO_NUMBER_SPACEPAD (2, tp->tm_mday);
 
-         /* All numeric formats set DIGITS and NUMBER_VALUE and then
-            jump to one of these two labels.  */
+         /* All numeric formats set DIGITS and NUMBER_VALUE (or
+            UNSIGNED_NUMBER_VALUE) and then jump to one of these
+            three labels.  */
 
        do_number_spacepad:
          /* Force `_' flag unless overridden by `0' or `-' flag.  */
@@ -855,14 +867,21 @@ my_strftime (CHAR_T *s, size_t maxsize, 
            pad = L_('_');
 
        do_number:
-         /* Format the number according to the MODIFIER flag.  */
-
-         if (modifier == L_('O') && 0 <= number_value)
+         /* Format NUMBER_VALUE according to the MODIFIER flag.  */
+         negative_number = number_value < 0;
+         unsigned_number_value = number_value;
+
+       do_signed_number:
+         /* Format UNSIGNED_NUMBER_VALUE according to the MODIFIER flag.
+            NEGATIVE_NUMBER is nonzero if the original number was
+            negative; in this case it was converted to unsigned int
+            without negation.  */
+         if (modifier == L_('O') && !negative_number)
            {
 #ifdef _NL_CURRENT
              /* Get the locale specific alternate representation of
-                the number NUMBER_VALUE.  If none exist NULL is returned.  */
-             const CHAR_T *cp = nl_get_alt_digit (number_value
+                the number.  If none exist NULL is returned.  */
+             const CHAR_T *cp = nl_get_alt_digit (unsigned_number_value
                                                   HELPER_LOCALE_ARG);
 
              if (cp != NULL)
@@ -881,10 +900,9 @@ my_strftime (CHAR_T *s, size_t maxsize, 
 #endif
            }
          {
-           unsigned int u = number_value;
+           unsigned int u = unsigned_number_value;
 
            bufp = buf + sizeof (buf) / sizeof (buf[0]);
-           negative_number = number_value < 0;
 
            if (negative_number)
              u = -u;
@@ -974,7 +992,7 @@ my_strftime (CHAR_T *s, size_t maxsize, 
          if (modifier == L_('E'))
            goto bad_format;
 
-         DO_NUMBER (3, 1 + tp->tm_yday);
+         DO_SIGNED_NUMBER (3, tp->tm_yday < -1, tp->tm_yday + 1U);
 
        case L_('M'):
          if (modifier == L_('E'))
@@ -986,7 +1004,7 @@ my_strftime (CHAR_T *s, size_t maxsize, 
          if (modifier == L_('E'))
            goto bad_format;
 
-         DO_NUMBER (2, tp->tm_mon + 1);
+         DO_SIGNED_NUMBER (2, tp->tm_mon < -1, tp->tm_mon + 1U);
 
 #ifndef _LIBC
        case L_('N'):           /* GNU extension.  */
@@ -1131,24 +1149,25 @@ my_strftime (CHAR_T *s, size_t maxsize, 
          if (modifier == L_('E'))
            goto bad_format;
          {
-           int year = tp->tm_year + TM_YEAR_BASE;
+           int days_in_year = 365 + leapyear (tp->tm_year);
+           int year_adjust = 0;
            int days = iso_week_days (tp->tm_yday, tp->tm_wday);
 
            if (days < 0)
              {
                /* This ISO week belongs to the previous year.  */
-               year--;
-               days = iso_week_days (tp->tm_yday + (365 + __isleap (year)),
+               year_adjust = -1;
+               days = iso_week_days (tp->tm_yday + days_in_year,
                                      tp->tm_wday);
              }
            else
              {
-               int d = iso_week_days (tp->tm_yday - (365 + __isleap (year)),
+               int d = iso_week_days (tp->tm_yday - days_in_year,
                                       tp->tm_wday);
                if (0 <= d)
                  {
                    /* This ISO week belongs to the next year.  */
-                   year++;
+                   year_adjust = 1;
                    days = d;
                  }
              }
@@ -1156,10 +1175,19 @@ my_strftime (CHAR_T *s, size_t maxsize, 
            switch (*f)
              {
              case L_('g'):
-               DO_NUMBER (2, (year % 100 + 100) % 100);
+               {
+                 int yy = (tp->tm_year % 100 + year_adjust) % 100;
+                 if (yy < 0)
+                   yy = (tp->tm_year < -TM_YEAR_BASE - year_adjust
+                         ? -yy
+                         : yy + 100);
+                 DO_NUMBER (2, yy);
+               }
 
              case L_('G'):
-               DO_NUMBER (1, year);
+               DO_SIGNED_NUMBER (4, tp->tm_year < -TM_YEAR_BASE - year_adjust,
+                                 (tp->tm_year + (unsigned int) TM_YEAR_BASE
+                                  + year_adjust));
 
              default:
                DO_NUMBER (2, days / 7 + 1);
@@ -1201,7 +1229,8 @@ my_strftime (CHAR_T *s, size_t maxsize, 
          if (modifier == L_('O'))
            goto bad_format;
          else
-           DO_NUMBER (1, tp->tm_year + TM_YEAR_BASE);
+           DO_SIGNED_NUMBER (4, tp->tm_year < -TM_YEAR_BASE,
+                             tp->tm_year + (unsigned int) TM_YEAR_BASE);
 
        case L_('y'):
          if (modifier == L_('E'))
@@ -1220,7 +1249,13 @@ my_strftime (CHAR_T *s, size_t maxsize, 
 # endif
 #endif
            }
-         DO_NUMBER (2, (tp->tm_year % 100 + 100) % 100);
+
+         {
+           int yy = tp->tm_year % 100;
+           if (yy < 0)
+             yy = tp->tm_year < - TM_YEAR_BASE ? -yy : yy + 100;
+           DO_NUMBER (2, yy);
+         }
 
        case L_('Z'):
          if (change_case)
@@ -1232,7 +1267,7 @@ my_strftime (CHAR_T *s, size_t maxsize, 
 #if HAVE_TZNAME
          /* The tzset() call might have changed the value.  */
          if (!(zone && *zone) && tp->tm_isdst >= 0)
-           zone = tzname[tp->tm_isdst];
+           zone = tzname[tp->tm_isdst != 0];
 #endif
          if (! zone)
            zone = "";