Re: [PATCH v2 01/12] Simplify allocations and fix merge and continue actions [BZ #28931]

[DJ Delorie <dj@redhat.com>]

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

Siddhesh Poyarekar via Libc-alpha <libc-alpha@sourceware.org> writes:
> diff --git a/nss/Makefile b/nss/Makefile
> +tests += tst-nss-gai-actions

New test OK.

> diff --git a/nss/tst-nss-gai-actions.c b/nss/tst-nss-gai-actions.c
> +enum
> +{
> +  ACTION_MERGE = 0,
> +  ACTION_CONTINUE,
> +};
> +
> +struct test_params
> +{
> +  int action;
> +  int family;
> +  bool canon;
> +};
> +
> +struct support_chroot *chroot_env;

Ok.

> +static void
> +prepare (int argc, char **argv)
> +{
> +  chroot_env = support_chroot_create
> +    ((struct support_chroot_configuration)
> +     {
> +       .resolv_conf = "",
> +       .hosts = "",
> +       .host_conf = "multi on\n",
> +     });
> +}

I'm curious why you chose to go this route rather than using the
test-container framework, which would have done most of this for you?
(but ok)

> +/* Create the /etc/hosts file from outside the chroot.  */
> +static void
> +write_hosts (void)
> +{
> +  const int count = 512;
> +
> +  FILE *fp = xfopen (chroot_env->path_hosts, "w");
> +  fputs ("127.0.0.1   localhost localhost.localdomain\n"
> +         "::1         localhost localhost.localdomain\n",
> +         fp);
> +  for (int i = 1; i < count; ++i)
> +    fprintf (fp, "192.0.%d.%d gnu.org\n", (i / 256) & 0xff, i & 0xff);

That's a lot of gnu.org :-)

> +  xfclose (fp);
> +}

Ok.

> +static const char *
> +family_str (int family)
> +{
> +  switch (family)
> +    {
> +    case AF_UNSPEC:
> +      return "AF_UNSPEC";
> +    case AF_INET:
> +      return "AF_INET";
> +    default:
> +      __builtin_unreachable ();
> +    }
> +}

Ok.

> +static const char *
> +action_str (int action)
> +{
> +  switch (action)
> +    {
> +    case ACTION_MERGE:
> +      return "merge";
> +    case ACTION_CONTINUE:
> +      return "continue";
> +    default:
> +      __builtin_unreachable ();
> +    }
> +}

Ok.

> +/* getaddrinfo test.  To be run from a subprocess.  */
> +static void
> +test_gai (void *closure)
> +{
> +  struct test_params *params = closure;
> +
> +  struct addrinfo hints =
> +    {
> +      .ai_family = params->family,
> +    };
> +
> +  struct addrinfo *ai;
> +
> +  if (params->canon)
> +    hints.ai_flags = AI_CANONNAME;
> +
> +  /* Use /etc/hosts in the chroot.  */
> +  xchroot (chroot_env->path_chroot);
> +
> +  printf ("***** Testing \"files [SUCCESS=%s] files\" for family %s, %s\n",
> +	  action_str (params->action), family_str (params->family),
> +	  params->canon ? "AI_CANONNAME" : "");
> +
> +  int ret = getaddrinfo ("gnu.org", "80", &hints, &ai);
> +
> +  switch (params->action)
> +    {
> +    case ACTION_MERGE:
> +      if (ret == 0)
> +	{
> +	  char *formatted = support_format_addrinfo (ai, ret);
> +
> +	  printf ("merge unexpectedly succeeded:\n %s\n", formatted);
> +	  support_record_failure ();
> +	  free (formatted);
> +	}
> +      else
> +	return;
> +    case ACTION_CONTINUE:
> +	{
> +	  char *formatted = support_format_addrinfo (ai, ret);
> +
> +	  /* Verify that the result appears exactly once.  */
> +	  const char *expected = "address: STREAM/TCP 192.0.0.1 80\n"
> +	    "address: DGRAM/UDP 192.0.0.1 80\n"
> +	    "address: RAW/IP 192.0.0.1 80\n";
> +
> +	  const char *contains = strstr (formatted, expected);
> +	  const char *contains2 = NULL;
> +
> +	  if (contains != NULL)
> +	    contains2 = strstr (contains + strlen (expected), expected);
> +
> +	  if (contains == NULL || contains2 != NULL)
> +	    {
> +	      printf ("continue failed:\n%s\n", formatted);
> +	      support_record_failure ();
> +	    }
> +
> +	  free (formatted);
> +	  break;
> +	}
> +    default:
> +      __builtin_unreachable ();
> +    }
> +}

Ok.

> +static void
> +test_in_subprocess (int action)
> +{
> +  char buf[32];
> +
> +  snprintf (buf, sizeof (buf), "files [SUCCESS=%s] files",
> +	    action_str (action));
> +  __nss_configure_lookup ("hosts", buf);
> +
> +  struct test_params params =
> +    {
> +      .action = action,
> +      .family = AF_UNSPEC,
> +      .canon = false,
> +    };
> +  support_isolate_in_subprocess (test_gai, &params);
> +  params.family = AF_INET;
> +  support_isolate_in_subprocess (test_gai, &params);
> +  params.canon = true;
> +  support_isolate_in_subprocess (test_gai, &params);
> +}

Ok.

> +static int
> +do_test (void)
> +{
> +  support_become_root ();
> +  if (!support_can_chroot ())
> +    FAIL_UNSUPPORTED ("Cannot chroot\n");
> +
> +  write_hosts ();
> +  test_in_subprocess (ACTION_CONTINUE);
> +  test_in_subprocess (ACTION_MERGE);
> +
> +  support_chroot_free (chroot_env);
> +  return 0;
> +}

Ok.

> +#define PREPARE prepare
> +#include <support/test-driver.c>

Ok.

> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
> index 18dccd5924..454a27eb2f 100644
> --- a/sysdeps/posix/getaddrinfo.c
> +++ b/sysdeps/posix/getaddrinfo.c
> @@ -458,11 +458,6 @@ gaih_inet (const char *name, const struct gaih_service *service,
>  
>    if (name != NULL)
>      {
> -      at = alloca_account (sizeof (struct gaih_addrtuple), alloca_used);
> -      at->family = AF_UNSPEC;
> -      at->scopeid = 0;
> -      at->next = NULL;
> -

Moved below, ok.

> -      if (__inet_aton_exact (name, (struct in_addr *) at->addr) != 0)
> +      uint32_t addr[4];
> +      if (__inet_aton_exact (name, (struct in_addr *) addr) != 0)

Use temp buffer because not allocated, ok.

> +	  at = alloca_account (sizeof (struct gaih_addrtuple), alloca_used);
> +	  at->scopeid = 0;
> +	  at->next = NULL;
> +

Ok.

>  	  if (req->ai_family == AF_UNSPEC || req->ai_family == AF_INET)
> -	    at->family = AF_INET;
> +	    {
> +	      memcpy (at->addr, addr, sizeof (at->addr));
> +	      at->family = AF_INET;
> +	    }

Ok.

>  	  else if (req->ai_family == AF_INET6 && (req->ai_flags & AI_V4MAPPED))
>  	    {
> -	      at->addr[3] = at->addr[0];
> +	      at->addr[3] = addr[0];

Ok.

> @@ -493,49 +496,62 @@ gaih_inet (const char *name, const struct gaih_service *service,
>  
>  	  if (req->ai_flags & AI_CANONNAME)
>  	    canon = name;
> +
> +	  goto process_list;

ok.

>  	}
> -      else if (at->family == AF_UNSPEC)
> +
> +      char *scope_delim = strchr (name, SCOPE_DELIMITER);
> +      int e;
> +
> +      if (scope_delim == NULL)
> +	e = inet_pton (AF_INET6, name, addr);
> +      else
> +	e = __inet_pton_length (AF_INET6, name, scope_delim - name, addr);

Moved out of scope, ok.

> +
> +      if (e > 0)

Moved up, ok.

>  	{
> -	  char *scope_delim = strchr (name, SCOPE_DELIMITER);
> -	  int e;
> -	  if (scope_delim == NULL)
> -	    e = inet_pton (AF_INET6, name, at->addr);

Move up, ok.

> +	  at = alloca_account (sizeof (struct gaih_addrtuple),
> +			       alloca_used);
> +	  at->scopeid = 0;
> +	  at->next = NULL;
> +
> +	  if (req->ai_family == AF_UNSPEC || req->ai_family == AF_INET6)
> +	    {
> +	      memcpy (at->addr, addr, sizeof (at->addr));
> +	      at->family = AF_INET6;
> +	    }
> +	  else if (req->ai_family == AF_INET
> +		   && IN6_IS_ADDR_V4MAPPED (addr))
> +	    {
> +	      at->addr[0] = addr[3];
> +	      at->addr[1] = addr[1];
> +	      at->addr[2] = addr[2];
> +	      at->addr[3] = addr[3];
> +	      at->family = AF_INET;
> +	    }

Moved, ok.

>  	  else
> -	    e = __inet_pton_length (AF_INET6, name, scope_delim - name,
> -				    at->addr);
> -	  if (e > 0)
>  	    {
> -	      if (req->ai_family == AF_UNSPEC || req->ai_family == AF_INET6)
> -		at->family = AF_INET6;
> -	      else if (req->ai_family == AF_INET
> -		       && IN6_IS_ADDR_V4MAPPED (at->addr))
> -		{
> -		  at->addr[0] = at->addr[3];
> -		  at->family = AF_INET;
> -		}
> -	      else
> -		{
> -		  result = -EAI_ADDRFAMILY;
> -		  goto free_and_return;
> -		}
> -
> -	      if (scope_delim != NULL
> -		  && __inet6_scopeid_pton ((struct in6_addr *) at->addr,
> -					   scope_delim + 1,
> -					   &at->scopeid) != 0)
> -		{
> -		  result = -EAI_NONAME;
> -		  goto free_and_return;
> -		}

Moved, ok.

> +	      result = -EAI_ADDRFAMILY;
> +	      goto free_and_return;
> +	    }

Moved, ok.

> -	      if (req->ai_flags & AI_CANONNAME)
> -		canon = name;
> +	  if (scope_delim != NULL
> +	      && __inet6_scopeid_pton ((struct in6_addr *) at->addr,
> +				       scope_delim + 1,
> +				       &at->scopeid) != 0)
> +	    {
> +	      result = -EAI_NONAME;
> +	      goto free_and_return;

Moved, ok.
>  	    }
> +
> +	  if (req->ai_flags & AI_CANONNAME)
> +	    canon = name;
> +
> +	  goto process_list;
>  	}

Ok.

> -      if (at->family == AF_UNSPEC && (req->ai_flags & AI_NUMERICHOST) == 0)
> +      if ((req->ai_flags & AI_NUMERICHOST) == 0)

Ok.

>  	{
> -	  struct gaih_addrtuple **pat = &at;

Ok.
> +	  bool do_merge = false;

Ok.

> @@ -579,7 +596,7 @@ gaih_inet (const char *name, const struct gaih_service *service,
>  			  result = -EAI_MEMORY;
>  			  goto free_and_return;
>  			}
> -		      *pat = addrmem;
> +		      at = addrmem;

Ok.

>  		  struct gaih_addrtuple *addrfree = addrmem;
> +		  struct gaih_addrtuple **pat = &at;
> +

Ok.

>  		  free (air);
>  
> -		  if (at->family == AF_UNSPEC)
> -		    {
> -		      result = -EAI_NONAME;
> -		      goto free_and_return;
> -		    }
> -

Ok.

> @@ -732,6 +745,21 @@ gaih_inet (const char *name, const struct gaih_service *service,
>  
>  	  while (!no_more)
>  	    {
> +	      /* Always start afresh; continue should discard previous results
> +		 and the hosts database does not support merge.  */
> +	      at = NULL;
> +	      free (canonbuf);
> +	      free (addrmem);
> +	      canon = canonbuf = NULL;
> +	      addrmem = NULL;
> +
> +	      if (do_merge)
> +		{
> +		  __set_h_errno (NETDB_INTERNAL);
> +		  __set_errno (EBUSY);
> +		  break;
> +		}
> +

Ok.

> -		      status = DL_CALL_FCT (fct4, (name, pat,
> +		      status = DL_CALL_FCT (fct4, (name, &at,

Ok.

> +		      /* gethostbyname4_r may write into AT, so reset it.  */
> +		      at = NULL;

This is alloca-ted, so no leak here.  ok.

> @@ -774,7 +804,9 @@ gaih_inet (const char *name, const struct gaih_service *service,
>  		      no_data = 1;
>  
>  		      if ((req->ai_flags & AI_CANONNAME) != 0 && canon == NULL)
> -			canon = (*pat)->name;
> +			canon = at->name;
> +
> +		      struct gaih_addrtuple **pat = &at;

Ok.

>  		      while (*pat != NULL)
>  			{
> @@ -826,6 +858,8 @@ gaih_inet (const char *name, const struct gaih_service *service,
>  
>  		  if (fct != NULL)
>  		    {
> +		      struct gaih_addrtuple **pat = &at;
> +

Ok.

> +	      /* The hosts database does not support MERGE.  */
> +	      if (nss_next_action (nip, status) == NSS_ACTION_MERGE)
> +		do_merge = true;
> +

Ok.

>      process_list:
> -      if (at->family == AF_UNSPEC)
> +      if (at == NULL)

Ok.