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

[Florian Weimer]

On 10/07/2015 12:28 PM, Torvald Riegel wrote:

> If you should write this page, check whether you really need the acquire
> MO in the load in the critical section, or whether relaxed MO is
> sufficient ;)

Acquire/release is needed because the access to the actual data needs to
be synchronized as well.

I'm struggling with the cppmem syntax to show that.  Is there a grammar
published anywhere?

>> It's probably best to document things that work and way, and stick to
>> that, instead of using ad-hoc schemes all over the place.
> 
> I agree that it's good to follow common patterns.  Nonetheless, you
> still need to say why the pattern applies, and make sure that all
> readers see the pattern and can check that the pattern is applied
> properly.  This can be brief -- but if people in the community feel like
> they need more details in the comments, it should be provided in most
> cases, I think.

It's not double-checked locking exactly, the lock can be taken multiple
times because the initialization might yield 0, in which case it is
retried (<= 0 is used instead of < 0).  I don't know if this
intentional, but I'm not going to change that.  It also means that I
can't use __libc_once.

>>  (I think the
>> code I showed is sufficiently close to the usual double-checked locking
>> idiom, except that the guarded section may run multiple times.)
> 
> Are you referring to that the critical section may be entered
> unnecessarily?  That is common for double-checked locking.  Or are you
> referring to something else?

This part:

  if (num_ifs <= 0)
    {
      struct ifreq *ifr, *cur_ifr;
      int sd, num, i;
…
      sd = __socket (AF_INET, SOCK_DGRAM, 0);
      if (sd < 0)
	return;
…
      __libc_lock_lock (lock);
…
      if (num_ifs <= 0)
	{

The standard idiom would create the socket *after* the final if
statement in that sequence above.  Nothing is gained by doing the socket
creation in parallel.

I'm attaching a new version of the patch with some additional comments.
 I also fixed one logic bug not present in the original code and relaxed
one of the loads.

However, I'm not sure if this is the right approach.  I wonder if it is
better to separate the concurrency aspects in a helper function (and
maybe simplify it at the time), and have the actual code use the
abstraction instead.

Florian

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-double-checked-locking-in-_res_hconf_reorder_add.patch
Type: text/x-patch
Size: 7229 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20151007/0c9bf855/attachment.bin>