[PATCH] Remove unnecessary locking when reading iconv configuration [BZ #22062]

[Arjun Shankar <arjun dot is at lostca dot se>]

In iconv/gconv_conf.c, __gconv_get_path unnecessarily obtains a lock when
populating the array pointed to by __gconv_path_elem. The locking is not
necessary because all calls to __gconv_read_conf (which in turn calls
__gconv_get_path) are serialized using __libc_once.

This patch:
- removes all locking in __gconv_get_path;
- replaces all explicitly serialized __gconv_read_conf calls with calls to
  __gconv_load_conf, a new wrapper that is serialized internally;
- adds a new test, iconv/tst-iconv_mt.c, to exercise iconv initialization,
  usage, and cleanup in a multi-threaded program;
- indents __gconv_get_path correctly, removing tab characters (which makes
  the patch look a little bigger than it really is).

ChangeLog:

2017-10-26  Arjun Shankar  <arjun@redhat.com>

	[BZ #22062]
	* iconv/gconv_conf.c (__gconv_get_path): Remove locking and fix
	indentation.
	* (__gconv_read_conf): Mark function static.
	* (once): New static variable.
	* (__gconv_load_conf): New function.
	* iconv/gconv_int.h (__gconv_load_conf): Likewise.
	* iconv/gconv_db.c (once): Remove static variable.
	* (__gconv_compare_alias): Use __gconv_load_conf instead of
	__gconv_read_conf.
	* (__gconv_find_transform): Likewise.
	* iconv/tst-iconv_mt.c: New test.
	* iconv/Makefile: Add tst-iconv_mt.
---
This patch takes the direction suggested during review of a previous submission:
https://sourceware.org/ml/libc-alpha/2017-10/msg00428.html
Given that the previous patch was based on an incorrect view, this patch isn't a
v3 but a re-work. After removing the unnecessary locking, I have confirmed
that the test case fails if the relevant __libc_once is removed. I have also
confirmed using gdb's all-stop mode that in this case, the assert I have added
to __gconv_get_path fails. This gives me high confidence that the test will
guard against regressions relating to iconv and multi-threading.

 iconv/Makefile       |   4 +-
 iconv/gconv_conf.c   | 207 ++++++++++++++++++++++++++-------------------------
 iconv/gconv_db.c     |   8 +-
 iconv/gconv_int.h    |   4 +-
 iconv/tst-iconv_mt.c | 146 ++++++++++++++++++++++++++++++++++++
 5 files changed, 260 insertions(+), 109 deletions(-)
 create mode 100644 iconv/tst-iconv_mt.c

diff --git a/iconv/Makefile b/iconv/Makefile
index d340565..9d84c1d 100644
--- a/iconv/Makefile
+++ b/iconv/Makefile
@@ -43,7 +43,7 @@ CFLAGS-charmap.c = -DCHARMAP_PATH='"$(i18ndir)/charmaps"' \
 CFLAGS-linereader.c = -DNO_TRANSLITERATION
 CFLAGS-simple-hash.c = -I../locale
 
-tests	= tst-iconv1 tst-iconv2 tst-iconv3 tst-iconv4 tst-iconv5 tst-iconv6
+tests	= tst-iconv1 tst-iconv2 tst-iconv3 tst-iconv4 tst-iconv5 tst-iconv6 tst-iconv_mt
 
 others		= iconv_prog iconvconfig
 install-others-programs	= $(inst_bindir)/iconv
@@ -67,6 +67,8 @@ endif
 $(objpfx)gconv-modules: test-gconv-modules
 	cp $< $@
 
+$(objpfx)tst-iconv_mt: $(shared-thread-library)
+
 ifeq (yes,$(build-shared))
 tests += tst-gconv-init-failure
 modules-names += tst-gconv-init-failure-mod
diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c
index f1c28ce..d5cf86e 100644
--- a/iconv/gconv_conf.c
+++ b/iconv/gconv_conf.c
@@ -423,115 +423,107 @@ read_conf_file (const char *filename, const char *directory, size_t dir_len,
 void
 __gconv_get_path (void)
 {
-  struct path_elem *result;
-  __libc_lock_define_initialized (static, lock);
-
-  __libc_lock_lock (lock);
-
-  /* Make sure there wasn't a second thread doing it already.  */
-  result = (struct path_elem *) __gconv_path_elem;
-  if (result == NULL)
+  /* Make sure we haven't done it already.  */
+  assert (__gconv_path_elem == NULL);
+
+  /* Determine the complete path first.  */
+  char *gconv_path;
+  size_t gconv_path_len;
+  char *elem;
+  char *oldp;
+  char *cp;
+  int nelems;
+  char *cwd;
+  size_t cwdlen;
+
+  if (__gconv_path_envvar == NULL)
     {
-      /* Determine the complete path first.  */
-      char *gconv_path;
-      size_t gconv_path_len;
-      char *elem;
-      char *oldp;
-      char *cp;
-      int nelems;
-      char *cwd;
-      size_t cwdlen;
-
-      if (__gconv_path_envvar == NULL)
-	{
-	  /* No user-defined path.  Make a modifiable copy of the
-	     default path.  */
-	  gconv_path = strdupa (default_gconv_path);
-	  gconv_path_len = sizeof (default_gconv_path);
-	  cwd = NULL;
-	  cwdlen = 0;
-	}
-      else
-	{
-	  /* Append the default path to the user-defined path.  */
-	  size_t user_len = strlen (__gconv_path_envvar);
-
-	  gconv_path_len = user_len + 1 + sizeof (default_gconv_path);
-	  gconv_path = alloca (gconv_path_len);
-	  __mempcpy (__mempcpy (__mempcpy (gconv_path, __gconv_path_envvar,
-					   user_len),
-				":", 1),
-		     default_gconv_path, sizeof (default_gconv_path));
-	  cwd = __getcwd (NULL, 0);
-	  cwdlen = __glibc_unlikely (cwd == NULL) ? 0 : strlen (cwd);
-	}
-      assert (default_gconv_path[0] == '/');
-
-      /* In a first pass we calculate the number of elements.  */
-      oldp = NULL;
-      cp = strchr (gconv_path, ':');
-      nelems = 1;
-      while (cp != NULL)
-	{
-	  if (cp != oldp + 1)
-	    ++nelems;
-	  oldp = cp;
-	  cp =  strchr (cp + 1, ':');
-	}
-
-      /* Allocate the memory for the result.  */
-      result = (struct path_elem *) malloc ((nelems + 1)
-					    * sizeof (struct path_elem)
-					    + gconv_path_len + nelems
-					    + (nelems - 1) * (cwdlen + 1));
-      if (result != NULL)
-	{
-	  char *strspace = (char *) &result[nelems + 1];
-	  int n = 0;
-
-	  /* Separate the individual parts.  */
-	  __gconv_max_path_elem_len = 0;
-	  elem = __strtok_r (gconv_path, ":", &gconv_path);
-	  assert (elem != NULL);
-	  do
-	    {
-	      result[n].name = strspace;
-	      if (elem[0] != '/')
-		{
-		  assert (cwd != NULL);
-		  strspace = __mempcpy (strspace, cwd, cwdlen);
-		  *strspace++ = '/';
-		}
-	      strspace = __stpcpy (strspace, elem);
-	      if (strspace[-1] != '/')
-		*strspace++ = '/';
-
-	      result[n].len = strspace - result[n].name;
-	      if (result[n].len > __gconv_max_path_elem_len)
-		__gconv_max_path_elem_len = result[n].len;
-
-	      *strspace++ = '\0';
-	      ++n;
-	    }
-	  while ((elem = __strtok_r (NULL, ":", &gconv_path)) != NULL);
-
-	  result[n].name = NULL;
-	  result[n].len = 0;
-	}
+      /* No user-defined path.  Make a modifiable copy of the
+         default path.  */
+      gconv_path = strdupa (default_gconv_path);
+      gconv_path_len = sizeof (default_gconv_path);
+      cwd = NULL;
+      cwdlen = 0;
+    }
+  else
+    {
+      /* Append the default path to the user-defined path.  */
+      size_t user_len = strlen (__gconv_path_envvar);
+
+      gconv_path_len = user_len + 1 + sizeof (default_gconv_path);
+      gconv_path = alloca (gconv_path_len);
+      __mempcpy (__mempcpy (__mempcpy (gconv_path, __gconv_path_envvar,
+                                       user_len),
+                            ":", 1),
+                 default_gconv_path, sizeof (default_gconv_path));
+      cwd = __getcwd (NULL, 0);
+      cwdlen = __glibc_unlikely (cwd == NULL) ? 0 : strlen (cwd);
+    }
+  assert (default_gconv_path[0] == '/');
 
-      __gconv_path_elem = result ?: (struct path_elem *) &empty_path_elem;
+  /* In a first pass we calculate the number of elements.  */
+  oldp = NULL;
+  cp = strchr (gconv_path, ':');
+  nelems = 1;
+  while (cp != NULL)
+    {
+      if (cp != oldp + 1)
+        ++nelems;
+      oldp = cp;
+      cp = strchr (cp + 1, ':');
+    }
 
-      free (cwd);
+  /* Allocate the memory for the result.  */
+  __gconv_path_elem = malloc ((nelems + 1)
+                              * sizeof (struct path_elem)
+                              + gconv_path_len + nelems
+                              + (nelems - 1) * (cwdlen + 1));
+  if (__gconv_path_elem != NULL)
+    {
+      char *strspace = (char *) &__gconv_path_elem[nelems + 1];
+      int n = 0;
+
+      /* Separate the individual parts.  */
+      __gconv_max_path_elem_len = 0;
+      elem = __strtok_r (gconv_path, ":", &gconv_path);
+      assert (elem != NULL);
+      do
+        {
+          __gconv_path_elem[n].name = strspace;
+          if (elem[0] != '/')
+            {
+              assert (cwd != NULL);
+              strspace = __mempcpy (strspace, cwd, cwdlen);
+              *strspace++ = '/';
+            }
+          strspace = __stpcpy (strspace, elem);
+          if (strspace[-1] != '/')
+            *strspace++ = '/';
+
+          __gconv_path_elem[n].len = strspace - __gconv_path_elem[n].name;
+          if (__gconv_path_elem[n].len > __gconv_max_path_elem_len)
+            __gconv_max_path_elem_len = __gconv_path_elem[n].len;
+
+          *strspace++ = '\0';
+          ++n;
+        }
+      while ((elem = __strtok_r (NULL, ":", &gconv_path)) != NULL);
+
+      __gconv_path_elem[n].name = NULL;
+      __gconv_path_elem[n].len = 0;
     }
 
-  __libc_lock_unlock (lock);
+  if (__gconv_path_elem == NULL)
+    __gconv_path_elem = (struct path_elem *) &empty_path_elem;
+
+  free (cwd);
 }
 
 
 /* Read all configuration files found in the user-specified and the default
-   path.  */
-void
-attribute_hidden
+   path.  This function is called only once during the program's lifetime so
+   we may disregard locking and synchronization.  */
+static void
 __gconv_read_conf (void)
 {
   void *modules = NULL;
@@ -602,6 +594,21 @@ __gconv_read_conf (void)
 }
 
 
+/* This "once" variable is used to do a one-time load of the configuration.  */
+__libc_once_define (static, once);
+
+
+/* Read all configuration files found in the user-specified and the default
+   path, but do it only "once" using __gconv_read_conf to do the actual
+   work.  This is the function that must be called when reading iconv
+   configuration.  */
+void
+attribute_hidden
+__gconv_load_conf (void)
+{
+  __libc_once (once, __gconv_read_conf);
+}
+
 
 /* Free all resources if necessary.  */
 libc_freeres_fn (free_mem)
diff --git a/iconv/gconv_db.c b/iconv/gconv_db.c
index 8fcb3cd..2291510 100644
--- a/iconv/gconv_db.c
+++ b/iconv/gconv_db.c
@@ -687,10 +687,6 @@ find_derivation (const char *toset, const char *toset_expand,
 }
 
 
-/* Control of initialization.  */
-__libc_once_define (static, once);
-
-
 static const char *
 do_lookup_alias (const char *name)
 {
@@ -709,7 +705,7 @@ __gconv_compare_alias (const char *name1, const char *name2)
   int result;
 
   /* Ensure that the configuration data is read.  */
-  __libc_once (once, __gconv_read_conf);
+  __gconv_load_conf ();
 
   if (__gconv_compare_alias_cache (name1, name2, &result) != 0)
     result = strcmp (do_lookup_alias (name1) ?: name1,
@@ -729,7 +725,7 @@ __gconv_find_transform (const char *toset, const char *fromset,
   int result;
 
   /* Ensure that the configuration data is read.  */
-  __libc_once (once, __gconv_read_conf);
+  __gconv_load_conf ();
 
   /* Acquire the lock.  */
   __libc_lock_lock (__gconv_lock);
diff --git a/iconv/gconv_int.h b/iconv/gconv_int.h
index 2afd12a..e56757a 100644
--- a/iconv/gconv_int.h
+++ b/iconv/gconv_int.h
@@ -196,8 +196,8 @@ extern int __gconv_compare_alias_cache (const char *name1, const char *name2,
 extern void __gconv_release_step (struct __gconv_step *step)
      attribute_hidden;
 
-/* Read all the configuration data and cache it.  */
-extern void __gconv_read_conf (void) attribute_hidden;
+/* Read all the configuration data and cache it if not done so already.  */
+extern void __gconv_load_conf (void) attribute_hidden;
 
 /* Try to read module cache file.  */
 extern int __gconv_load_cache (void) attribute_hidden;
diff --git a/iconv/tst-iconv_mt.c b/iconv/tst-iconv_mt.c
new file mode 100644
index 0000000..5fc1de4
--- /dev/null
+++ b/iconv/tst-iconv_mt.c
@@ -0,0 +1,146 @@
+/* Test that iconv works in a multi-threaded program.
+   Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* This test runs several worker threads that perform the following three
+   steps in staggered synchronization with each other:
+   1. initialization (iconv_open)
+   2. conversion (iconv)
+   3. cleanup (iconv_close)
+   Having several threads synchronously (using pthread_barrier_*) perform
+   these routines exercises iconv code that handles concurrent attempts to
+   initialize and/or read global iconv state (namely configuration).  */
+
+#include <iconv.h>
+#include <stdio.h>
+#include <string.h>
+
+#include <support/support.h>
+#include <support/xthread.h>
+
+#define TCOUNT 16
+#define CONV_INPUT "Hello, iconv!"
+
+
+#define WORKER_FAIL(fmt) \
+  do { printf ("FAIL: thread %lx: " fmt ": %m\n", tidx); \
+       pthread_exit ((void *) (long int) 1); } while (0)
+
+
+pthread_barrier_t sync;
+pthread_barrier_t sync_half_pool;
+
+
+/* Execute iconv_open, iconv and iconv_close in a synchronized way in
+   conjunction with other sibling worker threads.  If any step fails, print
+   an error to stdout and return NULL to the main thread to indicate the
+   error.  */
+static void *
+worker (void * arg)
+{
+  long int tidx = (long int) arg;
+
+  iconv_t cd;
+
+  char ascii[] = CONV_INPUT;
+  char *inbufpos = ascii;
+  size_t inbytesleft = sizeof (CONV_INPUT);
+
+  char *utf8 = xcalloc (sizeof (CONV_INPUT), 1);
+  char *outbufpos = utf8;
+  size_t outbytesleft = sizeof (CONV_INPUT);
+
+  if (tidx < TCOUNT/2)
+    /* The first half of the worker thread pool synchronize together here,
+       then call iconv_open immediately after.  */
+    xpthread_barrier_wait (&sync_half_pool);
+  else
+    /* The second half wait for the first half to finish iconv_open and
+       continue to the next barrier (before the call to iconv below).  */
+    xpthread_barrier_wait (&sync);
+
+  /* The above block of code staggers all subsequent pthread_barrier_wait
+     calls in a way that ensures a high chance of encountering these
+     combinations of concurrent iconv usage:
+     1) concurrent calls to iconv_open,
+     2) concurrent calls to iconv_open *and* iconv,
+     3) concurrent calls to iconv,
+     4) concurrent calls to iconv *and* iconv_close,
+     5) concurrent calls to iconv_close.  */
+
+  cd = iconv_open ("UTF8", "ASCII");
+  if (cd == (iconv_t) -1)
+    WORKER_FAIL ("iconv_open");
+
+  xpthread_barrier_wait (&sync);
+
+  if (iconv (cd, &inbufpos, &inbytesleft, &outbufpos, &outbytesleft)
+      == (size_t) -1)
+    WORKER_FAIL ("iconv");
+  else
+    *outbufpos = '\0';
+
+  xpthread_barrier_wait (&sync);
+
+  if (iconv_close (cd))
+    WORKER_FAIL ("iconv_close");
+
+  /* The next conditional barrier wait is needed because we staggered the
+     threads into two groups in the beginning and at this point, the second
+     half of worker threads are waiting for the first half to finish
+     iconv_close before they can executing the same:  */
+  if (tidx < TCOUNT/2)
+    xpthread_barrier_wait (&sync);
+
+  if (strncmp (utf8, CONV_INPUT, sizeof CONV_INPUT))
+    {
+      printf ("FAIL: thread %lx: invalid conversion output from iconv\n", tidx);
+      pthread_exit ((void *) (long int) 1);
+    }
+
+  pthread_exit (NULL);
+}
+
+
+static int
+do_test (void)
+{
+  pthread_t thread[TCOUNT];
+  void * worker_output;
+  int retval = 0;
+  int i;
+
+  xpthread_barrier_init (&sync, NULL, TCOUNT);
+  xpthread_barrier_init (&sync_half_pool, NULL, TCOUNT/2);
+
+  for (i = 0; i < TCOUNT; i++)
+    thread[i] = xpthread_create (NULL, worker, (void *) (long int) i);
+
+  for (i = 0; i < TCOUNT; i++)
+    {
+      worker_output = xpthread_join (thread[i]);
+      if (worker_output != NULL)
+        retval = 1;
+    }
+
+  xpthread_barrier_destroy (&sync);
+  xpthread_barrier_destroy (&sync_half_pool);
+
+  return retval;
+}
+
+#include <support/test-driver.c>
-- 
2.9.4