Attention to bug 170444

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Attention to bug 170444

Thomas Bushnell BSG

Attention please to http://bugzilla.gnome.org/show_bug.cgi?id=170444
(Debian bug http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=312109).

I believe I have the fix.  The problem is that many locales have date
string formats longer than eleven characters.  Eleven was fine for
"%d/%m/%Y" but not for anything longer.  Then when printDate is
called, it overruns the provided buffer if we are using a
locale-provided date string.

There is much chaos in the printDate regime in gnucash, with some
callers using MAX_DATE_LENGTH and other callers using 100 or making up
their own buffer sizes on the fly.  I'm bumping MAX_DATE_LENGTH from
eleven to forty for Debian (which will close the Debian bug), but it
would be nice if the upstream maintainers (y'all!) could at least put
a patch into the 1.8 branch and keep in mind that reported bugs about
random crashes, particularly if the locale is one of the following,
could be this problem.

In recent GNU libc, I think the following locales have such a problem:

ar_AE ar_BH ar_DZ ar_EG ar_IQ ar_JO ar_KW ar_LB ar_LY ar_MA ar_OM
ar_QA ar_SD ar_SY ar_TN ar_YE ar_SA ar_IN bn_BN bn_IN en_IN gu_IN
hi_IN kn_IN mr_IN ms_MY ne_NP pa_IN ta_IN te_IN en_HK en_PH en_SG
ml_IN eu_ES fa_IR is_IS ja_JP kl_GL ko_KR lv_LV mt_MT nn_NO sl_SI
sr_CS zh_CN zh_SG zh_HK zh_TW

Note particularly that all the Arabic, Chinese, Japanese, and Korean
locales will have a problem; likewise all the variants for India.
English variants for southeast Asia also have problems.  Those are
some of the more notable ones.  Basque too.

Thomas
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: Attention to bug 170444

Thomas Bushnell BSG
Thomas Bushnell BSG <[hidden email]> writes:

> Attention please to http://bugzilla.gnome.org/show_bug.cgi?id=170444
> (Debian bug http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=312109).
>
> I believe I have the fix.  The problem is that many locales have date
> string formats longer than eleven characters.  Eleven was fine for
> "%d/%m/%Y" but not for anything longer.  Then when printDate is
> called, it overruns the provided buffer if we are using a
> locale-provided date string.

Lol.  This is stunning.  The bug is freakier than I thought at first.
It is a bug that gnucash only allows 11 character long dates (and
should be fixed, at least in the gnome-2 branch, if it hasn't been
already).  And there is the chaos I mentioned.

But gnucash correctly fills in "11" for the MAX argument to strftime,
and never passes it a buffer shorter than that.  Upping
MAX_DATE_LENGTH doesn't fix anything.  The problem indeed has nothing
to do with buffer lengths.

Indeed, the following program generates a segv with glibc:


#include <time.h>
#include <stdio.h>

main ()
{
  char buffer[1000];
  struct tm tm_str;

  tm_str.tm_mday = 28;
  tm_str.tm_mon = 7;
  tm_str.tm_year = 105;
  tm_str.tm_hour = 0;
  tm_str.tm_min = 0;
  tm_str.tm_sec = 0;
  tm_str.tm_isdst = -1;
   
  strftime (buffer, 1000, "%A, %d %B, %Y", &tm_str);
  printf ("%s\n", buffer);
}


Isn't that impressive?!

Now, glibc is really casual in its use of strftime, and assumes that
the date will always fit.  Once the glibc bug is fixed, there will now
be bugs in using strftime when the locale's date string is too big to
fit in the buffer.  What glibc does in that case (when it isn't
blowing chunks) is to leave an *undefined* buffer, not necessarily
even null terminated.  Every call to strftime with any
locale-dependent format, should check the return value to avoid
overflows.



_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: Attention to bug 170444

Neil Williams-2
On Sunday 28 August 2005 11:39 pm, Thomas Bushnell BSG wrote:
> Thomas Bushnell BSG <[hidden email]> writes:
> > Attention please to http://bugzilla.gnome.org/show_bug.cgi?id=170444
> > (Debian bug http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=312109).
> >
> > I believe I have the fix.  The problem is that many locales have date
> > string formats longer than eleven characters.

As part of my work on QOF to handle UTC times and to output those to XML in an
xml-compatible form (that validates with a schema), I'd already upped the
length to 31:
#define MAX_DATE_LENGTH 31
gnc-date.h

I've no problem making it 40 - do some formats produce strings between 31 and
40?

> Lol.  This is stunning.  The bug is freakier than I thought at first.
> It is a bug that gnucash only allows 11 character long dates

G2 has been using 31 for about 6 months now.

> But gnucash correctly fills in "11" for the MAX argument to strftime,
> and never passes it a buffer shorter than that.  Upping
> MAX_DATE_LENGTH doesn't fix anything.  The problem indeed has nothing
> to do with buffer lengths.

Well, at least the 11 limit has been fixed.

--

Neil Williams
=============
http://www.data-freedom.org/
http://www.nosoftwarepatents.com/
http://www.linux.codehelp.co.uk/


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel

attachment0 (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Attention to bug 170444

Thomas Bushnell BSG
In reply to this post by Thomas Bushnell BSG
Thomas Bushnell BSG <[hidden email]> writes:

> Indeed, the following program generates a segv with glibc:
>
>
> #include <time.h>
> #include <stdio.h>
>
> main ()
> {
>   char buffer[1000];
>   struct tm tm_str;
>
>   tm_str.tm_mday = 28;
>   tm_str.tm_mon = 7;
>   tm_str.tm_year = 105;
>   tm_str.tm_hour = 0;
>   tm_str.tm_min = 0;
>   tm_str.tm_sec = 0;
>   tm_str.tm_isdst = -1;
>    
>   strftime (buffer, 1000, "%A, %d %B, %Y", &tm_str);
>   printf ("%s\n", buffer);
> }

Sorry for all the email.  Turns out, not a glibc bug.

The problem here is that tm_wday is not initialized, and some locales
print the weekday as part of the default date format.  Whew.

So the correct fix is to use mktime to generate the day of week data,
with the enclosed patch.

Please also put on a todo list somewhere to fix all the uses of
strftime with locale-dependent format strings, first to deal with
return values of zero, and second to use mktime to fill in the
complete tm structure.

--- gnucash-1.8.10.orig/src/engine/date.c
+++ gnucash-1.8.10/src/engine/date.c
@@ -362,19 +362,25 @@
       sprintf (buff, "%2d.%2d.%-4d", day, month, year);
       break;
     case DATE_FORMAT_ISO:
+  iso_date:
       sprintf (buff, "%04d-%02d-%02d", year, month, day);
       break;
     case DATE_FORMAT_LOCALE:
       {
         struct tm tm_str;
+ time_t t;
+ int n;
 
         tm_str.tm_mday = day;
         tm_str.tm_mon = month - 1;    /* tm_mon = 0 through 11 */
         tm_str.tm_year = year - 1900; /* this is what the standard
                                        * says, it's not a Y2K thing */
-
  gnc_tm_set_day_start (&tm_str);
-        strftime (buff, MAX_DATE_LENGTH, GNC_D_FMT, &tm_str);
+ t = mktime (&tm_str);
+ localtime_r (&t, &tm_str);
+        n = strftime (buff, MAX_DATE_LENGTH, GNC_D_FMT, &tm_str);
+ if (n == 0)
+  goto iso_date;
       }
       break;
 
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: Attention to bug 170444

Derek Atkins
Thomas,

Quoting Thomas Bushnell BSG <[hidden email]>:

> --- gnucash-1.8.10.orig/src/engine/date.c
> +++ gnucash-1.8.10/src/engine/date.c
> @@ -362,19 +362,25 @@
>       sprintf (buff, "%2d.%2d.%-4d", day, month, year);
>       break;
>     case DATE_FORMAT_ISO:
> +  iso_date:
>       sprintf (buff, "%04d-%02d-%02d", year, month, day);
>       break;
>     case DATE_FORMAT_LOCALE:
>       {
>         struct tm tm_str;
> + time_t t;
> + int n;
>
>         tm_str.tm_mday = day;
>         tm_str.tm_mon = month - 1;    /* tm_mon = 0 through 11 */
>         tm_str.tm_year = year - 1900; /* this is what the standard
>                                        * says, it's not a Y2K thing */
> -
> gnc_tm_set_day_start (&tm_str);
> -        strftime (buff, MAX_DATE_LENGTH, GNC_D_FMT, &tm_str);
> + t = mktime (&tm_str);
> + localtime_r (&t, &tm_str);
> +        n = strftime (buff, MAX_DATE_LENGTH, GNC_D_FMT, &tm_str);
> + if (n == 0)
> +  goto iso_date;
>       }
>       break;

Any reason not to change this to remove the goto?  We could just
reorder the two
case statements and change the code from:

  if (n == 0)
    goto iso_date;
  }
  break;

to...

  if (n != 0)
    break;
  }
  /* FALLTHROUGH */

