[PATCH] gethostid (Linux variant): Switch to struct scratch_buffer [BZ #18023]

[PATCH] gethostid (Linux variant): Switch to struct scratch_buffer [BZ #18023]

[Florian Weimer]

Previously, extend_alloca was used without alloca accounting,
which could have been problematic with large NSS results.

2018-06-26  Florian Weimer  <fweimer@redhat.com>

	[BZ #18023]
	* sysdeps/unix/sysv/linux/gethostid.c (gethostid): Use struct
	scratch_buffer instead of extend_alloca.  Update comments.

diff --git a/sysdeps/unix/sysv/linux/gethostid.c b/sysdeps/unix/sysv/linux/gethostid.c
index 73c56e57e5..c8a33fe14d 100644
--- a/sysdeps/unix/sysv/linux/gethostid.c
+++ b/sysdeps/unix/sysv/linux/gethostid.c
@@ -21,6 +21,7 @@
 #include <unistd.h>
 #include <netdb.h>
 #include <not-cancel.h>
+#include <stdbool.h>
 
 #define HOSTIDFILE "/etc/hostid"
 
@@ -63,13 +64,12 @@ sethostid (long int id)
 # include <sys/param.h>
 # include <resolv/netdb.h>
 # include <netinet/in.h>
+# include <scratch_buffer.h>
 
 long int
 gethostid (void)
 {
   char hostname[MAXHOSTNAMELEN + 1];
-  size_t buflen;
-  char *buffer;
   struct hostent hostbuf, *hp;
   int32_t id;
   struct in_addr in;
@@ -88,29 +88,40 @@ gethostid (void)
 	return id;
     }
 
-  /* Getting from the file was not successful.  An intelligent guess for
-     a unique number of a host is its IP address.  Return this.  */
+  /* Getting from the file was not successful.  An intelligent guess
+     for a unique number of a host is its IP address.  To get the IP
+     address we need to know the host name.  */
   if (__gethostname (hostname, MAXHOSTNAMELEN) < 0 || hostname[0] == '\0')
     /* This also fails.  Return and arbitrary value.  */
     return 0;
 
-  buflen = 1024;
-  buffer = __alloca (buflen);
-
-  /* To get the IP address we need to know the host name.  */
-  while (__gethostbyname_r (hostname, &hostbuf, buffer, buflen, &hp, &herr)
-	 != 0
-	 || hp == NULL)
-    if (herr != NETDB_INTERNAL || errno != ERANGE)
-      return 0;
-    else
-      /* Enlarge buffer.  */
-      buffer = extend_alloca (buffer, buflen, 2 * buflen);
+  /* Determine the IP address of the host name.  */
+  struct scratch_buffer tmpbuf;
+  scratch_buffer_init (&tmpbuf);
+  while (true)
+    {
+      int ret = __gethostbyname_r (hostname, &hostbuf,
+				   tmpbuf.data, tmpbuf.length, &hp, &herr);
+      if (ret == 0)
+	break;
+      else
+	{
+	  /* Enlarge the buffer on ERANGE.  */
+	  if (herr == NETDB_INTERNAL && errno == ERANGE)
+	    {
+	      if (!scratch_buffer_grow (&tmpbuf))
+		return 0;
+	    }
+	  else
+	    /* Other errors are a failure.  Return an arbitrary value.  */
+	    return 0;
+	}
+    }
 
   in.s_addr = 0;
   memcpy (&in, hp->h_addr,
 	  (int) sizeof (in) < hp->h_length ? (int) sizeof (in) : hp->h_length);
-
+  scratch_buffer_free (&tmpbuf);
   /* For the return value to be not exactly the IP address we do some
      bit fiddling.  */
   return (int32_t) (in.s_addr << 16 | in.s_addr >> 16);

Re: [PATCH] gethostid (Linux variant): Switch to struct scratch_buffer [BZ #18023]

[Adhemerval Zanella]

On 26/06/2018 12:46, Florian Weimer wrote:
> Previously, extend_alloca was used without alloca accounting,
> which could have been problematic with large NSS results.
> 
> 2018-06-26  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #18023]
> 	* sysdeps/unix/sysv/linux/gethostid.c (gethostid): Use struct
> 	scratch_buffer instead of extend_alloca.  Update comments.
> 
> diff --git a/sysdeps/unix/sysv/linux/gethostid.c b/sysdeps/unix/sysv/linux/gethostid.c
> index 73c56e57e5..c8a33fe14d 100644
> --- a/sysdeps/unix/sysv/linux/gethostid.c
> +++ b/sysdeps/unix/sysv/linux/gethostid.c
> @@ -21,6 +21,7 @@
>  #include <unistd.h>
>  #include <netdb.h>
>  #include <not-cancel.h>
> +#include <stdbool.h>
>  
>  #define HOSTIDFILE "/etc/hostid"
>  
> @@ -63,13 +64,12 @@ sethostid (long int id)
>  # include <sys/param.h>
>  # include <resolv/netdb.h>
>  # include <netinet/in.h>
> +# include <scratch_buffer.h>
>  
>  long int
>  gethostid (void)
>  {
>    char hostname[MAXHOSTNAMELEN + 1];
> -  size_t buflen;
> -  char *buffer;
>    struct hostent hostbuf, *hp;
>    int32_t id;
>    struct in_addr in;
> @@ -88,29 +88,40 @@ gethostid (void)
>  	return id;
>      }
>  
> -  /* Getting from the file was not successful.  An intelligent guess for
> -     a unique number of a host is its IP address.  Return this.  */
> +  /* Getting from the file was not successful.  An intelligent guess
> +     for a unique number of a host is its IP address.  To get the IP
> +     address we need to know the host name.  */
>    if (__gethostname (hostname, MAXHOSTNAMELEN) < 0 || hostname[0] == '\0')
>      /* This also fails.  Return and arbitrary value.  */
>      return 0;
>  
> -  buflen = 1024;
> -  buffer = __alloca (buflen);
> -
> -  /* To get the IP address we need to know the host name.  */
> -  while (__gethostbyname_r (hostname, &hostbuf, buffer, buflen, &hp, &herr)
> -	 != 0
> -	 || hp == NULL)
> -    if (herr != NETDB_INTERNAL || errno != ERANGE)
> -      return 0;
> -    else
> -      /* Enlarge buffer.  */
> -      buffer = extend_alloca (buffer, buflen, 2 * buflen);
> +  /* Determine the IP address of the host name.  */
> +  struct scratch_buffer tmpbuf;
> +  scratch_buffer_init (&tmpbuf);
> +  while (true)
> +    {
> +      int ret = __gethostbyname_r (hostname, &hostbuf,
> +				   tmpbuf.data, tmpbuf.length, &hp, &herr);
> +      if (ret == 0)
> +	break;
> +      else
> +	{
> +	  /* Enlarge the buffer on ERANGE.  */
> +	  if (herr == NETDB_INTERNAL && errno == ERANGE)
> +	    {
> +	      if (!scratch_buffer_grow (&tmpbuf))
> +		return 0;
> +	    }
> +	  else
> +	    /* Other errors are a failure.  Return an arbitrary value.  */

Shouldn' it call 'scratch_buffer_free' here for the case the buffer is
grown and a subsequent __gethostbyname_r results something different
than ERANGE (assuming it is possible)?

> +	    return 0;
> +	}
> +    }
>  
>    in.s_addr = 0;
>    memcpy (&in, hp->h_addr,
>  	  (int) sizeof (in) < hp->h_length ? (int) sizeof (in) : hp->h_length);
> -
> +  scratch_buffer_free (&tmpbuf);
>    /* For the return value to be not exactly the IP address we do some
>       bit fiddling.  */
>    return (int32_t) (in.s_addr << 16 | in.s_addr >> 16);
> 

Re: [PATCH] gethostid (Linux variant): Switch to struct scratch_buffer [BZ #18023]

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 883 bytes --]

On 06/26/2018 06:58 PM, Adhemerval Zanella wrote:
>> +  /* Determine the IP address of the host name.  */
>> +  struct scratch_buffer tmpbuf;
>> +  scratch_buffer_init (&tmpbuf);
>> +  while (true)
>> +    {
>> +      int ret = __gethostbyname_r (hostname, &hostbuf,
>> +				   tmpbuf.data, tmpbuf.length, &hp, &herr);
>> +      if (ret == 0)
>> +	break;
>> +      else
>> +	{
>> +	  /* Enlarge the buffer on ERANGE.  */
>> +	  if (herr == NETDB_INTERNAL && errno == ERANGE)
>> +	    {
>> +	      if (!scratch_buffer_grow (&tmpbuf))
>> +		return 0;
>> +	    }
>> +	  else
>> +	    /* Other errors are a failure.  Return an arbitrary value.  */
> Shouldn' it call 'scratch_buffer_free' here for the case the buffer is
> grown and a subsequent __gethostbyname_r results something different
> than ERANGE (assuming it is possible)?

Thanks, you are right.  New patch attached.

Florian

[-- Attachment #2: gethostid.patch --]
[-- Type: text/x-patch, Size: 2963 bytes --]

Subject: [PATCH] gethostid (Linux variant): Switch to struct scratch_buffer [BZ #18023]
To: libc-alpha@sourceware.org

Previously, extend_alloca was used without alloca accounting,
which could have been problematic with large NSS results.

2018-06-26  Florian Weimer  <fweimer@redhat.com>

	[BZ #18023]
	* sysdeps/unix/sysv/linux/gethostid.c (gethostid): Use struct
	scratch_buffer instead of extend_alloca.  Update comments.

diff --git a/sysdeps/unix/sysv/linux/gethostid.c b/sysdeps/unix/sysv/linux/gethostid.c
index 73c56e57e5..2e20f034dc 100644
--- a/sysdeps/unix/sysv/linux/gethostid.c
+++ b/sysdeps/unix/sysv/linux/gethostid.c
@@ -21,6 +21,7 @@
 #include <unistd.h>
 #include <netdb.h>
 #include <not-cancel.h>
+#include <stdbool.h>
 
 #define HOSTIDFILE "/etc/hostid"
 
@@ -63,13 +64,12 @@ sethostid (long int id)
 # include <sys/param.h>
 # include <resolv/netdb.h>
 # include <netinet/in.h>
+# include <scratch_buffer.h>
 
 long int
 gethostid (void)
 {
   char hostname[MAXHOSTNAMELEN + 1];
-  size_t buflen;
-  char *buffer;
   struct hostent hostbuf, *hp;
   int32_t id;
   struct in_addr in;
@@ -88,29 +88,43 @@ gethostid (void)
 	return id;
     }
 
-  /* Getting from the file was not successful.  An intelligent guess for
-     a unique number of a host is its IP address.  Return this.  */
+  /* Getting from the file was not successful.  An intelligent guess
+     for a unique number of a host is its IP address.  To get the IP
+     address we need to know the host name.  */
   if (__gethostname (hostname, MAXHOSTNAMELEN) < 0 || hostname[0] == '\0')
     /* This also fails.  Return and arbitrary value.  */
     return 0;
 
-  buflen = 1024;
-  buffer = __alloca (buflen);
-
-  /* To get the IP address we need to know the host name.  */
-  while (__gethostbyname_r (hostname, &hostbuf, buffer, buflen, &hp, &herr)
-	 != 0
-	 || hp == NULL)
-    if (herr != NETDB_INTERNAL || errno != ERANGE)
-      return 0;
-    else
-      /* Enlarge buffer.  */
-      buffer = extend_alloca (buffer, buflen, 2 * buflen);
+  /* Determine the IP address of the host name.  */
+  struct scratch_buffer tmpbuf;
+  scratch_buffer_init (&tmpbuf);
+  while (true)
+    {
+      int ret = __gethostbyname_r (hostname, &hostbuf,
+				   tmpbuf.data, tmpbuf.length, &hp, &herr);
+      if (ret == 0)
+	break;
+      else
+	{
+	  /* Enlarge the buffer on ERANGE.  */
+	  if (herr == NETDB_INTERNAL && errno == ERANGE)
+	    {
+	      if (!scratch_buffer_grow (&tmpbuf))
+		return 0;
+	    }
+	  /* Other errors are a failure.  Return an arbitrary value.  */
+	  else
+	    {
+	      scratch_buffer_free (&tmpbuf);
+	      return 0;
+	    }
+	}
+    }
 
   in.s_addr = 0;
   memcpy (&in, hp->h_addr,
 	  (int) sizeof (in) < hp->h_length ? (int) sizeof (in) : hp->h_length);
-
+  scratch_buffer_free (&tmpbuf);
   /* For the return value to be not exactly the IP address we do some
      bit fiddling.  */
   return (int32_t) (in.s_addr << 16 | in.s_addr >> 16);

Re: [PATCH] gethostid (Linux variant): Switch to struct scratch_buffer [BZ #18023]

[Adhemerval Zanella]

On 26/06/2018 14:33, Florian Weimer wrote:
> On 06/26/2018 06:58 PM, Adhemerval Zanella wrote:
>>> +  /* Determine the IP address of the host name.  */
>>> +  struct scratch_buffer tmpbuf;
>>> +  scratch_buffer_init (&tmpbuf);
>>> +  while (true)
>>> +    {
>>> +      int ret = __gethostbyname_r (hostname, &hostbuf,
>>> +                   tmpbuf.data, tmpbuf.length, &hp, &herr);
>>> +      if (ret == 0)
>>> +    break;
>>> +      else
>>> +    {
>>> +      /* Enlarge the buffer on ERANGE.  */
>>> +      if (herr == NETDB_INTERNAL && errno == ERANGE)
>>> +        {
>>> +          if (!scratch_buffer_grow (&tmpbuf))
>>> +        return 0;
>>> +        }
>>> +      else
>>> +        /* Other errors are a failure.  Return an arbitrary value.  */
>> Shouldn' it call 'scratch_buffer_free' here for the case the buffer is
>> grown and a subsequent __gethostbyname_r results something different
>> than ERANGE (assuming it is possible)?
> 
> Thanks, you are right.  New patch attached.
> 
> Florian

New version LGTM, thanks.

Re: [PATCH] gethostid (Linux variant): Switch to struct scratch_buffer [BZ #18023]

[DJ Delorie]

fweimer@redhat.com (Florian Weimer) writes:
>	[BZ #18023]
>	* sysdeps/unix/sysv/linux/gethostid.c (gethostid): Use struct
>	scratch_buffer instead of extend_alloca.  Update comments.

LGTM

A question about style, though... you have an if/else inside an else{}
block.  Would it be more readable to do if {} elseif {} else {} instead?

Re: [PATCH] gethostid (Linux variant): Switch to struct scratch_buffer [BZ #18023]

[Andreas Schwab]

On Jun 26 2018, Florian Weimer <fweimer@redhat.com> wrote:

> @@ -88,29 +88,43 @@ gethostid (void)
>  	return id;
>      }
>  
> -  /* Getting from the file was not successful.  An intelligent guess for
> -     a unique number of a host is its IP address.  Return this.  */
> +  /* Getting from the file was not successful.  An intelligent guess
> +     for a unique number of a host is its IP address.  To get the IP
> +     address we need to know the host name.  */
>    if (__gethostname (hostname, MAXHOSTNAMELEN) < 0 || hostname[0] == '\0')
>      /* This also fails.  Return and arbitrary value.  */
>      return 0;
>  
> -  buflen = 1024;
> -  buffer = __alloca (buflen);
> -
> -  /* To get the IP address we need to know the host name.  */
> -  while (__gethostbyname_r (hostname, &hostbuf, buffer, buflen, &hp, &herr)
> -	 != 0
> -	 || hp == NULL)
> -    if (herr != NETDB_INTERNAL || errno != ERANGE)
> -      return 0;
> -    else
> -      /* Enlarge buffer.  */
> -      buffer = extend_alloca (buffer, buflen, 2 * buflen);
> +  /* Determine the IP address of the host name.  */
> +  struct scratch_buffer tmpbuf;
> +  scratch_buffer_init (&tmpbuf);
> +  while (true)
> +    {
> +      int ret = __gethostbyname_r (hostname, &hostbuf,
> +				   tmpbuf.data, tmpbuf.length, &hp, &herr);
> +      if (ret == 0)
> +	break;

That fails to handle hp == NULL any more, see bug 23679.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] gethostid (Linux variant): Switch to struct scratch_buffer [BZ #18023]

[Florian Weimer]

* Andreas Schwab:

> On Jun 26 2018, Florian Weimer <fweimer@redhat.com> wrote:
>
>> @@ -88,29 +88,43 @@ gethostid (void)
>>  	return id;
>>      }
>>  
>> -  /* Getting from the file was not successful.  An intelligent guess for
>> -     a unique number of a host is its IP address.  Return this.  */
>> +  /* Getting from the file was not successful.  An intelligent guess
>> +     for a unique number of a host is its IP address.  To get the IP
>> +     address we need to know the host name.  */
>>    if (__gethostname (hostname, MAXHOSTNAMELEN) < 0 || hostname[0] == '\0')
>>      /* This also fails.  Return and arbitrary value.  */
>>      return 0;
>>  
>> -  buflen = 1024;
>> -  buffer = __alloca (buflen);
>> -
>> -  /* To get the IP address we need to know the host name.  */
>> -  while (__gethostbyname_r (hostname, &hostbuf, buffer, buflen, &hp, &herr)
>> -	 != 0
>> -	 || hp == NULL)
>> -    if (herr != NETDB_INTERNAL || errno != ERANGE)
>> -      return 0;
>> -    else
>> -      /* Enlarge buffer.  */
>> -      buffer = extend_alloca (buffer, buflen, 2 * buflen);
>> +  /* Determine the IP address of the host name.  */
>> +  struct scratch_buffer tmpbuf;
>> +  scratch_buffer_init (&tmpbuf);
>> +  while (true)
>> +    {
>> +      int ret = __gethostbyname_r (hostname, &hostbuf,
>> +				   tmpbuf.data, tmpbuf.length, &hp, &herr);
>> +      if (ret == 0)
>> +	break;
>
> That fails to handle hp == NULL any more, see bug 23679.

Oops.  Do you want me to write a patch?

Re: [PATCH] gethostid (Linux variant): Switch to struct scratch_buffer [BZ #18023]

[Andreas Schwab]

On Sep 18 2018, Florian Weimer <fw@deneb.enyo.de> wrote:

> Oops.  Do you want me to write a patch?

There is one attached to the bug.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] gethostid (Linux variant): Switch to struct scratch_buffer [BZ #18023]

[Florian Weimer]

* Andreas Schwab:

> On Sep 18 2018, Florian Weimer <fw@deneb.enyo.de> wrote:
>
>> Oops.  Do you want me to write a patch?
>
> There is one attached to the bug.

Right, I'll take care of it.