[PATCH 0/3] Fixes for getnameinfo() with NI_NOFQDN

[PATCH 0/3] Fixes for getnameinfo() with NI_NOFQDN

[Adhemerval Zanella]

Adhemerval Zanella (3):
  inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566)
  inet: Remove strdupa from nrl_domainname()
  inet: Return EAI_MEMORY when nrl_domainname() fails to allocate memory

 inet/getnameinfo.c | 146 +++++++++++++++++++++++----------------------
 1 file changed, 76 insertions(+), 70 deletions(-)

-- 
2.32.0

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

[Adhemerval Zanella]

The 'not_first' is accessed on nrl_domainname() in a non atomically
way, although it is only updated after the lock is taken.

Instead of being too clever, just always take the lock (another
option might to use an atomic load and only take the lock if lock
might be taken, but I think it would be better to have this optimization
on generic lll_lock instead of ad-hoc solution).

Checked on x86_64-linux-gnu.
---
 inet/getnameinfo.c | 120 ++++++++++++++++++++++-----------------------
 1 file changed, 59 insertions(+), 61 deletions(-)

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)
 {
-  static int not_first;
+  __libc_lock_define_initialized (static, lock);
+  __libc_lock_lock (lock);
 
+  static bool not_first = false;
   if (! not_first)
     {
-      __libc_lock_define_initialized (static, lock);
-      __libc_lock_lock (lock);
-
-      if (! not_first)
-	{
-	  char *c;
-	  struct hostent *h, th;
-	  int herror;
-	  struct scratch_buffer tmpbuf;
+      char *c;
+      struct hostent *h, th;
+      int herror;
+      struct scratch_buffer tmpbuf;
 
-	  scratch_buffer_init (&tmpbuf);
-	  not_first = 1;
+      scratch_buffer_init (&tmpbuf);
 
-	  while (__gethostbyname_r ("localhost", &th,
-				    tmpbuf.data, tmpbuf.length,
-				    &h, &herror))
+      while (__gethostbyname_r ("localhost", &th,
+				tmpbuf.data, tmpbuf.length,
+				&h, &herror))
+	{
+	  if (herror == NETDB_INTERNAL && errno == ERANGE)
 	    {
-	      if (herror == NETDB_INTERNAL && errno == ERANGE)
-		{
-		  if (!scratch_buffer_grow (&tmpbuf))
-		    goto done;
-		}
-	      else
-		break;
+	      if (!scratch_buffer_grow (&tmpbuf))
+		goto done;
 	    }
+	  else
+	    break;
+	}
 
-	  if (h && (c = strchr (h->h_name, '.')))
+      if (h && (c = strchr (h->h_name, '.')))
+	domain = __strdup (++c);
+      else
+	{
+	  /* 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))
+	      goto done;
+
+	  if ((c = strchr (tmpbuf.data, '.')))
 	    domain = __strdup (++c);
 	  else
 	    {
-	      /* 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))
-		  goto done;
+	      /* 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_preserve (&tmpbuf))
+			goto done;
+		    }
+		  else
+		    break;
+		}
 
-	      if ((c = strchr (tmpbuf.data, '.')))
+	      if (h && (c = strchr(h->h_name, '.')))
 		domain = __strdup (++c);
 	      else
 		{
-		  /* We need to preserve the hostname.  */
-		  const char *hstname = strdupa (tmpbuf.data);
+		  struct in_addr in_addr;
+
+		  in_addr.s_addr = htonl (INADDR_LOOPBACK);
 
-		  while (__gethostbyname_r (hstname, &th,
-					    tmpbuf.data, tmpbuf.length,
+		  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)
@@ -146,41 +166,19 @@ nrl_domainname (void)
 			break;
 		    }
 
-		  if (h && (c = strchr(h->h_name, '.')))
+		  if (h && (c = strchr (h->h_name, '.')))
 		    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))
-				goto done;
-			    }
-			  else
-			    break;
-			}
-
-		      if (h && (c = strchr (h->h_name, '.')))
-			domain = __strdup (++c);
-		    }
 		}
 	    }
-	done:
-	  scratch_buffer_free (&tmpbuf);
 	}
 
-      __libc_lock_unlock (lock);
+    done:
+      scratch_buffer_free (&tmpbuf);
+      not_first = true;
     }
 
+  __libc_lock_unlock (lock);
+
   return domain;
 };
 
-- 
2.32.0

[PATCH 2/3] inet: Remove strdupa from nrl_domainname()

[Adhemerval Zanella]

We can use the already in place scratch_buffer.

Checked on x86_64-linux-gnu.
---
 inet/getnameinfo.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
