[PATCH] Fix double-checked locking in __gconv_get_path and __gconv_read_conf [BZ #22062]

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

These two functions running in different threads could encounter a data race
if the CPU or compiler reordered the store of __gconv_path_elem so as to be
earlier than writes to the content that it points to. This has now been
corrected by using atomics every time the variable is accessed.

ChangeLog:

2017-09-20  Arjun Shankar  <arjun@redhat.com>

	* iconv/gconv_conf.c: Include <atomic.h>.
	(__gconv_get_path): Access __gconv_path_elem using atomics.
	(__gconv_read_conf): Likewise.
	(libc_freeres_fn): Likewise.
---
 iconv/gconv_conf.c | 42 ++++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c
index f1c28ce..8a5fa28 100644
--- a/iconv/gconv_conf.c
+++ b/iconv/gconv_conf.c
@@ -30,6 +30,7 @@
 #include <string.h>
 #include <unistd.h>
 #include <sys/param.h>
+#include <atomic.h>
 
 #include <libc-lock.h>
 #include <gconv_int.h>
@@ -428,8 +429,13 @@ __gconv_get_path (void)
 
   __libc_lock_lock (lock);
 
-  /* Make sure there wasn't a second thread doing it already.  */
-  result = (struct path_elem *) __gconv_path_elem;
+  /* __gconv_read_conf () uses double-checked locking and therefore can make
+     a redundant call to this function while another thread is already
+     executing it. So first we make sure another thread has not already done
+     the work we want to do.
+
+     A relaxed MO load is sufficient since we already have the lock.  */
+  result = atomic_load_relaxed (&__gconv_path_elem);
   if (result == NULL)
     {
       /* Determine the complete path first.  */
@@ -519,7 +525,10 @@ __gconv_get_path (void)
 	  result[n].len = 0;
 	}
 
-      __gconv_path_elem = result ?: (struct path_elem *) &empty_path_elem;
+      /* This store synchronizes with the acquire MO load in
+         __gconv_read_conf ().  */
+      atomic_store_release (&__gconv_path_elem,
+                            result ?: (struct path_elem *) &empty_path_elem);
 
       free (cwd);
     }
@@ -538,6 +547,7 @@ __gconv_read_conf (void)
   size_t nmodules = 0;
   int save_errno = errno;
   size_t cnt;
+  struct path_elem *gconv_path_elem_local;
 
   /* First see whether we should use the cache.  */
   if (__gconv_load_cache () == 0)
@@ -549,13 +559,20 @@ __gconv_read_conf (void)
 
 #ifndef STATIC_GCONV
   /* Find out where we have to look.  */
-  if (__gconv_path_elem == NULL)
-    __gconv_get_path ();
 
-  for (cnt = 0; __gconv_path_elem[cnt].name != NULL; ++cnt)
+  /* This load is synchronized with the release MO store done during the
+     initialization of __gconv_path_elem in __gconv_get_path ().  */
+  gconv_path_elem_local = atomic_load_acquire (&__gconv_path_elem);
+  if (gconv_path_elem_local == NULL)
+    {
+      __gconv_get_path ();
+      gconv_path_elem_local = atomic_load_acquire (&__gconv_path_elem);
+    }
+
+  for (cnt = 0; gconv_path_elem_local[cnt].name != NULL; ++cnt)
     {
-      const char *elem = __gconv_path_elem[cnt].name;
-      size_t elem_len = __gconv_path_elem[cnt].len;
+      const char *elem = gconv_path_elem_local[cnt].name;
+      size_t elem_len = gconv_path_elem_local[cnt].len;
       char *filename;
 
       /* No slash needs to be inserted between elem and gconv_conf_filename;
@@ -606,6 +623,11 @@ __gconv_read_conf (void)
 /* Free all resources if necessary.  */
 libc_freeres_fn (free_mem)
 {
-  if (__gconv_path_elem != NULL && __gconv_path_elem != &empty_path_elem)
-    free ((void *) __gconv_path_elem);
+  /* __gconv_path_elem is always accessed atomically because it is used in
+     double-checked locking. Since this function is called when the process
+     has become single-threaded, it is enough to used a relaxed MO load.  */
+  struct path_elem *gconv_path_elem_local
+    = atomic_load_relaxed (&__gconv_path_elem);
+  if (gconv_path_elem_local != NULL && gconv_path_elem_local != &empty_path_elem)
+    free ((void *) gconv_path_elem_local);
 }
-- 
2.9.4