* 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).