index 58ebbb1154..69a94604bd 100644
--- a/inet/getnameinfo.c
+++ b/inet/getnameinfo.c
@@ -127,10 +127,10 @@ nrl_domainname (void)
 	  else
 	    {
 	      /* We need to preserve the hostname.  */
-	      const char *hstname = strdupa (tmpbuf.data);
-	      while (__gethostbyname_r (hstname, &th,
-					tmpbuf.data,
-					tmpbuf.length,
+	      size_t hstnamelen = strlen (tmpbuf.data) + 1;
+	      while (__gethostbyname_r (tmpbuf.data, &th,
+					tmpbuf.data + hstnamelen,
+					tmpbuf.length - hstnamelen,
 					&h, &herror))
 		{
 		  if (herror == NETDB_INTERNAL && errno == ERANGE)
-- 
2.32.0

[PATCH 3/3] inet: Return EAI_MEMORY when nrl_domainname() fails to allocate memory

[Adhemerval Zanella]

It aligns NI_NOFQDN with default behavior for getnameinfo().

Checked on x86_64-linux-gnu.
---
 inet/getnameinfo.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
index 69a94604bd..16d461428d 100644
--- a/inet/getnameinfo.c
+++ b/inet/getnameinfo.c
@@ -83,9 +83,11 @@ libc_freeres_ptr (static char *domain);
    now ignored.  */
 #define DEPRECATED_NI_IDN 192
 
-static char *
+static bool
 nrl_domainname (void)
 {
+  int r;
+
   __libc_lock_define_initialized (static, lock);
   __libc_lock_lock (lock);
 
@@ -171,15 +173,17 @@ nrl_domainname (void)
 		}
 	    }
 	}
+      not_first = true;
 
     done:
       scratch_buffer_free (&tmpbuf);
-      not_first = true;
     }
 
+  r = not_first;
+
   __libc_lock_unlock (lock);
 
-  return domain;
+  return r;
 };
 
 /* Copy a string to a destination buffer with length checking.  Return
@@ -275,13 +279,17 @@ gni_host_inet_name (struct scratch_buffer *tmpbuf,
 
   if (h)
     {
-      char *c;
-      if ((flags & NI_NOFQDN)
-	  && (c = nrl_domainname ())
-	  && (c = strstr (h->h_name, c))
-	  && (c != h->h_name) && (*(--c) == '.'))
-	/* Terminate the string after the prefix.  */
-	*c = '\0';
+      if (flags & NI_NOFQDN)
+	{
+	  if (!nrl_domainname ())
+	    return EAI_MEMORY;
+
+	  char *c = domain;
+	  if (c != NULL && (c = strstr (h->h_name, c))
+	       && (c != h->h_name) && (*(--c) == '.'))
+	    /* Terminate the string after the prefix.  */
+	    *c = '\0';
+	}
 
       /* If requested, convert from the IDN format.  */
       bool do_idn = flags & NI_IDN;
-- 
2.32.0

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

[Florian Weimer]

* 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.

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

h != NULL?

> +		      if (!scratch_buffer_grow_preserve (&tmpbuf))


I think the change to _preserve should be in the alloca elimination
patch (but see my comment there).

Thanks,
Florian

