This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
[PATCH v2] Fix double-checked locking in __gconv_get_path and __gconv_read_conf [BZ #22062]
- From: Arjun Shankar <arjun dot is at lostca dot se>
- To: libc-alpha at sourceware dot org
- Cc: Florian Weimer <fweimer at redhat dot com>
- Date: Mon, 2 Oct 2017 14:31:48 +0000
- Subject: [PATCH v2] Fix double-checked locking in __gconv_get_path and __gconv_read_conf [BZ #22062]
- Authentication-results: sourceware.org; auth=none
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-10-02 Arjun Shankar <arjun@redhat.com>
[BZ #22062]
* iconv/gconv_conf.c: Include <atomic.h>.
(__gconv_get_path): Access __gconv_path_elem using atomics.
(__gconv_read_conf): Likewise.
(libc_freeres_fn): Likewise.
---
Discussion on PATCH v1:
https://sourceware.org/ml/libc-alpha/2017-09/msg00779.html
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..58fd28e 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_relaxed (&__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