* A minor fix in nl-langinfo
@ 2014-08-09 14:19 Eli Zaretskii
2014-08-10 14:22 ` Eli Zaretskii
0 siblings, 1 reply; 6+ messages in thread
From: Eli Zaretskii @ 2014-08-09 14:19 UTC (permalink / raw)
To: guile-devel
This was in my sources for some time, but I somehow failed to send it.
The problem is that nl_langinfo can return pointers to static buffers
that are overwritten on subsequent calls. So we need to usher the
value away before the next call.
--- libguile/i18n.c~0 2014-08-08 17:05:57.262034100 +0300
+++ libguile/i18n.c 2014-08-09 13:04:19.901125000 +0300
@@ -1522,6 +1522,8 @@ SCM_DEFINE (scm_nl_langinfo, "nl-langinf
else
{
c_result = nl_langinfo (c_item);
+ if (c_result != NULL)
+ c_result = strdup (c_result);
codeset = nl_langinfo (CODESET);
restore_locale_settings (&lsec_prev_locale);
@@ -1532,12 +1534,11 @@ SCM_DEFINE (scm_nl_langinfo, "nl-langinf
else
{
c_result = nl_langinfo (c_item);
+ if (c_result != NULL)
+ c_result = strdup (c_result);
codeset = nl_langinfo (CODESET);
}
- if (c_result != NULL)
- c_result = strdup (c_result);
-
unlock_locale_mutex ();
if (c_result == NULL)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: A minor fix in nl-langinfo 2014-08-09 14:19 A minor fix in nl-langinfo Eli Zaretskii @ 2014-08-10 14:22 ` Eli Zaretskii 2014-08-12 20:15 ` Ludovic Courtès 0 siblings, 1 reply; 6+ messages in thread From: Eli Zaretskii @ 2014-08-10 14:22 UTC (permalink / raw) To: guile-devel > Date: Sat, 09 Aug 2014 17:19:03 +0300 > From: Eli Zaretskii <eliz@gnu.org> > > This was in my sources for some time, but I somehow failed to send it. > > The problem is that nl_langinfo can return pointers to static buffers > that are overwritten on subsequent calls. So we need to usher the > value away before the next call. > > --- libguile/i18n.c~0 2014-08-08 17:05:57.262034100 +0300 > +++ libguile/i18n.c 2014-08-09 13:04:19.901125000 +0300 Sorry, that missed one more instance. Please use this patch instead. --- libguile/i18n.c~0 2014-08-08 17:05:57.262034100 +0300 +++ libguile/i18n.c 2014-08-10 17:20:52.073000000 +0300 @@ -1497,6 +1497,8 @@ SCM_DEFINE (scm_nl_langinfo, "nl-langinf { #ifdef USE_GNU_LOCALE_API c_result = nl_langinfo_l (c_item, c_locale); + if (c_result != NULL) + c_result = strdup (c_result); codeset = nl_langinfo_l (CODESET, c_locale); #else /* !USE_GNU_LOCALE_API */ /* We can't use `RUN_IN_LOCALE_SECTION ()' here because the locale @@ -1522,6 +1524,8 @@ SCM_DEFINE (scm_nl_langinfo, "nl-langinf else { c_result = nl_langinfo (c_item); + if (c_result != NULL) + c_result = strdup (c_result); codeset = nl_langinfo (CODESET); restore_locale_settings (&lsec_prev_locale); @@ -1532,12 +1536,11 @@ SCM_DEFINE (scm_nl_langinfo, "nl-langinf else { c_result = nl_langinfo (c_item); + if (c_result != NULL) + c_result = strdup (c_result); codeset = nl_langinfo (CODESET); } - if (c_result != NULL) - c_result = strdup (c_result); - unlock_locale_mutex (); if (c_result == NULL) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: A minor fix in nl-langinfo 2014-08-10 14:22 ` Eli Zaretskii @ 2014-08-12 20:15 ` Ludovic Courtès 2014-08-12 21:34 ` Mark H Weaver 0 siblings, 1 reply; 6+ messages in thread From: Ludovic Courtès @ 2014-08-12 20:15 UTC (permalink / raw) To: Eli Zaretskii; +Cc: guile-devel Eli Zaretskii <eliz@gnu.org> skribis: > Sorry, that missed one more instance. Please use this patch instead. > > --- libguile/i18n.c~0 2014-08-08 17:05:57.262034100 +0300 > +++ libguile/i18n.c 2014-08-10 17:20:52.073000000 +0300 > @@ -1497,6 +1497,8 @@ SCM_DEFINE (scm_nl_langinfo, "nl-langinf > { > #ifdef USE_GNU_LOCALE_API > c_result = nl_langinfo_l (c_item, c_locale); > + if (c_result != NULL) > + c_result = strdup (c_result); > codeset = nl_langinfo_l (CODESET, c_locale); > #else /* !USE_GNU_LOCALE_API */ > /* We can't use `RUN_IN_LOCALE_SECTION ()' here because the locale > @@ -1522,6 +1524,8 @@ SCM_DEFINE (scm_nl_langinfo, "nl-langinf > else > { > c_result = nl_langinfo (c_item); > + if (c_result != NULL) > + c_result = strdup (c_result); > codeset = nl_langinfo (CODESET); > > restore_locale_settings (&lsec_prev_locale); > @@ -1532,12 +1536,11 @@ SCM_DEFINE (scm_nl_langinfo, "nl-langinf > else > { > c_result = nl_langinfo (c_item); > + if (c_result != NULL) > + c_result = strdup (c_result); > codeset = nl_langinfo (CODESET); > } > > - if (c_result != NULL) > - c_result = strdup (c_result); > - > unlock_locale_mutex (); > > if (c_result == NULL) Makes sense, please push. Thanks, Ludo’. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: A minor fix in nl-langinfo 2014-08-12 20:15 ` Ludovic Courtès @ 2014-08-12 21:34 ` Mark H Weaver 2014-08-13 17:22 ` Mark H Weaver 0 siblings, 1 reply; 6+ messages in thread From: Mark H Weaver @ 2014-08-12 21:34 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guile-devel ludo@gnu.org (Ludovic Courtès) writes: > Eli Zaretskii <eliz@gnu.org> skribis: > >> Sorry, that missed one more instance. Please use this patch instead. >> >> --- libguile/i18n.c~0 2014-08-08 17:05:57.262034100 +0300 >> +++ libguile/i18n.c 2014-08-10 17:20:52.073000000 +0300 >> @@ -1497,6 +1497,8 @@ SCM_DEFINE (scm_nl_langinfo, "nl-langinf >> { >> #ifdef USE_GNU_LOCALE_API >> c_result = nl_langinfo_l (c_item, c_locale); >> + if (c_result != NULL) >> + c_result = strdup (c_result); >> codeset = nl_langinfo_l (CODESET, c_locale); >> #else /* !USE_GNU_LOCALE_API */ >> /* We can't use `RUN_IN_LOCALE_SECTION ()' here because the locale >> @@ -1522,6 +1524,8 @@ SCM_DEFINE (scm_nl_langinfo, "nl-langinf >> else >> { >> c_result = nl_langinfo (c_item); >> + if (c_result != NULL) >> + c_result = strdup (c_result); >> codeset = nl_langinfo (CODESET); >> >> restore_locale_settings (&lsec_prev_locale); >> @@ -1532,12 +1536,11 @@ SCM_DEFINE (scm_nl_langinfo, "nl-langinf >> else >> { >> c_result = nl_langinfo (c_item); >> + if (c_result != NULL) >> + c_result = strdup (c_result); >> codeset = nl_langinfo (CODESET); >> } >> >> - if (c_result != NULL) >> - c_result = strdup (c_result); >> - >> unlock_locale_mutex (); >> >> if (c_result == NULL) > > Makes sense, please push. Actually, please hold off on this. It doesn't entirely fix the problems, and I'm working on a more complete fix. Thanks, Mark ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: A minor fix in nl-langinfo 2014-08-12 21:34 ` Mark H Weaver @ 2014-08-13 17:22 ` Mark H Weaver 2014-08-13 19:35 ` Ludovic Courtès 0 siblings, 1 reply; 6+ messages in thread From: Mark H Weaver @ 2014-08-13 17:22 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guile-devel Mark H Weaver <mhw@netris.org> writes: > ludo@gnu.org (Ludovic Courtès) writes: > >> Eli Zaretskii <eliz@gnu.org> skribis: >> >>> Sorry, that missed one more instance. Please use this patch instead. >>> >>> --- libguile/i18n.c~0 2014-08-08 17:05:57.262034100 +0300 >>> +++ libguile/i18n.c 2014-08-10 17:20:52.073000000 +0300 >>> @@ -1497,6 +1497,8 @@ SCM_DEFINE (scm_nl_langinfo, "nl-langinf >>> { >>> #ifdef USE_GNU_LOCALE_API >>> c_result = nl_langinfo_l (c_item, c_locale); >>> + if (c_result != NULL) >>> + c_result = strdup (c_result); >>> codeset = nl_langinfo_l (CODESET, c_locale); So, I think we have to copy 'codeset' also, because it is not used until after the locale mutex is released, at which point another thread could call 'nl_langinfo*' and overwrite the static buffer that 'codeset' points to. Here's the patch I had in mind. What do you think? Mark --8<---------------cut here---------------start------------->8--- diff --git a/libguile/i18n.c b/libguile/i18n.c index e38e560..411f38a 100644 --- a/libguile/i18n.c +++ b/libguile/i18n.c @@ -1,4 +1,4 @@ -/* Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011, 2012, 2013 Free Software Foundation, Inc. +/* Copyright (C) 2006-2014 Free Software Foundation, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public License @@ -1465,6 +1465,13 @@ SCM_DEFINE (scm_locale_string_to_inexact, "locale-string->inexact", Note: We don't use Gnulib's `nl_langinfo' module because it's currently not as complete as the compatibility hacks in `i18n.scm'. */ +static char * +copy_string_or_null (char *s) +{ + if (s != NULL) + s = strdup (s); + return s; +} SCM_DEFINE (scm_nl_langinfo, "nl-langinfo", 1, 1, 0, (SCM item, SCM locale), @@ -1496,8 +1503,8 @@ SCM_DEFINE (scm_nl_langinfo, "nl-langinfo", 1, 1, 0, if (c_locale != NULL) { #ifdef USE_GNU_LOCALE_API - c_result = nl_langinfo_l (c_item, c_locale); - codeset = nl_langinfo_l (CODESET, c_locale); + c_result = copy_string_or_null (nl_langinfo_l (c_item, c_locale)); + codeset = copy_string_or_null (nl_langinfo_l (CODESET, c_locale)); #else /* !USE_GNU_LOCALE_API */ /* We can't use `RUN_IN_LOCALE_SECTION ()' here because the locale mutex is already taken. */ @@ -1521,8 +1528,8 @@ SCM_DEFINE (scm_nl_langinfo, "nl-langinfo", 1, 1, 0, scm_locale_error (FUNC_NAME, lsec_err); else { - c_result = nl_langinfo (c_item); - codeset = nl_langinfo (CODESET); + c_result = copy_string_or_null (nl_langinfo (c_item)); + codeset = copy_string_or_null (nl_langinfo (CODESET)); restore_locale_settings (&lsec_prev_locale); free_locale_settings (&lsec_prev_locale); @@ -1531,13 +1538,10 @@ SCM_DEFINE (scm_nl_langinfo, "nl-langinfo", 1, 1, 0, } else { - c_result = nl_langinfo (c_item); - codeset = nl_langinfo (CODESET); + c_result = copy_string_or_null (nl_langinfo (c_item)); + codeset = copy_string_or_null (nl_langinfo (CODESET)); } - if (c_result != NULL) - c_result = strdup (c_result); - unlock_locale_mutex (); if (c_result == NULL) @@ -1669,6 +1673,9 @@ SCM_DEFINE (scm_nl_langinfo, "nl-langinfo", 1, 1, 0, } } + if (codeset != NULL) + free (codeset); + return result; } #undef FUNC_NAME --8<---------------cut here---------------end--------------->8--- ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: A minor fix in nl-langinfo 2014-08-13 17:22 ` Mark H Weaver @ 2014-08-13 19:35 ` Ludovic Courtès 0 siblings, 0 replies; 6+ messages in thread From: Ludovic Courtès @ 2014-08-13 19:35 UTC (permalink / raw) To: Mark H Weaver; +Cc: guile-devel Mark H Weaver <mhw@netris.org> skribis: > Mark H Weaver <mhw@netris.org> writes: > >> ludo@gnu.org (Ludovic Courtès) writes: >> >>> Eli Zaretskii <eliz@gnu.org> skribis: >>> >>>> Sorry, that missed one more instance. Please use this patch instead. >>>> >>>> --- libguile/i18n.c~0 2014-08-08 17:05:57.262034100 +0300 >>>> +++ libguile/i18n.c 2014-08-10 17:20:52.073000000 +0300 >>>> @@ -1497,6 +1497,8 @@ SCM_DEFINE (scm_nl_langinfo, "nl-langinf >>>> { >>>> #ifdef USE_GNU_LOCALE_API >>>> c_result = nl_langinfo_l (c_item, c_locale); >>>> + if (c_result != NULL) >>>> + c_result = strdup (c_result); >>>> codeset = nl_langinfo_l (CODESET, c_locale); > > So, I think we have to copy 'codeset' also, because it is not used until > after the locale mutex is released, at which point another thread could > call 'nl_langinfo*' and overwrite the static buffer that 'codeset' > points to. Indeed. > +static char * > +copy_string_or_null (char *s) const char *s Fine with me! Ludo’. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-13 19:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-08-09 14:19 A minor fix in nl-langinfo Eli Zaretskii 2014-08-10 14:22 ` Eli Zaretskii 2014-08-12 20:15 ` Ludovic Courtès 2014-08-12 21:34 ` Mark H Weaver 2014-08-13 17:22 ` Mark H Weaver 2014-08-13 19:35 ` Ludovic Courtès
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).