Re: [PATCH 2/3] inet: Remove strdupa from nrl_domainname()

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> We can use the already in place scratch_buffer.
>
> Checked on x86_64-linux-gnu.
> ---
>  inet/getnameinfo.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
> index 58ebbb1154..69a94604bd 100644
> --- a/inet/getnameinfo.c
> +++ b/inet/getnameinfo.c
> @@ -127,10 +127,10 @@ nrl_domainname (void)
>  	  else
>  	    {
>  	      /* We need to preserve the hostname.  */
> -	      const char *hstname = strdupa (tmpbuf.data);
> -	      while (__gethostbyname_r (hstname, &th,
> -					tmpbuf.data,
> -					tmpbuf.length,
> +	      size_t hstnamelen = strlen (tmpbuf.data) + 1;
> +	      while (__gethostbyname_r (tmpbuf.data, &th,
> +					tmpbuf.data + hstnamelen,
> +					tmpbuf.length - hstnamelen,
>  					&h, &herror))
>  		{
>  		  if (herror == NETDB_INTERNAL && errno == ERANGE)

Can you use malloc instead?  scratch_buffer_grow_preserve is a bit of an
outlier in the interface.

Thanks,
Florian

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

[Adhemerval Zanella]

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.

Re: [PATCH 2/3] inet: Remove strdupa from nrl_domainname()

[Adhemerval Zanella]

On 11/11/2021 05:18, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> We can use the already in place scratch_buffer.
>>
>> Checked on x86_64-linux-gnu.
>> ---
>>  inet/getnameinfo.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
>> index 58ebbb1154..69a94604bd 100644
>> --- a/inet/getnameinfo.c
>> +++ b/inet/getnameinfo.c
>> @@ -127,10 +127,10 @@ nrl_domainname (void)
>>  	  else
>>  	    {
>>  	      /* We need to preserve the hostname.  */
>> -	      const char *hstname = strdupa (tmpbuf.data);
>> -	      while (__gethostbyname_r (hstname, &th,
>> -					tmpbuf.data,
>> -					tmpbuf.length,
>> +	      size_t hstnamelen = strlen (tmpbuf.data) + 1;
>> +	      while (__gethostbyname_r (tmpbuf.data, &th,
>> +					tmpbuf.data + hstnamelen,
>> +					tmpbuf.length - hstnamelen,
>>  					&h, &herror))
>>  		{
>>  		  if (herror == NETDB_INTERNAL && errno == ERANGE)
> 
> Can you use malloc instead?  scratch_buffer_grow_preserve is a bit of an
> outlier in the interface.

It would require to add more complexity, since now we have two allocations
to handle (the hstname and the scratch_buffer) and on fast patch it would
actually require a malloc call, where mostly likely it would be done in
the stack fror scratch_buffer.  I am not sure it is really an improvement
here.

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

[Florian Weimer]

* Adhemerval Zanella:

> 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.

Ah, sorry, I had missed that.

> 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.

The double-checked locking idiom avoids the CAS after initialization.
With the lll_lock change, an atomic read-modify-write operation still
happens on the lock in all cases (prior to the eventual return to the
caller).

Thanks,
Florian

Re: [PATCH 2/3] inet: Remove strdupa from nrl_domainname()

[Florian Weimer]

* Adhemerval Zanella:

> On 11/11/2021 05:18, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>> We can use the already in place scratch_buffer.
>>>
>>> Checked on x86_64-linux-gnu.
>>> ---
>>>  inet/getnameinfo.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
>>> index 58ebbb1154..69a94604bd 100644
>>> --- a/inet/getnameinfo.c
>>> +++ b/inet/getnameinfo.c
>>> @@ -127,10 +127,10 @@ nrl_domainname (void)
>>>  	  else
>>>  	    {
>>>  	      /* We need to preserve the hostname.  */
>>> -	      const char *hstname = strdupa (tmpbuf.data);
>>> -	      while (__gethostbyname_r (hstname, &th,
>>> -					tmpbuf.data,
>>> -					tmpbuf.length,
>>> +	      size_t hstnamelen = strlen (tmpbuf.data) + 1;
>>> +	      while (__gethostbyname_r (tmpbuf.data, &th,
>>> +					tmpbuf.data + hstnamelen,
>>> +					tmpbuf.length - hstnamelen,
>>>  					&h, &herror))
>>>  		{
>>>  		  if (herror == NETDB_INTERNAL && errno == ERANGE)
>> 
>> Can you use malloc instead?  scratch_buffer_grow_preserve is a bit of an
>> outlier in the interface.
>
> It would require to add more complexity, since now we have two allocations
> to handle (the hstname and the scratch_buffer) and on fast patch it would
> actually require a malloc call, where mostly likely it would be done in
> the stack fror scratch_buffer.  I am not sure it is really an improvement
> here.

Fair enough, let's keep the code simple.

Thanks,
Florian

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

[Adhemerval Zanella]

On 11/11/2021 10:54, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> 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.
> 
> Ah, sorry, I had missed that.
> 
>> 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.
> 
> The double-checked locking idiom avoids the CAS after initialization.
> With the lll_lock change, an atomic read-modify-write operation still
> happens on the lock in all cases (prior to the eventual return to the
> caller).

I meant H.J internal lock optimization [1], where the lll_lock int this
case will be mostly a relaxed load instead of a CAS (since once 'domain'
is properly initialized, there is no need to take the lock).

[1] https://sourceware.org/pipermail/libc-alpha/2021-November/132899.html

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

[Florian Weimer]

* Adhemerval Zanella:

> I meant H.J internal lock optimization [1], where the lll_lock int this
> case will be mostly a relaxed load instead of a CAS (since once 'domain'
> is properly initialized, there is no need to take the lock).
>
> [1] https://sourceware.org/pipermail/libc-alpha/2021-November/132899.html

It doesn't work this way.  It still does a CAS.

Thanks,
Florian

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

[Adhemerval Zanella]

On 11/11/2021 11:12, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> I meant H.J internal lock optimization [1], where the lll_lock int this
>> case will be mostly a relaxed load instead of a CAS (since once 'domain'
>> is properly initialized, there is no need to take the lock).
>>
>> [1] https://sourceware.org/pipermail/libc-alpha/2021-November/132899.html
> 
> It doesn't work this way.  It still does a CAS.

Yeah, it helps only on contended case.  Do you think we need to keep the
double-check optimization here?

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

[Florian Weimer]

* Adhemerval Zanella:

> On 11/11/2021 11:12, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> I meant H.J internal lock optimization [1], where the lll_lock int this
>>> case will be mostly a relaxed load instead of a CAS (since once 'domain'
>>> is properly initialized, there is no need to take the lock).
>>>
>>> [1] https://sourceware.org/pipermail/libc-alpha/2021-November/132899.html
>> 
>> It doesn't work this way.  It still does a CAS.
>
> Yeah, it helps only on contended case.  Do you think we need to keep the
> double-check optimization here?

I would say yes.  It also helps with fork safety.  After the first call,
concurrent forks won't get a bad lock state.

Thanks,
Florian