Re: [PATCH] nss_files: Use struct scratch_buffer for gethostbyname [BZ #18023]

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 04/09/2017 14:31, Florian Weimer wrote:
> 2017-09-04  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #18023]
> 	* nss/nss_files/files-hosts.c (gethostbyname3_multi): Use struct
> 	scratch_buffer.

LGTM with some nits below.


> 
> diff --git a/nss/nss_files/files-hosts.c b/nss/nss_files/files-hosts.c
> index 867c10c2ef..c2cd07584c 100644
> --- a/nss/nss_files/files-hosts.c
> +++ b/nss/nss_files/files-hosts.c
> @@ -22,6 +22,7 @@
>  #include <arpa/nameser.h>
>  #include <netdb.h>
>  #include <resolv/resolv-internal.h>
> +#include <scratch_buffer.h>
>  
>  
>  /* Get implementation for some internal functions.  */
> @@ -121,15 +122,12 @@ gethostbyname3_multi (FILE * stream, const char *name, int af,
>  		      int *errnop, int *herrnop, int flags)
>  {
>    /* We have to get all host entries from the file.  */
> -  size_t tmp_buflen = MIN (buflen, 4096);
> -  char tmp_buffer_stack[tmp_buflen]
> -    __attribute__ ((__aligned__ (__alignof__ (struct hostent_data))));

I can't really tell how important is the alignment of this buffer in particular,
since on subsequent 'internal_getent' it does uses a plain char buffer.  Do we
need to keep the alignment of this buffer?

> -  char *tmp_buffer = tmp_buffer_stack;
> +  struct scratch_buffer tmp_buffer;
> +  scratch_buffer_init (&tmp_buffer);
>    struct hostent tmp_result_buf;
>    int naddrs = 1;
>    int naliases = 0;
>    char *bufferend;
> -  bool tmp_buffer_malloced = false;
>    enum nss_status status;
>  
>    while (result->h_aliases[naliases] != NULL)
> @@ -138,8 +136,8 @@ gethostbyname3_multi (FILE * stream, const char *name, int af,
>    bufferend = (char *) &result->h_aliases[naliases + 1];
>  
>   again:
> -  while ((status = internal_getent (stream, &tmp_result_buf, tmp_buffer,
> -				    tmp_buflen, errnop, herrnop, af,
> +  while ((status = internal_getent (stream, &tmp_result_buf, tmp_buffer.data,
> +				    tmp_buffer.length, errnop, herrnop, af,
>  				    flags))
>  	 == NSS_STATUS_SUCCESS)
>      {
> @@ -266,52 +264,18 @@ gethostbyname3_multi (FILE * stream, const char *name, int af,
>  
>    if (status == NSS_STATUS_TRYAGAIN)
>      {
> -      size_t newsize = 2 * tmp_buflen;
> -      if (tmp_buffer_malloced)
> +      if (!scratch_buffer_grow (&tmp_buffer))
>  	{
> -	  char *newp = realloc (tmp_buffer, newsize);
> -	  if (newp != NULL)
> -	    {
> -	      assert ((((uintptr_t) newp)
> -		       & (__alignof__ (struct hostent_data) - 1))
> -		      == 0);
> -	      tmp_buffer = newp;
> -	      tmp_buflen = newsize;
> -	      goto again;
> -	    }
> -	}
> -      else if (!__libc_use_alloca (buflen + newsize))
> -	{
> -	  tmp_buffer = malloc (newsize);
> -	  if (tmp_buffer != NULL)
> -	    {
> -	      assert ((((uintptr_t) tmp_buffer)
> -		       & (__alignof__ (struct hostent_data) - 1))
> -		      == 0);
> -	      tmp_buffer_malloced = true;
> -	      tmp_buflen = newsize;
> -	      goto again;
> -	    }
> +	  *herrnop = NETDB_INTERNAL;
> +	  status = NSS_STATUS_TRYAGAIN;
>  	}
>        else
> -	{
> -	  tmp_buffer
> -	    = extend_alloca (tmp_buffer, tmp_buflen,
> -			     newsize
> -			     + __alignof__ (struct hostent_data));
> -	  tmp_buffer = (char *) (((uintptr_t) tmp_buffer
> -				  + __alignof__ (struct hostent_data)
> -				  - 1)
> -				 & ~(__alignof__ (struct hostent_data)
> -				     - 1));
> -	  goto again;
> -	}
> +	goto again;
>      }
>    else
>      status = NSS_STATUS_SUCCESS;
>   out:
> -  if (tmp_buffer_malloced)
> -    free (tmp_buffer);
> +  scratch_buffer_free (&tmp_buffer);
>    return status;
>  }

I do think this it is easier to read and follow the code *without* the goto,
something like:

scratch_buffer_init (...);
while (1)
  {
    while ((status = internal_getent (...)) == NSS_STATUS_SUCCESS)
      {
        ...
      }
    if (status == NSS_STATUS_TRYAGAIN)
      if (!scratch_buffer_grow (&tmp_buffer))
        {
          *herrnop = NETDB_INTERNAL;
          status = NSS_STATUS_TRYAGAIN;
          break;
        }
    else
      status = NSS_STATUS_SUCCESS;
  }
scratch_buffer_free (...);