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

[Florian Weimer <fweimer at redhat dot com>]

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