Re: [bug #43404] gl_locale_name_default() thread issues on OS X

[Daiki Ueno]

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 <address@hidden> 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 <address@hidden>
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/address@hidden
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  <address@hidden>
+
+       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/address@hidden
+       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  <address@hidden>
 
        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/address@hidden>.  */
+        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