Fix double-checked locking in __gconv_get_path and __gconv_read_conf [BZ #22062]
Commit Message
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
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
@@ -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);
}