Sorry for long delay. I've refreshed the patch.
Changes from v1 are:
- remove unnecessary heap usage in
gl_locale_name_default_from_CoreFoundation
- move pthread_is_threaded_np check from localename.m4 to intlmacosx.m4
- fix close() error handling as Noah pointed (see below)
- handle EINTR for close(), read(), and write()
- document the reason why our code can use async-signal-unsafe functions in
a child process (in gl_locale_name_default_from_CoreFoundation_forked)
- do the test in a forked process so it doesn't affect the default
locale cache
- spell "Core Foundation" instead of "CoreFoundation", according to
Apple's document
Noah Misch <n...@leadboat.com> writes:
>> Perhaps it might require a copyright assignment
>> from the original author (Cc'ed).
>
> No problem. I will send you an off-list message verifying my assignment.
Noah said that he had already contributed the code to PostgreSQL, but
the overlap between the original patch and gnulib's could be considered
trivial. I also think it's OK, because the additional code is just a
usual pipe/fork/waitpid wrapper around the existing code.
>> + if (close (fd[1]) < 0)
>> + {
>> + close (fd[0]);
>> + goto done;
>
> If the function follows this goto, it neglects waitpid().
Thanks; fixed.
> Though orthogonal to this patch, libintl_setlocale() and
> libintl_newlocale() would do better to return 0, not "C", after such
> an internal error.
I'm skeptical that this is the right thing to do. POSIX says:
Upon successful completion, setlocale() shall return the string
associated with the specified category for the new locale. Otherwise,
setlocale() shall return a null pointer and the global locale shall not
be changed.
A null pointer for locale shall cause setlocale() to return a pointer to
the string associated with the specified category for the current global
locale. The global locale shall not be changed.
I read this as: setlocale() returns NULL only when changing the locale
(not querying the locale). And in any case, the default global locale
should be "POSIX" or "C".
Regards,
--
Daiki Ueno
>From 904c08aa514f38d862ad890cbc8529c0fe859543 Mon Sep 17 00:00:00 2001
From: Daiki Ueno <u...@gnu.org>
Date: Tue, 14 Oct 2014 12:47:06 +0900
Subject: [PATCH] localename: avoid thread creation on Mac OS X
Mac OS X Core Foundation framework internally creates a thread,
that might cause trouble if a single-threaded consumer later
forks a child process. Isolate the Core Foundation calls into a
separate process and guarantee that the parent process isn't
affected by the implicit thread creation.
Based on the approach taken by PostgreSQL, proposed by Noah Misch
in:
http://www.postgresql.org/message-id/20141011002433.ga15...@tornado.leadboat.com
Problem reported by Peter Eisentraut in:
http://savannah.gnu.org/bugs/?43404.
* modules/localename-tests (Depends-on): Add waitpid.
* tests/test-localename.c (is_threaded) [__APPLE__]: New variable.
(test_locale_name_default_forked) [__APPLE__]: Check if the
process is single-threaded, before and after calling
gl_locale_name_default.
* m4/intlmacosx.m4: Bump serial number.
(gt_INTL_MACOSX): Check pthread_is_threaded_np.
* lib/localename.c [__APPLE__]: Include <pthread.h>, <signal.h>, and
<unistd.h> if pthread_is_threaded_np is available.
(gl_locale_name_default_from_CoreFoundation) [__APPLE__]: New
function, split off from gl_locale_name_default.
(gl_locale_name_default_from_CoreFoundation_forked) [__APPLE__]: New
function.
(gl_locale_name_default) [__APPLE__]: Call _from_CoreFoundation or
_from_CoreFoundation_forked depending on whether or not the caller is
multi-threaded.
---
ChangeLog | 30 +++++++
lib/localename.c | 203 +++++++++++++++++++++++++++++++++++++++++------
m4/intlmacosx.m4 | 3 +-
modules/localename-tests | 1 +
tests/test-localename.c | 39 ++++++++-
5 files changed, 248 insertions(+), 28 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 69e3425..6aff36b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,33 @@
+2015-01-31 Daiki Ueno <u...@gnu.org>
+
+ localename: avoid thread creation on Mac OS X
+ Mac OS X Core Foundation framework internally creates a thread,
+ that might cause trouble if a single-threaded consumer later
+ forks a child process. Isolate the Core Foundation calls into a
+ separate process and guarantee that the parent process isn't
+ affected by the implicit thread creation.
+ Based on the approach taken by PostgreSQL, proposed by Noah Misch
+ in:
+ http://www.postgresql.org/message-id/20141011002433.ga15...@tornado.leadboat.com
+ Problem reported by Peter Eisentraut in:
+ http://savannah.gnu.org/bugs/?43404.
+ * modules/localename-tests (Depends-on): Add waitpid.
+ * tests/test-localename.c (is_threaded) [__APPLE__]: New variable.
+ (test_locale_name_default_forked) [__APPLE__]: Check if the
+ process is single-threaded, before and after calling
+ gl_locale_name_default.
+ * m4/intlmacosx.m4: Bump serial number.
+ (gt_INTL_MACOSX): Check pthread_is_threaded_np.
+ * lib/localename.c [__APPLE__]: Include <pthread.h>, <signal.h>, and
+ <unistd.h> if pthread_is_threaded_np is available.
+ (gl_locale_name_default_from_CoreFoundation) [__APPLE__]: New
+ function, split off from gl_locale_name_default.
+ (gl_locale_name_default_from_CoreFoundation_forked) [__APPLE__]: New
+ function.
+ (gl_locale_name_default) [__APPLE__]: Call _from_CoreFoundation or
+ _from_CoreFoundation_forked depending on whether or not the caller is
+ multi-threaded.
+
2015-01-29 Pádraig Brady <p...@draigbrady.com>
localename: support Solaris 12 and illumos
diff --git a/lib/localename.c b/lib/localename.c
index c6f326e..08d684e 100644
--- a/lib/localename.c
+++ b/lib/localename.c
@@ -55,6 +55,12 @@ extern char * getlocalename_l(int, locale_t);
# elif HAVE_CFPREFERENCESCOPYAPPVALUE
# include <CoreFoundation/CFPreferences.h>
# endif
+# if HAVE_PTHREAD_IS_THREADED_NP
+# define _DARWIN_C_SOURCE 1
+# include <pthread.h>
+# include <signal.h>
+# include <unistd.h>
+# endif
#endif
#if defined _WIN32 || defined __WIN32__
@@ -2843,6 +2849,160 @@ gl_locale_name_environ (int category, const char *categoryname)
return NULL;
}
+#if HAVE_CFLOCALECOPYCURRENT || HAVE_CFPREFERENCESCOPYAPPVALUE
+static const char *
+gl_locale_name_default_from_CoreFoundation (void)
+{
+ static char namebuf[256];
+ const char *localename = "C";
+# if HAVE_CFLOCALECOPYCURRENT /* Mac OS X 10.3 or newer */
+ CFLocaleRef locale = CFLocaleCopyCurrent ();
+ CFStringRef name = CFLocaleGetIdentifier (locale);
+
+ if (CFStringGetCString (name, namebuf, sizeof (namebuf),
+ kCFStringEncodingASCII))
+ {
+ gl_locale_name_canonicalize (namebuf);
+ localename = namebuf;
+ }
+ CFRelease (locale);
+# elif HAVE_CFPREFERENCESCOPYAPPVALUE /* Mac OS X 10.2 or newer */
+ CFTypeRef value =
+ CFPreferencesCopyAppValue (CFSTR ("AppleLocale"),
+ kCFPreferencesCurrentApplication);
+ if (value != NULL
+ && CFGetTypeID (value) == CFStringGetTypeID ()
+ && CFStringGetCString ((CFStringRef)value,
+ namebuf, sizeof (namebuf),
+ kCFStringEncodingASCII))
+ {
+ gl_locale_name_canonicalize (namebuf);
+ localename = namebuf;
+ }
+# endif
+ return localename;
+}
+
+# if HAVE_PTHREAD_IS_THREADED_NP
+/* EINTR handling for close(), read(), and write().
+ These functions can return -1/EINTR even though we don't have any
+ signal handlers set up, namely when we get interrupted via SIGSTOP. */
+
+static int
+nonintr_close (int fd)
+{
+ int retval;
+
+ do
+ retval = close (fd);
+ while (retval < 0 && errno == EINTR);
+
+ return retval;
+}
+#define close nonintr_close
+
+ssize_t
+nonintr_read (int fd, void *buf, size_t count)
+{
+ ssize_t retval;
+
+ do
+ retval = read (fd, buf, count);
+ while (retval < 0 && errno == EINTR);
+
+ return retval;
+}
+#define read nonintr_read
+
+ssize_t
+nonintr_write (int fd, const void *buf, size_t count)
+{
+ ssize_t retval;
+
+ do
+ retval = write (fd, buf, count);
+ while (retval < 0 && errno == EINTR);
+
+ return retval;
+}
+#undef write /* avoid warning on VMS */
+#define write nonintr_write
+
+static char *
+gl_locale_name_default_from_CoreFoundation_forked (void)
+{
+ static char namebuf[256];
+ const char *localename = "C";
+ int fd[2];
+ sigset_t sigs, old_sigs;
+ pid_t pid;
+ int child_status, close_pipe_stdout_status;
+ ssize_t n;
+
+ /* Block SIGCHLD so we can get an exit status. */
+ sigemptyset (&sigs);
+ sigaddset (&sigs, SIGCHLD);
+ sigprocmask (SIG_BLOCK, &sigs, &old_sigs);
+
+ if (pipe (fd) < 0)
+ goto done;
+
+ pid = fork ();
+ if (pid < 0)
+ {
+ close (fd[0]);
+ close (fd[1]);
+ goto done;
+ }
+
+ if (pid == 0)
+ {
+ /* Although the Core Foundation calls in
+ gl_locale_name_default_from_CoreFoundation are not
+ async-signal-safe, we can safely use it in a child process
+ here, because the caller is guaranteed to be a
+ single-threaded process (checked using pthread_is_threaded_np). */
+ const char *locname = gl_locale_name_default_from_CoreFoundation ();
+ size_t locname_len = strlen (locname);
+
+ if (close (fd[0]) < 0
+ || write (fd[1], locname, locname_len) < locname_len
+ || close (fd[1]) < 0)
+ _exit (EXIT_FAILURE);
+
+ _exit (EXIT_SUCCESS);
+ }
+
+ close_pipe_stdout_status = close (fd[1]);
+ if (waitpid (pid, &child_status, 0) != pid
+ || !(WIFEXITED (child_status)
+ && WEXITSTATUS (child_status) == EXIT_SUCCESS)
+ || close_pipe_stdout_status < 0)
+ {
+ close (fd[0]);
+ goto done;
+ }
+
+ n = read (fd[0], namebuf, sizeof (namebuf));
+ if (n <= 0 || n == sizeof (namebuf))
+ {
+ close (fd[0]);
+ goto done;
+ }
+
+ if (close (fd[0]) < 0)
+ goto done;
+
+ namebuf[n] = '\0';
+ localename = namebuf;
+
+ done:
+ sigprocmask (SIG_SETMASK, &old_sigs, NULL);
+ return strdup (localename);
+}
+# endif
+#endif
+
const char *
gl_locale_name_default (void)
{
@@ -2890,37 +3050,28 @@ gl_locale_name_default (void)
# if HAVE_CFLOCALECOPYCURRENT || HAVE_CFPREFERENCESCOPYAPPVALUE
/* Mac OS X 10.2 or newer */
{
- /* Cache the locale name, since CoreFoundation calls are expensive. */
+ /* Cache the locale name, since Core Foundation calls are expensive. */
static const char *cached_localename;
if (cached_localename == NULL)
{
- char namebuf[256];
-# if HAVE_CFLOCALECOPYCURRENT /* Mac OS X 10.3 or newer */
- CFLocaleRef locale = CFLocaleCopyCurrent ();
- CFStringRef name = CFLocaleGetIdentifier (locale);
-
- if (CFStringGetCString (name, namebuf, sizeof (namebuf),
- kCFStringEncodingASCII))
- {
- gl_locale_name_canonicalize (namebuf);
- cached_localename = strdup (namebuf);
- }
- CFRelease (locale);
-# elif HAVE_CFPREFERENCESCOPYAPPVALUE /* Mac OS X 10.2 or newer */
- CFTypeRef value =
- CFPreferencesCopyAppValue (CFSTR ("AppleLocale"),
- kCFPreferencesCurrentApplication);
- if (value != NULL
- && CFGetTypeID (value) == CFStringGetTypeID ()
- && CFStringGetCString ((CFStringRef)value,
- namebuf, sizeof (namebuf),
- kCFStringEncodingASCII))
- {
- gl_locale_name_canonicalize (namebuf);
- cached_localename = strdup (namebuf);
- }
+# if HAVE_PTHREAD_IS_THREADED_NP
+ /* The Core Foundation calls in
+ gl_locale_name_default_from_CoreFoundation create a new
+ thread, which may cause a problem if a consumer later forks
+ a child process.
+ If the consumer is single-threaded, isolate the
+ Core Foundation calls into a separate process. Based on the
+ approach taken by PostgreSQL, proposed by Noah Misch in:
+ <http://www.postgresql.org/message-id/20141011002433.ga15...@tornado.leadboat.com>. */
+ if (!pthread_is_threaded_np ())
+ cached_localename
+ = gl_locale_name_default_from_CoreFoundation_forked ();
+ else
# endif
+ cached_localename
+ = gl_locale_name_default_from_CoreFoundation ();
+
if (cached_localename == NULL)
cached_localename = "C";
}
diff --git a/m4/intlmacosx.m4 b/m4/intlmacosx.m4
index 0d8d298..a2f3d89 100644
--- a/m4/intlmacosx.m4
+++ b/m4/intlmacosx.m4
@@ -1,4 +1,4 @@
-# intlmacosx.m4 serial 5 (gettext-0.18.2)
+# intlmacosx.m4 serial 6 (gettext-0.18.2)
dnl Copyright (C) 2004-2015 Free Software Foundation, Inc.
dnl This file is free software; the Free Software Foundation
dnl gives unlimited permission to copy and/or distribute it,
@@ -50,6 +50,7 @@ AC_DEFUN([gt_INTL_MACOSX],
fi
INTL_MACOSX_LIBS=
if test $gt_cv_func_CFPreferencesCopyAppValue = yes || test $gt_cv_func_CFLocaleCopyCurrent = yes; then
+ AC_CHECK_FUNCS([pthread_is_threaded_np])
INTL_MACOSX_LIBS="-Wl,-framework -Wl,CoreFoundation"
fi
AC_SUBST([INTL_MACOSX_LIBS])
diff --git a/modules/localename-tests b/modules/localename-tests
index f7633f0..1c68a19 100644
--- a/modules/localename-tests
+++ b/modules/localename-tests
@@ -7,6 +7,7 @@ locale
setenv
unsetenv
strdup
+waitpid
configure.ac:
AC_CHECK_FUNCS_ONCE([newlocale])
diff --git a/tests/test-localename.c b/tests/test-localename.c
index b5dd742..cf1201e 100644
--- a/tests/test-localename.c
+++ b/tests/test-localename.c
@@ -23,6 +23,7 @@
#include <locale.h>
#include <stdlib.h>
#include <string.h>
+#include <sys/wait.h>
#include "macros.h"
@@ -711,8 +712,9 @@ test_locale_name_environ (void)
static void
test_locale_name_default (void)
{
- const char *name = gl_locale_name_default ();
+ const char *name;
+ name = gl_locale_name_default ();
ASSERT (name != NULL);
/* Only Mac OS X and Windows have a facility for the user to set the default
@@ -734,9 +736,44 @@ test_locale_name_default (void)
#endif
}
+static void
+test_locale_name_default_forked (void)
+{
+#if (HAVE_CFLOCALECOPYCURRENT || HAVE_CFPREFERENCESCOPYAPPVALUE) \
+ && HAVE_PTHREAD_IS_THREADED_NP
+ pid_t pid, ret;
+ int status;
+
+ /* Do the test in a forked process, so it does not affect the defaut
+ locale cache. */
+ pid = fork ();
+ ASSERT (pid >= 0);
+
+ if (pid == 0)
+ {
+ /* Check if the Core Foundation calls are properly isolated into a
+ separate process, and the process' multi-threadness doesn't
+ change after gl_locale_name_default. */
+ ASSERT (!pthread_is_threaded_np ());
+ test_locale_name_default ();
+ ASSERT (!pthread_is_threaded_np ());
+ exit (0);
+ }
+
+ ret = waitpid (pid, &status, 0);
+ ASSERT (ret == pid);
+ ASSERT (WIFEXITED (status));
+#endif
+}
+
int
main ()
{
+ /* This test needs to be called first, to ensure that there is no
+ thread created and that the default locale is not cached
+ already. */
+ test_locale_name_default_forked ();
+
test_locale_name ();
test_locale_name_thread ();
test_locale_name_posix ();
--
2.1.3
