This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Fix double-checked locking in _res_hconf_reorder_addrs [BZ #19074]
- From: Siddhesh Poyarekar <sid at reserved-bit dot com>
- To: Florian Weimer <fweimer at redhat dot com>, GNU C Library <libc-alpha at sourceware dot org>
- Cc: Torvald Riegel <triegel at redhat dot com>
- Date: Tue, 6 Oct 2015 19:38:53 +0530
- Subject: Re: [PATCH] Fix double-checked locking in _res_hconf_reorder_addrs [BZ #19074]
- Authentication-results: sourceware.org; auth=none
- References: <5613BF47 dot 9000503 at redhat 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;
>