I'll try to apply this to 1.8 and g2 either tonight or, more likely, tomorrow.

-derek

--
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       [hidden email]                        PGP key available

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: Attention to bug 170444

Thomas Bushnell BSG
Derek Atkins <[hidden email]> writes:

> Any reason not to change this to remove the goto?  We could just
> reorder the two
> case statements and change the code from:
>
>  if (n == 0)
>    goto iso_date;
>  }
>  break;
>
> to...
>
>  if (n != 0)
>    break;
>  }
>  /* FALLTHROUGH */

Looks equivalent to me.

> I'll try to apply this to 1.8 and g2 either tonight or, more likely,
> tomorrow.

Timing doesn't matter from my end; I've made the change already in
Debian.

How about an audit of the other uses of strftime?


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: Attention to bug 170444

Derek Atkins
We should probably have one gnucash API that calls strftime() and have
all other
places just call the gnc api.

-derek

Quoting Thomas Bushnell BSG <[hidden email]>:

> Derek Atkins <[hidden email]> writes:
>
>> Any reason not to change this to remove the goto?  We could just
>> reorder the two
>> case statements and change the code from:
>>
>>  if (n == 0)
>>    goto iso_date;
>>  }
>>  break;
>>
>> to...
>>
>>  if (n != 0)
>>    break;
>>  }
>>  /* FALLTHROUGH */
>
> Looks equivalent to me.
>
>> I'll try to apply this to 1.8 and g2 either tonight or, more likely,
>> tomorrow.
>
> Timing doesn't matter from my end; I've made the change already in
> Debian.
>
> How about an audit of the other uses of strftime?
>
>
>



