Re: [bug-gnulib] PING: strftime bugs

[Eric Blake]

According to Paul Eggert on 2/16/2005 12:52 PM:
> 
> 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?
> 
> --- strftime.c.~1.78.~        2004-11-10 21:32:43 -0800
> +++ strftime.c        2005-02-16 11:51:30 -0800
> @@ -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);

Per the POSIX specification of <time.h>, tp->tm_yday is restricted to be
between 0 and 365 in a compliant program, so the user passing in a
negative tm_yday evokes unspecified behavior.  The existing behavior did
not crash on illegal input, so why add another comparison in the
instruction stream to format the illegal input nicely?  (Similarly to a
couple of other modifiers that you converted to DO_SIGNED_NUMBER - only
tm_year and tm_isdst are unrestricted in value.)

> @@ -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);

The old formula was checking if the PREVIOUS year was a leap year, you
changed it to check if the current year is leap year.  That will break %V
(Jan 1 is week 53 if the previous year ended on Thursday, or if the
previous year was leap year and ended on Friday; otherwise Jan 1 is week
52 if it belongs to the previous year).

>             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));

You just switched %G from 1-character minimum to 4-character minimum; I
guess that is okay since no spec calls out the minimum width, and this
will make %G consistent with %Y.

> @@ -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);

In this case, your approach is much better than mine - it still allows
width and padding formatting to take place on a single number rather than
adding recursion, and I'm okay with your use of guaranteed 2's complement
unsigned math avoiding overflow, although a comment might be helpful.

Overall, your patch seems pretty good to me, as a nice improvement on my
initial ideas.  Attached is a test program I wrote for my use when fixing
the same strftime bugs in newlib (admittedly it doesn't test everything,
since newlib is hardcoded to the "C" locale and does not support width, -,
or _ modifiers, but it helps).

-- 
Life is short - so eat dessert first!

Eric Blake             address@hidden