Re: [PATCH 3/4] getaddrinfo: rewrite inet servname resolution

[OndÅej BÃlka <neleai at seznam dot cz>]

On Mon, Dec 09, 2013 at 08:41:45AM -0500, Pavel Simerda wrote:
> Move servname resolution into a separate function, clean it up
> and make it behave correctly and consistently. Check upper bound
> of numeric port input.
>
This is quite big, could it be split to smaller parts? First move code
as necessary into separate functions. Then do cleaning, Now its hand to
distinguins what was changed and what just copied.
 
> Resolves:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=14990
> https://sourceware.org/bugzilla/show_bug.cgi?id=16208
> 
> 	* sysdeps/posix/getaddrinfo.c (struct gaih_service): Remove definition.
> 	(struct gaih_typeproto): Remove dummy item.
> 	(getportbyname): Add function definition.
> 	(gaih_inet_serv): Remove function definition.
> 	(free_service_list): Add function definition.
> 	(get_service_list): Add function definition.
> 	(gaih_inet): Rename to getaddrinfo_inet().
> 	(getaddrinfo_inet): Remove old servname resolution code.
> 	(getaddrinfo_inet): Use get_service_list() instead.
> 	(getaddrinfo_inet): Ensure free_service_list() is called on error.
> 	(getaddrinfo): Remove old servname resolution code.
> 	(getaddrinfo): Call getaddrinfo_inet().
> 
> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
> index 1300238..94b21e9 100644
> --- a/sysdeps/posix/getaddrinfo.c
> +++ b/sysdeps/posix/getaddrinfo.c
> @@ -71,12 +71,6 @@ extern int __idna_to_unicode_lzlz (const char *input, char **output,
>  # include <libidn/idna.h>
>  #endif
>  
> -struct gaih_service
> -  {
> -    const char *name;
> -    int num;
> -  };
> -
>  struct gaih_servtuple
>    {
>      struct gaih_servtuple *next;
> @@ -103,7 +97,6 @@ struct gaih_typeproto
>  
>  static const struct gaih_typeproto gaih_inet_typeproto[] =
>  {
> -  { 0, 0, 0, false, "" },
>    { SOCK_STREAM, IPPROTO_TCP, 0, true, "tcp" },
>    { SOCK_DGRAM, IPPROTO_UDP, 0, true, "udp" },
>  #if defined SOCK_DCCP && defined IPPROTO_DCCP
> @@ -132,10 +125,8 @@ static const struct addrinfo default_hints =
>      .ai_next = NULL
>    };
>  
> -
>  static int
> -gaih_inet_serv (const char *servicename, const struct gaih_typeproto *tp,
> -		const struct addrinfo *req, struct gaih_servtuple *st)
> +getportbyname (const char *servname, const char *proto)
>  {
>    struct servent *s;
>    size_t tmpbuflen = 1024;
> @@ -147,7 +138,7 @@ gaih_inet_serv (const char *servicename, const struct gaih_typeproto *tp,
>      {
>        tmpbuf = __alloca (tmpbuflen);
>  
> -      r = __getservbyname_r (servicename, tp->name, &ts, tmpbuf, tmpbuflen,
> +      r = __getservbyname_r (servname, proto, &ts, tmpbuf, tmpbuflen,
>  			     &s);
>        if (r != 0 || s == NULL)
>  	{
> @@ -159,12 +150,109 @@ gaih_inet_serv (const char *servicename, const struct gaih_typeproto *tp,
>      }
>    while (r);
>  
> -  st->next = NULL;
> -  st->socktype = tp->socktype;
> -  st->protocol = ((tp->protoflag & GAI_PROTO_PROTOANY)
> -		  ? req->ai_protocol : tp->protocol);
> -  st->port = s->s_port;
> +  return s->s_port;
> +}
> +
> +static void
> +free_service_list (struct gaih_servtuple *st)
> +{
> +  while (st)
> +    {
> +      struct gaih_servtuple *tmp = st;
> +      st = st->next;
> +      free (tmp);
> +    }
> +}
> +
> +static int
> +get_service_list (const char *servname, int socktype, int protocol, int flags, struct gaih_servtuple **servtuple)
> +{
> +  if (!servname)
> +    {
> +      /* POSIX1-2008: If servname is null, the call shall return network-level
> +       * addresses for the specified nodename.
> +       *
> +       * Example:
> +       *
> +       * getaddrinfo ("localhost", NULL, ...) ->
> +       *   family=AF_INET6 socktype=0 protocol=0 address=::1 port=0
> +       *   family=AF_INET socktype=0 protocol=0 address=127.0.0.1 port=0
> +       */
> +      struct gaih_servtuple *st = malloc (sizeof (struct gaih_servtuple));
> +      if (!st)
> +        return EAI_MEMORY;
> +      st->next = NULL;
> +      st->socktype = 0;
> +      st->protocol = 0;
> +      st->port = 0;
> +
> +      *servtuple = st;
> +      return 0;
> +    }
> +
> +  int port;
> +  char *ptr;
> +  /* POSIX1-2008: If the specified address family is AF_INET, AF_INET6,
> +   * AF_UNSPEC, the service can be specified as a string specifying a
> +   * decimal port number.
> +   *
> +   * Socktype and protocol values are then used to
> +   * filter out the protocol records.
> +   */
> +  port = strtoul (servname, &ptr, 10);
> +  if (*ptr || port > 65535)
> +    {
> +      if (flags & AI_NUMERICSERV)
> +        return EAI_SERVICE;
> +      port = -1;
> +    }
> +
> +  struct gaih_servtuple head = { .next = NULL };
> +  struct gaih_servtuple *st = &head;
>  
> +  const struct gaih_typeproto *tp;
> +  bool socktype_found = false;
> +  for (tp = gaih_inet_typeproto; tp->name[0]; tp++)
> +    {
> +      /* check socktype, protocol and GAI_PROTO_NOSERVICE */
> +      if (socktype != 0 && socktype != tp->socktype)
> +        continue;
> +      socktype_found = true;
> +      if (protocol != 0 && protocol != tp->protocol && !(tp->protoflag & GAI_PROTO_PROTOANY))
> +        continue;
> +      if (!(tp->protoflag & GAI_PROTO_NOSERVICE) && servname)
> +        continue;
> +
> +      /* resolve servname if needed, check port */
> +      int st_port = port;
> +      if (st_port < 0)
> +        st_port = getportbyname (servname, tp->name);
> +      if (st_port < 0)
> +        continue;
> +
> +      st = st->next = malloc (sizeof (struct gaih_servtuple));
> +      if (!st)
> +        {
> +          free_service_list (head.next);
> +          return EAI_MEMORY;
> +        }
> +      st->socktype = tp->socktype;
> +      st->protocol = (tp->protoflag & GAI_PROTO_PROTOANY ? protocol : tp->protocol);
> +      st->port = st_port;
> +    }
> +
> +  /* POSIX1-2008:
> +   *
> +   * [EAI_SOCKTYPE] The intended socket type was not recognized.
> +   * [EAI_SERVICE] The service passed was not recognized for the specified socket type.
> +   *
> +   * Actually, POSIX is a bit unclear as it doesn't define EAI_PROTOCOL and doesn't
> +   * specify what error code should be used for unrecognized protocol.
> +   */
> +  if (!head.next)
> +    return socktype_found ? -EAI_SERVICE : -EAI_SOCKTYPE;
> +
> +  *servtuple = head.next;
>    return 0;
>  }
>  
> @@ -253,7 +341,6 @@ gaih_inet_serv (const char *servicename, const struct gaih_typeproto *tp,
>      }									      \
>   }
>  
> -
>  typedef enum nss_status (*nss_gethostbyname4_r)
>    (const char *name, struct gaih_addrtuple **pat,
>     char *buffer, size_t buflen, int *errnop,
> @@ -267,134 +354,23 @@ typedef enum nss_status (*nss_getcanonname_r)
>     int *errnop, int *h_errnop);
>  extern service_user *__nss_hosts_database attribute_hidden;
>  
> -
>  static int
> -gaih_inet (const char *name, const struct gaih_service *service,
> -	   const struct addrinfo *req, struct addrinfo **pai,
> -	   unsigned int *naddrs)
> +getaddrinfo_inet (const char *name, const char *servname,
> +                  const struct addrinfo *req, struct addrinfo **pai,
> +                  unsigned int *naddrs)
>  {
> -  const struct gaih_typeproto *tp = gaih_inet_typeproto;
> -  struct gaih_servtuple *st = (struct gaih_servtuple *) &nullserv;
>    struct gaih_addrtuple *at = NULL;
>    int rc;
>    bool got_ipv6 = false;
>    const char *canon = NULL;
>    const char *orig_name = name;
>    size_t alloca_used = 0;
> +  int error;
>  
> -  if (req->ai_protocol || req->ai_socktype)
> -    {
> -      ++tp;
> -
> -      while (tp->name[0]
> -	     && ((req->ai_socktype != 0 && req->ai_socktype != tp->socktype)
> -		 || (req->ai_protocol != 0
> -		     && !(tp->protoflag & GAI_PROTO_PROTOANY)
> -		     && req->ai_protocol != tp->protocol)))
> -	++tp;
> -
> -      if (! tp->name[0])
> -	{
> -	  if (req->ai_socktype)
> -	    return EAI_SOCKTYPE;
> -	  else
> -	    return EAI_SERVICE;
> -	}
> -    }
> -
> -  int port = 0;
> -  if (service != NULL)
> -    {
> -      if ((tp->protoflag & GAI_PROTO_NOSERVICE) != 0)
> -	return EAI_SERVICE;
> -
> -      if (service->num < 0)
> -	{
> -	  if (tp->name[0])
> -	    {
> -	      st = (struct gaih_servtuple *)
> -		alloca_account (sizeof (struct gaih_servtuple), alloca_used);
> -
> -	      if ((rc = gaih_inet_serv (service->name, tp, req, st)))
> -		return rc;
> -	    }
> -	  else
> -	    {
> -	      struct gaih_servtuple **pst = &st;
> -	      for (tp++; tp->name[0]; tp++)
> -		{
> -		  struct gaih_servtuple *newp;
> -
> -		  if ((tp->protoflag & GAI_PROTO_NOSERVICE) != 0)
> -		    continue;
> -
> -		  if (req->ai_socktype != 0
> -		      && req->ai_socktype != tp->socktype)
> -		    continue;
> -		  if (req->ai_protocol != 0
> -		      && !(tp->protoflag & GAI_PROTO_PROTOANY)
> -		      && req->ai_protocol != tp->protocol)
> -		    continue;
> -
> -		  newp = (struct gaih_servtuple *)
> -		    alloca_account (sizeof (struct gaih_servtuple),
> -				    alloca_used);
> -
> -		  if ((rc = gaih_inet_serv (service->name, tp, req, newp)))
> -		    {
> -		      if (rc)
> -			continue;
> -		      return rc;
> -		    }
> -
> -		  *pst = newp;
> -		  pst = &(newp->next);
> -		}
> -	      if (st == (struct gaih_servtuple *) &nullserv)
> -		return EAI_SERVICE;
> -	    }
> -	}
> -      else
> -	{
> -	  port = htons (service->num);
> -	  goto got_port;
> -	}
> -    }
> -  else
> -    {
> -    got_port:
> -
> -      if (req->ai_socktype || req->ai_protocol)
> -	{
> -	  st = alloca_account (sizeof (struct gaih_servtuple), alloca_used);
> -	  st->next = NULL;
> -	  st->socktype = tp->socktype;
> -	  st->protocol = ((tp->protoflag & GAI_PROTO_PROTOANY)
> -			  ? req->ai_protocol : tp->protocol);
> -	  st->port = port;
> -	}
> -      else
> -	{
> -	  /* Neither socket type nor protocol is set.  Return all socket types
> -	     we know about.  */
> -	  struct gaih_servtuple **lastp = &st;
> -	  for (++tp; tp->name[0]; ++tp)
> -	    if (tp->defaultflag)
> -	      {
> -		struct gaih_servtuple *newp;
> -
> -		newp = alloca_account (sizeof (struct gaih_servtuple),
> -				       alloca_used);
> -		newp->next = NULL;
> -		newp->socktype = tp->socktype;
> -		newp->protocol = tp->protocol;
> -		newp->port = port;
> -
> -		*lastp = newp;
> -		lastp = &newp->next;
> -	      }
> -	}
> -    }
> +  struct gaih_servtuple *st;
> +  if ((error = get_service_list (servname, req->ai_socktype, req->ai_protocol,
> +                                 req->ai_flags & AI_NUMERICSERV, &st)))
> +    return error;
>  
>    bool malloc_name = false;
>    bool malloc_addrmem = false;
> @@ -424,12 +400,13 @@ gaih_inet (const char *name, const struct gaih_service *service,
>  	  rc = __idna_to_ascii_lz (name, &p, idn_flags);
>  	  if (rc != IDNA_SUCCESS)
>  	    {
> -	      /* No need to jump to free_and_return here.  */
>  	      if (rc == IDNA_MALLOC_ERROR)
> -		return EAI_MEMORY;
> -	      if (rc == IDNA_DLOPEN_ERROR)
> -		return EAI_SYSTEM;
> -	      return EAI_IDN_ENCODE;
> +                result = EAI_MEMORY;
> +              else if (rc == IDNA_DLOPEN_ERROR)
> +                result = EAI_SYSTEM;
> +              else
> +                result = EAI_IDN_ENCODE;
> +              goto free_and_return;
>  	    }
>  	  /* In case the output string is the same as the input string
>  	     no new string has been allocated.  */
> @@ -491,7 +468,8 @@ gaih_inet (const char *name, const struct gaih_service *service,
>  			if (namebuf == NULL)
>  			  {
>  			    assert (!malloc_name);
> -			    return EAI_MEMORY;
> +                            result = EAI_MEMORY;
> +                            goto free_and_return;
>  			  }
>  			malloc_namebuf = true;
>  		      }
> @@ -1260,7 +1238,8 @@ gaih_inet (const char *name, const struct gaih_service *service,
>        }
>    }
>  
> - free_and_return:
> +free_and_return:
> +  free_service_list (st);
>    if (malloc_name)
>      free ((char *) name);
>    if (malloc_addrmem)
> @@ -2313,7 +2292,6 @@ getaddrinfo (const char *name, const char *service,
>    int i = 0;
>    int nresults = 0;
>    struct addrinfo *p = NULL;
> -  struct gaih_service gaih_service, *pservice;
>    struct addrinfo local_hints;
>  
>    if (name != NULL && name[0] == '*' && name[1] == 0)
> @@ -2376,32 +2354,11 @@ getaddrinfo (const char *name, const char *service,
>  	}
>      }
>  
> -  if (service && service[0])
> -    {
> -      char *c;
> -      gaih_service.name = service;
> -      gaih_service.num = strtoul (gaih_service.name, &c, 10);
> -      if (*c != '\0')
> -	{
> -	  if (hints->ai_flags & AI_NUMERICSERV)
> -	    {
> -	      __free_in6ai (in6ai);
> -	      return EAI_NONAME;
> -	    }
> -
> -	  gaih_service.num = -1;
> -	}
> -
> -      pservice = &gaih_service;
> -    }
> -  else
> -    pservice = NULL;
> -
>    if (hints->ai_family == AF_UNSPEC || hints->ai_family == AF_INET
>        || hints->ai_family == AF_INET6)
>      {
>        int error;
> -      if ((error = gaih_inet (name, pservice, hints, &p, &nresults)))
> +      if ((error = getaddrinfo_inet (name, service, hints, &p, &nresults)))
>  	{
>  	  freeaddrinfo (p);
>  	  __free_in6ai (in6ai);