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

[Pavel Simerda <psimerda at redhat dot com>]

----- Original Message -----
> From: "OndÅej BÃlka" <neleai@seznam.cz>
> To: "Pavel Simerda" <psimerda@redhat.com>
> Cc: "libc-alpha" <libc-alpha@sourceware.org>, "Siddhesh Poyarekar" <siddhesh@redhat.com>
> Sent: Monday, December 9, 2013 4:49:44 PM
> Subject: Re: [PATCH 3/4] getaddrinfo: rewrite inet servname resolution
> 
> 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?

I tried. And I actually kept the patch for like half a year before submitting to the ML. It's not very splittable as the original code is very poorly structured.

> First move code
> as necessary into separate functions. Then do cleaning, Now its hand to
> distinguins what was changed and what just copied.

That was my original plan but it didn't work out for the reason stated above. I'm ok with a bit longer review period if only I can avoid ending up with a badly structured code just because the original code was. No wonder nobody wanted to touch that.

Any ideas?

Cheers,

Pavel

>  
> > 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);
> 
>