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

  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(-)
  

On 10/02/2017 04:31 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-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.

Looks good.

Thanks,
Florian
  

On 10/02/2017 07:31 AM, 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.

My reviews are structured into 3 pats: high level, design, and
implementation.

(a) At a high level I see a problem with this change.

All of the callers of __gconv_read_conf do so using __libc_once.

Why can't we just delete all the double checked locking assuming that
__libc_once works correctly?

The use of __libc_once ensures that __gconv_read_conf is called only
once, and that takes into account all threads.

The only scenario it doesn't take into account is if an in-progress
call to __libc_once was interrupted by a dlopen of libpthread that
then started threads. Those threads might attempt a parallel
initialization. Though this problem has existed *before* the changes
you are making.

Either way, because of the use of __libc_once, the function need not
do any locking or atomics.

Was the __libc_once usage by the callers considered when the patch
was created?

(b) At the design level, the changes look good.

What you've done is a correct fix of DCLP (thought at a high level
it might not be needed and we might just delete the code and update
the comment to say it must be called with __libc_once).

I see perhaps one case where an atomic operation is not needed, and
I've commented on that below.

(c) The details of the patch are great, and the comments are good.

> 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>

OK.

>  
>  #include <libc-lock.h>
>  #include <gconv_int.h>
> @@ -428,8 +429,13 @@ __gconv_get_path (void)
>  
>    __libc_lock_lock (lock);
>  

All of the caller of __gconv_read_conf do so using __libc_once, which 
already does locking using pthread_once to avoid __gconv_read_conf
ever being called by a another thread?

e.g.
iconv/gconv_db.c:  __libc_once (once, __gconv_read_conf);
iconv/gconv_db.c:  __libc_once (once, __gconv_read_conf);

See my comments above.

I will continue reviewing the DCLP usage below because it has value.

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

OK. Good, this removes the data race between the unordered read by a thread
in __gconv_read_conf and a write by another thread in __gonv_get_path.

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

OK. Good comment.

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

OK.

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

OK. Perfect. If the acquire sees NULL it knows that the other thread might
not have completed the work, and so proceeds to acquire the lock and check
again (double checked locking). If it sees non-NULL it knows the other function
completed all of the work and the element data should be visible.

> +  gconv_path_elem_local = atomic_load_acquire (&__gconv_path_elem);
> +  if (gconv_path_elem_local == NULL)
> +    {
> +      __gconv_get_path ();

Multiple reads by threads are safe. In this case we can have multiple threads
here, but all write to __gconv_path_elem are complete. All we are doing is
having multiple readers, which is not a data race.

Is there any reason we need this atomic load?

> +      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;

OK.

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

OK. We use an atomic load to see the update from a thread that initialized
__gconv_path_elem.

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

On 10/05/2017 10:22 PM, Carlos O'Donell wrote:
> Is there any reason we need this atomic load?

Based on the context, it's not clear to which load you are referring.

Florian
  

On 10/05/2017 03:20 PM, Florian Weimer wrote:
> On 10/05/2017 10:22 PM, Carlos O'Donell wrote:
>> Is there any reason we need this atomic load?
> 
> Based on the context, it's not clear to which load you are referring.
> 
> Florian

My apologies, clipped, to show context:

> +  if (gconv_path_elem_local == NULL)
> +    {
> +      __gconv_get_path ();

Multiple reads by threads are safe. In this case we can have multiple threads
here, but all writes to __gconv_path_elem are complete. All we are doing is
having multiple readers, which is not a data race.

Is there any reason we need this atomic load?

> +      gconv_path_elem_local = atomic_load_relaxed (&__gconv_path_elem);
                                 ^^^^^^^^^^^^^^^^^^^
In the exiting free_mem case I can see the argument for a detached thread
doing the initialization while free_mem is called, and it should be thread
safe to call exit and these functions, avoiding the data race, but perhaps
still crashing for other reasons. We are being consistent in our use of
atomic accesses to provide data race freedom.

But in this case there is no race. Is it just belt-and-suspenders? If so,
we should comment on that.
  

On 10/06/2017 01:05 AM, Carlos O'Donell wrote:
> Is there any reason we need this atomic load?
> 
>> +      gconv_path_elem_local = atomic_load_relaxed (&__gconv_path_elem);
>                                   ^^^^^^^^^^^^^^^^^^^

Oh, this follows just the general rule that if atomics are used, all 
accesses should employ atomics.  This follows from the general principle 
of C11 compatibility, where an atomic type would default to seq-cst 
access (which we don't want here).  I see this was not mentioned in the 
Concurrency wiki page.  I've fixed that.

I did not notice that there was a __libc_once guard.  So it would be 
better to remove the locking from __gconv_get_path and document that 
(including that this serializes initialization of the gconv cache via 
__gconv_load_cache).

Thanks,
Florian
  

On 10/05/2017 11:38 PM, Florian Weimer wrote:
> On 10/06/2017 01:05 AM, Carlos O'Donell wrote:
>> Is there any reason we need this atomic load?
>> 
>>> +      gconv_path_elem_local = atomic_load_relaxed
>>> (&__gconv_path_elem);
>> ^^^^^^^^^^^^^^^^^^^
> 
> Oh, this follows just the general rule that if atomics are used, all
> accesses should employ atomics.  This follows from the general
> principle of C11 compatibility, where an atomic type would default to
> seq-cst access (which we don't want here).  I see this was not
> mentioned in the Concurrency wiki page.  I've fixed that.

Ah! Yes, that makes perfect sense. The assumption is we would switch
the type of the variable to an atomic type, and then the unadorned
accesses have to be seq-cst, which we don't need here.

Thanks for updating the wiki. I'd forgotten entirely about this.

> I did not notice that there was a __libc_once guard.  So it would be
> better to remove the locking from __gconv_get_path and document that
> (including that this serializes initialization of the gconv cache via
> __gconv_load_cache).

Yes, that's right.

Arjun,

I suggest you rework the patch like this:

- Remove locking from __gconv_get_path, and add comments to that function
  access is serialized by __libc_once in the caller.
- Add comment to __gconv_read_conf that it must be serialized by the
  __libc_once in the caller.