[syslinux] Bug#771441: [PATCH tftpd-hpa] tftpd: don't use AI_CANONNAME and AI_ADDRCONFIG to resolve addresses for bind

[Ron]

On Sun, Jan 29, 2017 at 12:09:43PM +0100, Uwe Kleine-König wrote:
> AI_CANONNAME is only relevant when the resulting official name is used,
> which is not the case in tftpd for the address to bind to. Also
> AI_ADDRCONFIG isn't helpful. This flag is good for sockets used to
> connect(2) somewhere. But for listening sockets it makes tftpd fail to
> start when -a 0.0.0.0:69 is passed and no network device is up yet.
> 
> This addresses Debian bug https://bugs.debian.org/771441
> ---
>  common/tftpsubs.c | 4 ++--
>  common/tftpsubs.h | 2 +-
>  tftp/main.c       | 9 ++++++---
>  tftpd/tftpd.c     | 6 ++++--
>  4 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/common/tftpsubs.c b/common/tftpsubs.c
> index 8c999f66eed8..344c74b3d78c 100644
> --- a/common/tftpsubs.c
> +++ b/common/tftpsubs.c
> @@ -300,7 +300,7 @@ int pick_port_bind(int sockfd, union sock_addr *myaddr,
>  }
>  
>  int
> -set_sock_addr(char *host,union sock_addr  *s, char **name)
> +set_sock_addr(char *host, union sock_addr *s, char **name, int ai_flags)
>  {
>      struct addrinfo *addrResult;
>      struct addrinfo hints;
> @@ -308,7 +308,7 @@ set_sock_addr(char *host,union sock_addr  *s, char **name)
>  
>      memset(&hints, 0, sizeof(hints));
>      hints.ai_family = s->sa.sa_family;
> -    hints.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
> +    hints.ai_flags = ai_flags;
>      hints.ai_socktype = SOCK_DGRAM;
>      hints.ai_protocol = IPPROTO_UDP;
>      err = getaddrinfo(strip_address(host), NULL, &hints, &addrResult);
> diff --git a/common/tftpsubs.h b/common/tftpsubs.h
> index b3a3bf3c95e1..0edda03a514c 100644
> --- a/common/tftpsubs.h
> +++ b/common/tftpsubs.h
> @@ -98,7 +98,7 @@ static inline int sa_set_port(union sock_addr *s, u_short port)
>         return 0;
>  }
>  
> -int set_sock_addr(char *, union sock_addr *, char **);
> +int set_sock_addr(char *, union sock_addr *, char **, int);
>  
>  struct tftphdr;
>  
> diff --git a/tftp/main.c b/tftp/main.c
> index b2f90593ed31..4aebe630af1e 100644
> --- a/tftp/main.c
> +++ b/tftp/main.c
> @@ -414,7 +414,8 @@ void setpeer(int argc, char *argv[])
>      }
>  
>      peeraddr.sa.sa_family = ai_fam;
> -    err = set_sock_addr(argv[1], &peeraddr, &hostname);
> +    err = set_sock_addr(argv[1], &peeraddr, &hostname,
> +                        AI_CANONNAME | AI_ADDRCONFIG);
>      if (err) {
>          printf("Error: %s\n", gai_strerror(err));
>          printf("%s: unknown host\n", argv[1]);
> @@ -557,7 +558,8 @@ void put(int argc, char *argv[])
>          targ = strchr(cp, ':');
>          *targ++ = 0;
>          peeraddr.sa.sa_family = ai_fam;
> -        err = set_sock_addr(cp, &peeraddr,&hostname);
> +        err = set_sock_addr(cp, &peeraddr, &hostname,
> +                            AI_CANONNAME | AI_ADDRCONFIG);
>          if (err) {
>              printf("Error: %s\n", gai_strerror(err));
>              printf("%s: unknown host\n", argv[1]);
> @@ -648,7 +650,8 @@ void get(int argc, char *argv[])
>  
>              *src++ = 0;
>              peeraddr.sa.sa_family = ai_fam;
> -            err = set_sock_addr(argv[n], &peeraddr, &hostname);
> +            err = set_sock_addr(argv[n], &peeraddr, &hostname,
> +                                AI_CANONNAME | AI_ADDRCONFIG);
>              if (err) {
>                  printf("Warning: %s\n", gai_strerror(err));
>                  printf("%s: unknown host\n", argv[1]);

Up to this point, this patch doesn't actually change the existing
operation in any way.  But in what follows ...

> diff --git a/tftpd/tftpd.c b/tftpd/tftpd.c
> index 364e7d2303e0..db22426edbb9 100644
> --- a/tftpd/tftpd.c
> +++ b/tftpd/tftpd.c
> @@ -638,7 +638,8 @@ int main(int argc, char **argv)
>                  if (fd4 >= 0) {
>                      bindaddr4.sin_family = AF_INET;
>                      err = set_sock_addr(address,
> -                                        (union sock_addr *)&bindaddr4, NULL);
> +                                        (union sock_addr *)&bindaddr4, NULL,
> +                                        AI_PASSIVE);
>                      if (err) {
>                          syslog(LOG_ERR,
>                                 "cannot resolve local IPv4 bind address: %s, %s",
> @@ -650,7 +651,8 @@ int main(int argc, char **argv)
>                  if (fd6 >= 0) {
>                      bindaddr6.sin6_family = AF_INET6;
>                      err = set_sock_addr(address,
> -                                        (union sock_addr *)&bindaddr6, NULL);
> +                                        (union sock_addr *)&bindaddr6, NULL,
> +                                        AI_PASSIVE);
>                      if (err) {
>                          if (fd4 >= 0) {
>                              syslog(LOG_ERR,

The use of AI_PASSIVE here is a placebo.  That flag has no effect unless
address was NULL, and if that was true, neither of the hunks here would
actually be executed in the first place.

Using AI_CANONNAME here should be harmless at worst.  So the only actual
change is to drop AI_ADDRCONFIG - the flag which limits getaddrinfo to
returning only the address families that are actually supported by the
configured interfaces on the system.  And ordinarily that would seem to
be a fairly uncontroversially Good Thing to do, for both connecting and
listening sockets.

So unless upstream sees this differently, I still think we'd need to see
some stronger rationale for why that isn't a Good Thing in this particular
case than just "Dropping that flag hides a real bug in NetworkManager".
Because it could hide or introduce real problems in other cases too, and
if the bug in NM is fixed, then the only reason I'm so far aware of for
you proposing this patch (based on the discussion on #d-d) also goes away
too ...

Assuming that at some point the NM bug will be fixed, why would we still
want to make this change in this code?

  Cheers,
  Ron