Re: [PATCH 07/13] nss_dns: Split getanswer_ptr from getanswer_r

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 2022-08-10 05:30, Florian Weimer via Libc-alpha wrote:
> And expand the use of name_ok and qtype in getanswer_ptr (the
> former also in getanswer_r).
> 
> After further cleanups, not much code will be shared between the
> two functions.
> ---
>   resolv/nss_dns/dns-host.c | 320 +++++++++++++++++++++++++++++++-------
>   1 file changed, 268 insertions(+), 52 deletions(-)
> 

Reviewed patch with git diff --anchored="getanswer_r (const querybuf 
*answer, int anslen, const char *qname," and it looks functionally same 
as before.

LGTM.

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

> diff --git a/resolv/nss_dns/dns-host.c b/resolv/nss_dns/dns-host.c
> index 544cffbecd..d384e1f82d 100644
> --- a/resolv/nss_dns/dns-host.c
> +++ b/resolv/nss_dns/dns-host.c
> @@ -116,6 +116,11 @@ static enum nss_status getanswer_r (struct resolv_context *ctx,
>   				    struct hostent *result, char *buffer,
>   				    size_t buflen, int *errnop, int *h_errnop,
>   				    int map, int32_t *ttlp, char **canonp);
> +static enum nss_status getanswer_ptr (const querybuf *answer, int anslen,
> +				      const char *qname,
> +				      struct hostent *result, char *buffer,
> +				      size_t buflen, int *errnop,
> +				      int *h_errnop, int32_t *ttlp);
>   
>   static enum nss_status gaih_getanswer (const querybuf *answer1, int anslen1,
>   				       const querybuf *answer2, int anslen2,
> @@ -561,9 +566,8 @@ _nss_dns_gethostbyaddr2_r (const void *addr, socklen_t len, int af,
>         return errno == ECONNREFUSED ? NSS_STATUS_UNAVAIL : NSS_STATUS_NOTFOUND;
>       }
>   
> -  status = getanswer_r
> -    (ctx, host_buffer.buf, n, qbuf, T_PTR, result, buffer, buflen,
> -     errnop, h_errnop, 0 /* XXX */, ttlp, NULL);
> +  status = getanswer_ptr (host_buffer.buf, n, qbuf, result,
> +			  buffer, buflen, errnop, h_errnop, ttlp);
>     if (host_buffer.buf != orig_host_buffer)
>       free (host_buffer.buf);
>     if (status != NSS_STATUS_SUCCESS)
> @@ -659,8 +663,6 @@ getanswer_r (struct resolv_context *ctx,
>     int haveanswer, had_error;
>     char *bp, **ap, **hap;
>     char tbuf[MAXDNAME];
> -  const char *tname;
> -  int (*name_ok) (const char *);
>     u_char packtmp[NS_MAXCDNAME];
>     int have_to_map = 0;
>     uintptr_t pad = -(uintptr_t) buffer % __alignof__ (struct host_data);
> @@ -679,22 +681,8 @@ getanswer_r (struct resolv_context *ctx,
>     if (buflen - sizeof (struct host_data) != linebuflen)
>       linebuflen = INT_MAX;
>   
> -  tname = qname;
>     result->h_name = NULL;
>     end_of_message = answer->buf + anslen;
> -  switch (qtype)
> -    {
> -    case T_A:
> -    case T_AAAA:
> -      name_ok = __libc_res_hnok;
> -      break;
> -    case T_PTR:
> -      name_ok = __libc_res_dnok;
> -      break;
> -    default:
> -      *errnop = ENOENT;
> -      return NSS_STATUS_UNAVAIL;  /* XXX should be abort(); */
> -    }
>   
>     /*
>      * find first satisfactory answer
> @@ -729,7 +717,7 @@ getanswer_r (struct resolv_context *ctx,
>         *h_errnop = NO_RECOVERY;
>         return NSS_STATUS_UNAVAIL;
>       }
> -  if (__glibc_unlikely (name_ok (bp) == 0))
> +  if (__glibc_unlikely (__libc_res_hnok (bp) == 0))
>       {
>         errno = EBADMSG;
>         *errnop = EBADMSG;
> @@ -783,7 +771,7 @@ getanswer_r (struct resolv_context *ctx,
>   	  n = -1;
>   	}
>   
> -      if (__glibc_unlikely (n < 0 || (*name_ok) (bp) == 0))
> +      if (__glibc_unlikely (n < 0 || __libc_res_hnok (bp) == 0))
>   	{
>   	  ++had_error;
>   	  continue;
> @@ -816,7 +804,7 @@ getanswer_r (struct resolv_context *ctx,
>   	  continue;			/* XXX - had_error++ ? */
>   	}
>   
> -      if ((qtype == T_A || qtype == T_AAAA) && type == T_CNAME)
> +      if (type == T_CNAME)
>   	{
>   	  /* A CNAME could also have a TTL entry.  */
>   	  if (ttlp != NULL && ttl < *ttlp)
> @@ -826,7 +814,7 @@ getanswer_r (struct resolv_context *ctx,
>   	    continue;
>   	  n = __libc_dn_expand (answer->buf, end_of_message, cp,
>   				tbuf, sizeof tbuf);
> -	  if (__glibc_unlikely (n < 0 || (*name_ok) (tbuf) == 0))
> +	  if (__glibc_unlikely (n < 0 || __libc_res_hnok (tbuf) == 0))
>   	    {
>   	      ++had_error;
>   	      continue;
> @@ -857,7 +845,260 @@ getanswer_r (struct resolv_context *ctx,
>   	  continue;
>   	}
>   
> -      if (qtype == T_PTR && type == T_CNAME)
> +      if (type == T_A && qtype == T_AAAA && map)
> +	have_to_map = 1;
> +      else if (__glibc_unlikely (type != qtype))
> +	{
> +	  cp += n;
> +	  continue;			/* XXX - had_error++ ? */
> +	}
> +
> +      switch (type)
> +	{
> +	case T_A:
> +	case T_AAAA:
> +	  if (__glibc_unlikely (__strcasecmp (result->h_name, bp) != 0))
> +	    {
> +	      cp += n;
> +	      continue;			/* XXX - had_error++ ? */
> +	    }
> +
> +	  /* Stop parsing at a record whose length is incorrect.  */
> +	  if (n != rrtype_to_rdata_length (type))
> +	    {
> +	      ++had_error;
> +	      break;
> +	    }
> +
> +	  /* Skip records of the wrong type.  */
> +	  if (n != result->h_length)
> +	    {
> +	      cp += n;
> +	      continue;
> +	    }
> +	  if (!haveanswer)
> +	    {
> +	      int nn;
> +
> +	      /* We compose a single hostent out of the entire chain of
> +	         entries, so the TTL of the hostent is essentially the lowest
> +		 TTL in the chain.  */
> +	      if (ttlp != NULL && ttl < *ttlp)
> +		*ttlp = ttl;
> +	      if (canonp != NULL)
> +		*canonp = bp;
> +	      result->h_name = bp;
> +	      nn = strlen (bp) + 1;	/* for the \0 */
> +	      bp += nn;
> +	      linebuflen -= nn;
> +	    }
> +
> +	  /* Provide sufficient alignment for both address
> +	     families.  */
> +	  enum { align = 4 };
> +	  _Static_assert ((align % __alignof__ (struct in_addr)) == 0,
> +			  "struct in_addr alignment");
> +	  _Static_assert ((align % __alignof__ (struct in6_addr)) == 0,
> +			  "struct in6_addr alignment");
> +	  {
> +	    char *new_bp = PTR_ALIGN_UP (bp, align);
> +	    linebuflen -= new_bp - bp;
> +	    bp = new_bp;
> +	  }
> +
> +	  if (__glibc_unlikely (n > linebuflen))
> +	    goto too_small;
> +	  bp = __mempcpy (*hap++ = bp, cp, n);
> +	  cp += n;
> +	  linebuflen -= n;
> +	  break;
> +	default:
> +	  abort ();
> +	}
> +      if (had_error == 0)
> +	++haveanswer;
> +    }
> +
> +  if (haveanswer > 0)
> +    {
> +      *ap = NULL;
> +      *hap = NULL;
> +      /*
> +       * Note: we sort even if host can take only one address
> +       * in its return structures - should give it the "best"
> +       * address in that case, not some random one
> +       */
> +      if (haveanswer > 1 && qtype == T_A
> +	  && __resolv_context_sort_count (ctx) > 0)
> +	addrsort (ctx, host_data->h_addr_ptrs, haveanswer);
> +
> +      if (result->h_name == NULL)
> +	{
> +	  n = strlen (qname) + 1;	/* For the \0.  */
> +	  if (n > linebuflen)
> +	    goto too_small;
> +	  if (n >= MAXHOSTNAMELEN)
> +	    goto no_recovery;
> +	  result->h_name = bp;
> +	  bp = __mempcpy (bp, qname, n);	/* Cannot overflow.  */
> +	  linebuflen -= n;
> +	}
> +
> +      if (have_to_map)
> +	if (map_v4v6_hostent (result, &bp, &linebuflen))
> +	  goto too_small;
> +      *h_errnop = NETDB_SUCCESS;
> +      return NSS_STATUS_SUCCESS;
> +    }
> + no_recovery:
> +  *h_errnop = NO_RECOVERY;
> +  *errnop = ENOENT;
> +  /* Special case here: if the resolver sent a result but it only
> +     contains a CNAME while we are looking for a T_A or T_AAAA record,
> +     we fail with NOTFOUND instead of TRYAGAIN.  */
> +  return ((qtype == T_A || qtype == T_AAAA) && ap != host_data->aliases
> +	   ? NSS_STATUS_NOTFOUND : NSS_STATUS_TRYAGAIN);
> +}
> +
> +static enum nss_status
> +getanswer_ptr (const querybuf *answer, int anslen, const char *qname,
> +	       struct hostent *result, char *buffer, size_t buflen,
> +	       int *errnop, int *h_errnop, int32_t *ttlp)
> +{
> +  struct host_data
> +  {
> +    char *aliases[MAX_NR_ALIASES];
> +    unsigned char host_addr[16];	/* IPv4 or IPv6 */
> +    char *h_addr_ptrs[0];
> +  } *host_data;
> +  int linebuflen;
> +  const HEADER *hp;
> +  const u_char *end_of_message, *cp;
> +  int n, ancount, qdcount;
> +  int haveanswer, had_error;
> +  char *bp, **ap, **hap;
> +  char tbuf[MAXDNAME];
> +  const char *tname;
> +  u_char packtmp[NS_MAXCDNAME];
> +  uintptr_t pad = -(uintptr_t) buffer % __alignof__ (struct host_data);
> +  buffer += pad;
> +  buflen = buflen > pad ? buflen - pad : 0;
> +  if (__glibc_unlikely (buflen < sizeof (struct host_data)))
> +    {
> +      /* The buffer is too small.  */
> +    too_small:
> +      *errnop = ERANGE;
> +      *h_errnop = NETDB_INTERNAL;
> +      return NSS_STATUS_TRYAGAIN;
> +    }
> +  host_data = (struct host_data *) buffer;
> +  linebuflen = buflen - sizeof (struct host_data);
> +  if (buflen - sizeof (struct host_data) != linebuflen)
> +    linebuflen = INT_MAX;
> +
> +  tname = qname;
> +  result->h_name = NULL;
> +  end_of_message = answer->buf + anslen;
> +
> +  /*
> +   * find first satisfactory answer
> +   */
> +  hp = &answer->hdr;
> +  ancount = ntohs (hp->ancount);
> +  qdcount = ntohs (hp->qdcount);
> +  cp = answer->buf + HFIXEDSZ;
> +  if (__glibc_unlikely (qdcount != 1))
> +    {
> +      *h_errnop = NO_RECOVERY;
> +      return NSS_STATUS_UNAVAIL;
> +    }
> +  if (sizeof (struct host_data) + (ancount + 1) * sizeof (char *) >= buflen)
> +    goto too_small;
> +  bp = (char *) &host_data->h_addr_ptrs[ancount + 1];
> +  linebuflen -= (ancount + 1) * sizeof (char *);
> +
> +  n = __ns_name_unpack (answer->buf, end_of_message, cp,
> +			packtmp, sizeof packtmp);
> +  if (n != -1 && __ns_name_ntop (packtmp, bp, linebuflen) == -1)
> +    {
> +      if (__glibc_unlikely (errno == EMSGSIZE))
> +	goto too_small;
> +
> +      n = -1;
> +    }
> +
> +  if (__glibc_unlikely (n < 0))
> +    {
> +      *errnop = errno;
> +      *h_errnop = NO_RECOVERY;
> +      return NSS_STATUS_UNAVAIL;
> +    }
> +  if (__glibc_unlikely (__libc_res_dnok (bp) == 0))
> +    {
> +      errno = EBADMSG;
> +      *errnop = EBADMSG;
> +      *h_errnop = NO_RECOVERY;
> +      return NSS_STATUS_UNAVAIL;
> +    }
> +  cp += n + QFIXEDSZ;
> +
> +  ap = host_data->aliases;
> +  *ap = NULL;
> +  result->h_aliases = host_data->aliases;
> +  hap = host_data->h_addr_ptrs;
> +  *hap = NULL;
> +  result->h_addr_list = host_data->h_addr_ptrs;
> +  haveanswer = 0;
> +  had_error = 0;
> +
> +  while (ancount-- > 0 && cp < end_of_message && had_error == 0)
> +    {
> +      int type, class;
> +
> +      n = __ns_name_unpack (answer->buf, end_of_message, cp,
> +			    packtmp, sizeof packtmp);
> +      if (n != -1 && __ns_name_ntop (packtmp, bp, linebuflen) == -1)
> +	{
> +	  if (__glibc_unlikely (errno == EMSGSIZE))
> +	    goto too_small;
> +
> +	  n = -1;
> +	}
> +
> +      if (__glibc_unlikely (n < 0 || __libc_res_dnok (bp) == 0))
> +	{
> +	  ++had_error;
> +	  continue;
> +	}
> +      cp += n;				/* name */
> +
> +      if (__glibc_unlikely (cp + 10 > end_of_message))
> +	{
> +	  ++had_error;
> +	  continue;
> +	}
> +
> +      NS_GET16 (type, cp);
> +      NS_GET16 (class, cp);
> +      int32_t ttl;
> +      NS_GET32 (ttl, cp);
> +      NS_GET16 (n, cp);		/* RDATA length.  */
> +
> +      if (end_of_message - cp < n)
> +	{
> +	  /* RDATA extends beyond the end of the packet.  */
> +	  ++had_error;
> +	  continue;
> +	}
> +
> +      if (__glibc_unlikely (class != C_IN))
> +	{
> +	  /* XXX - debug? syslog? */
> +	  cp += n;
> +	  continue;			/* XXX - had_error++ ? */
> +	}
> +
> +      if (type == T_CNAME)
>   	{
>   	  /* A CNAME could also have a TTL entry.  */
>   	  if (ttlp != NULL && ttl < *ttlp)
> @@ -886,14 +1127,6 @@ getanswer_r (struct resolv_context *ctx,
>   	  continue;
>   	}
>   
> -      if (type == T_A && qtype == T_AAAA && map)
> -	have_to_map = 1;
> -      else if (__glibc_unlikely (type != qtype))
> -	{
> -	  cp += n;
> -	  continue;			/* XXX - had_error++ ? */
> -	}
> -
>         switch (type)
>   	{
>   	case T_PTR:
> @@ -955,8 +1188,6 @@ getanswer_r (struct resolv_context *ctx,
>   		 TTL in the chain.  */
>   	      if (ttlp != NULL && ttl < *ttlp)
>   		*ttlp = ttl;
> -	      if (canonp != NULL)
> -		*canonp = bp;
>   	      result->h_name = bp;
>   	      nn = strlen (bp) + 1;	/* for the \0 */
>   	      bp += nn;
> @@ -983,7 +1214,8 @@ getanswer_r (struct resolv_context *ctx,
>   	  linebuflen -= n;
>   	  break;
>   	default:
> -	  abort ();
> +	  cp += n;
> +	  continue;			/* XXX - had_error++ ? */
>   	}
>         if (had_error == 0)
>   	++haveanswer;
> @@ -993,14 +1225,6 @@ getanswer_r (struct resolv_context *ctx,
>       {
>         *ap = NULL;
>         *hap = NULL;
> -      /*
> -       * Note: we sort even if host can take only one address
> -       * in its return structures - should give it the "best"
> -       * address in that case, not some random one
> -       */
> -      if (haveanswer > 1 && qtype == T_A
> -	  && __resolv_context_sort_count (ctx) > 0)
> -	addrsort (ctx, host_data->h_addr_ptrs, haveanswer);
>   
>         if (result->h_name == NULL)
>   	{
> @@ -1014,23 +1238,15 @@ getanswer_r (struct resolv_context *ctx,
>   	  linebuflen -= n;
>   	}
>   
> -      if (have_to_map)
> -	if (map_v4v6_hostent (result, &bp, &linebuflen))
> -	  goto too_small;
>         *h_errnop = NETDB_SUCCESS;
>         return NSS_STATUS_SUCCESS;
>       }
>    no_recovery:
>     *h_errnop = NO_RECOVERY;
>     *errnop = ENOENT;
> -  /* Special case here: if the resolver sent a result but it only
> -     contains a CNAME while we are looking for a T_A or T_AAAA record,
> -     we fail with NOTFOUND instead of TRYAGAIN.  */
> -  return ((qtype == T_A || qtype == T_AAAA) && ap != host_data->aliases
> -	   ? NSS_STATUS_NOTFOUND : NSS_STATUS_TRYAGAIN);
> +  return NSS_STATUS_TRYAGAIN;
>   }
>   
> -
>   static enum nss_status
>   gaih_getanswer_slice (const querybuf *answer, int anslen, const char *qname,
>   		      struct gaih_addrtuple ***patp,