--
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       [hidden email]                        PGP key available

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: Attention to bug 170444

Thomas Bushnell BSG
Derek Atkins <[hidden email]> writes:

> We should probably have one gnucash API that calls strftime() and
> have all other places just call the gnc api.

That surely seems best to me, especially given the brokenness of the
strftime specification.

Thomas
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: Attention to bug 170444

Neil Williams-2
In reply to this post by Thomas Bushnell BSG
On Sunday 28 August 2005 11:39 pm, Thomas Bushnell BSG wrote:

This didn't show up on the list first time, can't see why.

> Thomas Bushnell BSG <[hidden email]> writes:
> > Attention please to http://bugzilla.gnome.org/show_bug.cgi?id=170444
> > (Debian bug http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=312109).
> >
> > I believe I have the fix.  The problem is that many locales have date
> > string formats longer than eleven characters.

As part of my work on QOF to handle UTC times and to output those to XML in an
xml-compatible form (that validates with a schema), I'd already upped the
length to 31:
#define MAX_DATE_LENGTH 31
gnc-date.h

These XML UTC times are not locale-sensitive.

I've no problem making it 40 - do some formats produce strings between 31 and
40?

> Lol.  This is stunning.  The bug is freakier than I thought at first.
> It is a bug that gnucash only allows 11 character long dates

