Re: [PATCH 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566)

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 11/11/2021 05:16, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
>> index 8380d85783..58ebbb1154 100644
>> --- a/inet/getnameinfo.c
>> +++ b/inet/getnameinfo.c
>> @@ -86,55 +86,75 @@ libc_freeres_ptr (static char *domain);
>>  static char *
>>  nrl_domainname (void)
>>  {
>> +  __libc_lock_define_initialized (static, lock);
>> +  __libc_lock_lock (lock);
>>  
>> +  static bool not_first = false;
>>    if (! not_first)
> 
>> +    done:
>> +      scratch_buffer_free (&tmpbuf);
>> +      not_first = true;
> 
> This is missing the acquire/release pairing for the double-checked
> locking idiom.  You can probably use the domain variable directly.

But it is done now within the lock, different than current implementation
which does outside.  I moved to be within the lock exactly to avoid the
double-checked locking idiom.

I think now that we might be moving to a more optimized lll_lock internally 
using a acquire-load+CAS instead of just CAS we can get it without need 
to code it explicitly.

> 
>> -	      if ((c = strchr (tmpbuf.data, '.')))
>> +	      if (h && (c = strchr(h->h_name, '.')))
> 
> h != NULL?

I tried to make the code as-is, since is essentially removing a tab;
but I think it makes sense.

> 
>> +		      if (!scratch_buffer_grow_preserve (&tmpbuf))
> 
> 
> I think the change to _preserve should be in the alloca elimination
> patch (but see my comment there).

Yeah, this is in fact something I did saw on my rebase, but I am 
not sure why it is still in the patch submission.