Syslinux - Jan 2017 - [PATCH tftpd-hpa] tftpd: don't use AI_CANONNAME and AI_ADDRCONFIG to resolve addresses for bind

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]);
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,
-- 
2.11.0

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

Hello Ron,

On Thu, Feb 02, 2017 at 04:08:49PM +1030, Ron via Syslinux
wrote:
> 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.
Right. This coult be accomplished with a less intrusive patch that
assumes AI_CANONNAME | AI_ADDRCONFIG if name (i.e. the 3rd argument) is
non-NULL. YMMV.
> 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
Right. Today it only has an effect if the first argument to getaddrinfo
is NULL. The intension (IIUC) is that it should be used when you plan to
feed the result to bind (opposed to connect).
> 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.
The downside of using AI_ADDRCONFIG is that it makes binding to 0.0.0.0
(or ::) fail when no interface is up yet.

If we can agree in principle I can rework the patch to make one change
per patch:

 - drop AI_ADDRCONFIG for tftpd use
 - (maybe) introduce AI_PASSIVE for tftpd
 - (maybe) drop AI_CANONNAME for tftpd
> 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".
This is not the (only) reason for me. This is mostly only how it showed
up for some people, but still there are more IMHO good reasons to fix
it:

 - inconsistent behaviour when no interface is up: -a 0.0.0.0 fails,
   -a :: fails, not passing -a doesn't fail and makes tftpd bind to all
   interfaces.
 - The "no interface is up" also happens with ifupdown with no auto
   interface is used (only hotplug)
 - The "no interface is up" also happens if your laptop has no network
   connection during boot
 - It's more robust to try what the admin requested. It is possible even
   if no interface is up to bind to 0.0.0.0. So I suggest to do that and
   not try to know it better than the admin.
 - The error message

	cannot resolve local IPv4 bind address: 0.0.0.0, Name or service not known

   is misleading.
> 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 ...
See above. Which problems do you see introduced by my patch?

IMHO "we don't do it right because it might paper over other
problems"
is a poor reason for not patching. ("I don't need seatbelt or a helmet
because if my head gets hurt there is a different problem.")
 
> Assuming that at some point the NM bug will be fixed, why would we still
> want to make this change in this code?
See above. FTR: The NM bug is already reported upstream and a first
(though not fixing) patch was suggested by them.

Best regards
Uwe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL:
<http://www.zytor.com/pipermail/syslinux/attachments/20170202/45d4612f/attachment.sig>