G2 has been using 31 for about 6 months now.

--

Neil Williams
=============
http://www.data-freedom.org/
http://www.nosoftwarepatents.com/
http://www.linux.codehelp.co.uk/


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel

attachment0 (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Attention to bug 170444

Derek Atkins
In reply to this post by Thomas Bushnell BSG
Okay, I've got a modified version of this patch in my 1.8 tree and
I'll commit to CVS shortly.

-derek

Thomas Bushnell BSG <[hidden email]> writes:

> Thomas Bushnell BSG <[hidden email]> writes:
>
>> Indeed, the following program generates a segv with glibc:
>>
>>
>> #include <time.h>
>> #include <stdio.h>
>>
>> main ()
>> {
>>   char buffer[1000];
>>   struct tm tm_str;
>>
>>   tm_str.tm_mday = 28;
>>   tm_str.tm_mon = 7;
>>   tm_str.tm_year = 105;
>>   tm_str.tm_hour = 0;
>>   tm_str.tm_min = 0;
>>   tm_str.tm_sec = 0;
>>   tm_str.tm_isdst = -1;
>>    
>>   strftime (buffer, 1000, "%A, %d %B, %Y", &tm_str);
>>   printf ("%s\n", buffer);
>> }
>
> Sorry for all the email.  Turns out, not a glibc bug.
>
> The problem here is that tm_wday is not initialized, and some locales
> print the weekday as part of the default date format.  Whew.
>
> So the correct fix is to use mktime to generate the day of week data,
> with the enclosed patch.
>
> Please also put on a todo list somewhere to fix all the uses of
> strftime with locale-dependent format strings, first to deal with
> return values of zero, and second to use mktime to fill in the
> complete tm structure.
>
> --- gnucash-1.8.10.orig/src/engine/date.c
> +++ gnucash-1.8.10/src/engine/date.c
> @@ -362,19 +362,25 @@
>        sprintf (buff, "%2d.%2d.%-4d", day, month, year);
>        break;
>      case DATE_FORMAT_ISO:
> +  iso_date:
>        sprintf (buff, "%04d-%02d-%02d", year, month, day);
>        break;
>      case DATE_FORMAT_LOCALE:
>        {
>          struct tm tm_str;
> + time_t t;
> + int n;
>  
>          tm_str.tm_mday = day;
>          tm_str.tm_mon = month - 1;    /* tm_mon = 0 through 11 */
>          tm_str.tm_year = year - 1900; /* this is what the standard
>                                         * says, it's not a Y2K thing */
> -
>   gnc_tm_set_day_start (&tm_str);
> -        strftime (buff, MAX_DATE_LENGTH, GNC_D_FMT, &tm_str);
> + t = mktime (&tm_str);
> + localtime_r (&t, &tm_str);
> +        n = strftime (buff, MAX_DATE_LENGTH, GNC_D_FMT, &tm_str);
> + if (n == 0)
> +  goto iso_date;
>        }
>        break;
>  
> _______________________________________________
> gnucash-devel mailing list
> [hidden email]
> https://lists.gnucash.org/mailman/listinfo/gnucash-devel
>
>

--
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       [hidden email]                        PGP key available
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: Attention to bug 170444

Thomas Bushnell BSG
In reply to this post by Neil Williams-2
Neil Williams <[hidden email]> writes:

> I've no problem making it 40 - do some formats produce strings
> between 31 and 40?

The solution to buffer overrun bugs is not to make the buffer bigger.

Repeat three times, lather, rinse, call the doctor in the morning, and
make the code able to deal no matter how big the locale has set it.

Thomas
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel