Fix double-checked locking in __gconv_get_path and __gconv_read_conf [BZ #22062]
diff mbox series

Message ID 20170920130836.GA3716@aloka.lostca.se
State New
Headers show
Series
  • Fix double-checked locking in __gconv_get_path and __gconv_read_conf [BZ #22062]
Related show

Commit Message

Arjun Shankar Sept. 20, 2017, 1:08 p.m. UTC
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(-)

Comments

Florian Weimer Sept. 27, 2017, 11:08 a.m. UTC | #1
On 09/20/2017 03:08 PM, Arjun Shankar wrote:
> 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.

Looks good overall.  I only have a few minor nits.

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

GNU style says to refer functions by name without trailing “()”.  See 
<https://www.gnu.org/prep/standards/html_node/GNU-Manuals.html>.

Please double-check that you use two spaces after a full stop at the end 
of each sentence, not just before the */.

> +  /* 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);
> +    }

You can use relaxed MO for the second load  because we acquired the lock 
in __gconv_get_path, so in both cases (initialization by this thread and 
another thread), we are already fully synchronized on the lock.

Thanks,
Florian

Patch
diff mbox series

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);
 }