Re: [PATCH] Fix double-checked locking in _res_hconf_reorder_addrs [BZ #19074]

[Siddhesh Poyarekar <sid at reserved-bit dot com>]

On Tuesday 06 October 2015 06:02 PM, Florian Weimer wrote:
> This addresses a data race in libresolv.  The race is not entirely
> benign, it could cause programs to access a partially initialized
> ifaddrs array.
> 
> I made sure this compiles on x86_64, but this code is difficult to test.
> 
> A potential follow-up optimization would be to move the socket creation
> under the lock.  No need to create a socket if we lose the race to the lock.

The code needs verbose comments to describe the rationale for the
ordering semantics you've decided on.

>  #if IS_IN (libc)
>  # define fgets_unlocked __fgets_unlocked
> @@ -393,6 +394,7 @@ _res_hconf_reorder_addrs (struct hostent *hp)
>    int i, j;
>    /* Number of interfaces.  */

Expand the comment to describe at a high level what code paths can
concurrently access this and what the effect would be/ought to be.

>    static int num_ifs = -1;
> +  int num_ifs_local;
>    /* We need to protect the dynamic buffer handling.  */
>    __libc_lock_define_initialized (static, lock);
>  
> @@ -404,7 +406,8 @@ _res_hconf_reorder_addrs (struct hostent *hp)
>    if (hp->h_addrtype != AF_INET)
>      return;
>  
> -  if (num_ifs <= 0)
> +  num_ifs_local = atomic_load_acquire (&num_ifs);
> +  if (num_ifs_local <= 0)

Why is acquire necessary/sufficient?

>      {
>        struct ifreq *ifr, *cur_ifr;
>        int sd, num, i;
> @@ -422,7 +425,7 @@ _res_hconf_reorder_addrs (struct hostent *hp)
>        __libc_lock_lock (lock);
>  
>        /* Recheck, somebody else might have done the work by now.  */
> -      if (num_ifs <= 0)
> +      if (atomic_load_acquire (&num_ifs) <= 0)

Likewise.

>  	{
>  	  int new_num_ifs = 0;
>  
> @@ -472,7 +475,8 @@ _res_hconf_reorder_addrs (struct hostent *hp)
>  	  /* Release lock, preserve error value, and close socket.  */
>  	  errno = save;
>  
> -	  num_ifs = new_num_ifs;
> +	  atomic_store_release (&num_ifs, new_num_ifs);

Likewise.

> +	  num_ifs_local = new_num_ifs;
>  	}
>  
>        __libc_lock_unlock (lock);
> @@ -480,7 +484,7 @@ _res_hconf_reorder_addrs (struct hostent *hp)
>        __close (sd);
>      }
>  
> -  if (num_ifs == 0)
> +  if (num_ifs_local == 0)
>      return;
>  
>    /* Find an address for which we have a direct connection.  */
> @@ -488,7 +492,7 @@ _res_hconf_reorder_addrs (struct hostent *hp)
>      {
>        struct in_addr *haddr = (struct in_addr *) hp->h_addr_list[i];
>  
> -      for (j = 0; j < num_ifs; ++j)
> +      for (j = 0; j < num_ifs_local; ++j)
>  	{
>  	  u_int32_t if_addr    = ifaddrs[j].u.ipv4.addr;
>  	  u_int32_t if_netmask = ifaddrs[j].u.ipv4.mask;
>