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

[DJ Delorie <dj@redhat.com>]

Reviewing the resulting text intead of the patch, because... easier?

Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> writes:
> static void
> nrl_domainname_core (struct scratch_buffer *tmpbuf)
> {
>   char *c;
>   struct hostent *h, th;
>   int herror;
> 
>   while (__gethostbyname_r ("localhost", &th,
> 			    tmpbuf->data, tmpbuf->length,
> 			    &h, &herror))
>     {
>       if (herror == NETDB_INTERNAL && errno == ERANGE)
> 	{
> 	  if (!scratch_buffer_grow (tmpbuf))
> 	    return;
> 	}
>       else
> 	break;
>     }

Ok.

>   if (h != NULL && (c = strchr (h->h_name, '.')) != NULL)
>     domain = __strdup (++c);
>   else

Ok.

>     {
>       /* The name contains no domain information.  Use the name
> 	 now to get more information.  */
>       while (__gethostname (tmpbuf->data, tmpbuf->length))
> 	if (!scratch_buffer_grow (tmpbuf))
> 	  return;

Ok.

>       if ((c = strchr (tmpbuf->data, '.')) != NULL)
> 	domain = __strdup (++c);
>       else
> 	{
> 	  /* We need to preserve the hostname.  */
> 	  const char *hstname = strdupa (tmpbuf->data);
> 	  while (__gethostbyname_r (hstname, &th,
> 				    tmpbuf->data,
> 				    tmpbuf->length,
> 				    &h, &herror))
> 	    {
> 	      if (herror == NETDB_INTERNAL && errno == ERANGE)
> 		{
> 		  if (!scratch_buffer_grow (tmpbuf))
> 		    return;
> 		}
> 	      else
> 		break;
> 	    }

Ok.

> 	  if (h != NULL && (c = strchr(h->h_name, '.')) != NULL)
> 	    domain = __strdup (++c);
> 	  else
> 	    {
> 	      struct in_addr in_addr;
> 
> 	      in_addr.s_addr = htonl (INADDR_LOOPBACK);
> 
> 	      while (__gethostbyaddr_r ((const char *) &in_addr,
> 					sizeof (struct in_addr),
> 					AF_INET, &th,
> 					tmpbuf->data,
> 					tmpbuf->length,
> 					&h, &herror))
> 		{
> 		  if (herror == NETDB_INTERNAL && errno == ERANGE)
> 		    {
> 		      if (!scratch_buffer_grow (tmpbuf))
> 			return;
> 		    }
> 		  else
> 		    break;
> 		}

Ok.

> 	      if (h != NULL && (c = strchr (h->h_name, '.')) != NULL)
> 		domain = __strdup (++c);
> 	    }
> 	}
>     }
> }

Ok.

> static char *
> nrl_domainname (void)
> {
>   static int not_first;
> 
>   if (__glibc_likely (atomic_load_acquire (&not_first) != 0))
>     return domain;

Multiple threads may have gotten to this point the "first time" (esp if
they get called while the above function is running)...

>   __libc_lock_define_initialized (static, lock);
>   __libc_lock_lock (lock);
> 
>   if (atomic_load_relaxed (&not_first) == 0)
>     {

... so we test again inside the lock, only one will get the lock *and*
see not_first still zero; ok.

>       struct scratch_buffer tmpbuf;
>       scratch_buffer_init (&tmpbuf);
> 
>       nrl_domainname_core (&tmpbuf);
> 
>       scratch_buffer_free (&tmpbuf);

Calculate the static "domain", ok

>       atomic_store_release (&not_first, 1);

Set not_first before releasing lock; Ok.

>     }
> 
>   __libc_lock_unlock (lock);

This is the only exit path, so OK.

>   return domain;
> };

Ok.

LGTM

Reviewed-by: DJ Delorie <dj@redhat.com>