Re: [bug-inetutils] [patches 1,2,3] Making tftp/tftpd IPv6-capable.

[Mats Erik Andersson]

onsdag den  8 september 2010 klockan 06:09 skrev Guillem Jover detta:
> Hi!
> 
> On Mon, 2010-09-06 at 11:36:12 +0200, Mats Erik Andersson wrote:
> > we have a clash of two objectives here. Support OpenBSD, or not?
> 
> Well, I'm not a GNU maintainer for inetutils, so my opinion is
> non-authoritative, but I'd say portability is important, more so to
> the BSDs.
> 
> > söndag den  5 september 2010 klockan 23:05 skrev Guillem Jover detta:
> > > On Thu, 2010-08-19 at 10:39:04 +0200, Mats Erik Andersson wrote:
> > > > diff --git a/libinetutils/sockaddr_aux.c b/libinetutils/sockaddr_aux.c
> > > > new file mode 100644
> > > > index 0000000..f882f6e
> > > > --- /dev/null
> > > > +++ b/libinetutils/sockaddr_aux.c
> > > > @@ -0,0 +1,98 @@
> > > > +get_port (struct sockaddr *sa)
> > > > +set_port (struct sockaddr *sa, in_port_t port)
> > > 
> > > These two functions can be completely avoided if the port is passed as
> > > the service name in the getaddrinfo calls in the code, it will also
> > > substantially reduce the current code.
> > 
> > Not all uses are related to getaddrinfo(3), getnameinfo(3) calls.
> > Both functions are there to extract port information for printing.
> > Their use in tftp/tftpd mirror exactly the old port manipulations.
> > If you indend a complete rewrite of the code, be my guest.
> 
> I might not be looking at the right place, but I don't see where it
> might use the ports for printing, the only places where it does print
> something is in command usage, but then there we know the port already
> as it's a string argument.
> 
Interesting. I must check the code with this viewpoint in mind.
Clearly, the grand picture escaped me.

> > > > +get_socklen (struct sockaddr *sa)
> > > 
> > > This one is also unneeded, you should just use sockaddr_storage for
> > > storage as it's guaranteed to be able to store any socket address,
> > > then sizeof of the sockaddr_storage type or variable should just
> > > work fine.
> 
> > This function is absolutely necessary to get support for OpenBSD.
> > I had to redo the whole patch set once I did observe this fact.
> > The frasing "should work fine" is utterly inapplicable here.
> 
> Oh right, all BSD expect the socklen_t argument to match exactly with
> the sockaddr length of the specific protocol.
> 
> Anyway, the function is still not needed, you always know the size of
> the sockaddr. Either from getaddrinfo's ai_addrlen in struct addrinfo
> or from recvfrom's addrlen argument, for example.

Point taken! Now I better follow your original motivation for claiming
the new library functions to be superfluous. I will certainly reread
the code with the purpose of retaining as much socket information as
is safely possible.
 
> > > > diff --git a/libinetutils/tftpsubs.c b/libinetutils/tftpsubs.c
> > > > index 6eb0a09..371764b 100644
> > > > --- a/libinetutils/tftpsubs.c
> > > > +++ b/libinetutils/tftpsubs.c
> > > > @@ -287,7 +287,7 @@ synchnet (int f)
> > > >  {
> > > >    int i, j = 0;
> > > >    char rbuf[PKTSIZE];
> > > > -  struct sockaddr_in from;
> > > > +  struct sockaddr_storage from;
> > > >    socklen_t fromlen;
> > > >  
> > > >    while (1)
> > > 
> > > Shouldn't this hunk be in the next patch instead of this one anyway?
> > 
> > It went there since it is independent of any changes to src/tftp.c
> > and tftpd.c, and since the change is correct also for IPv4-only
> > settings.
> 
> Checking the actual source, it seems to be unneeded, as it's not using
> from nor fromlen at all, so switching recvfrom() to recv() should do it
> too.

Would not recv(2) instead of recvfrom(2) involve a minute risk for spoofing?
That was my original reason for leaving all recvfrom() essentially untouched.

> > > > diff --git a/src/tftpd.c b/src/tftpd.c
> > > > index f343f8a..892c709 100644
> > > > --- a/src/tftpd.c
> > > > +++ b/src/tftpd.c
> > > > @@ -103,8 +104,9 @@ static int maxtimeout = 5 * TIMEOUT;
> > > >  #define PKTSIZE        SEGSIZE+4
> > > >  static char buf[PKTSIZE];
> > > >  static char ackbuf[PKTSIZE];
> > > > -static struct sockaddr_in from;
> > > > +static struct sockaddr_storage from;
> > > >  static socklen_t fromlen;
> > > > +static char host[INET6_ADDRSTRLEN];
> > > 
> > > Probably better to reduce the scope of host and place it inside
> > > verifyhost which is the only user (although keeping it static), which
> > > implies it will be non-reentrant, but that's currently the case anyway.
> > > Also use NI_MAXHOST instead of INET6_ADDRSTRLEN from <netdb.h>.
> > 
> > It is a numerical resolution. Right?
> 
> Actually no, it should be a host resolution, which degrades to a
> numeric address in case the resolving fails, which is what getnameinfo
> will do by default if NI_NUMERICHOST is not passed to it. Which will map
> to what gethostbyaddr + inet_ntoa were doing previously.

Admittedly, my patch did contain the intentional departure from the previous
code in one aspect here: where the old code did a full hostname resolution
for logging/reporting, I found it to be wasting sufficiently much time, that
I restricted the lockup to give a numerical result only. My plan was to discuss
this matter later, but it slipped my mind.

> > > > diff --git a/src/tftp.c b/src/tftp.c
> > > > index f3b3760..e2cfe4a 100644
> > > > --- a/src/tftp.c
> > > > +++ b/src/tftp.c
> > > > @@ -338,6 +363,7 @@ setpeer (int argc, char *argv[])
> > > >      case RESOLVE_FAIL:
> > > >        return;
> > > >  
> > > > +#if 0
> > > 
> > > ??
> > 
> > I inactivate old code, which I am not certain whether it
> > need be reinserted.
> 
> Right, sorry I was not clear, I meant that this should either remove
> the code block or replace with new code.
> 
> Looking at setpeer and resolve_name, it seems it's just a way to
> gracefully handle unresolvable hosts w/o terminating the program, so
> changing the RESOLVE_NOT_RESOLVED case to just:
> 
>       connected = 0;
>       printf ("%s: unknown host\n", argv[1]);
>       return;
> 
> should preserve the intent of the previous code, as getaddrinfo will
> handle hostname and addresses in dotted format.
> 

This is also my impression. I was simply cautious about its removal.


Your comments are truly appreciated, once I now understand where our
oppinions lead different observations.

Best regards,
Mats E A