Re: [bug-inetutils] [PATCH] Make `tftpd' IPv6-compatible

[Ludovic Courtès]

Hi Guillem,

Guillem Jover <address@hidden> writes:

> On Tue, 2010-09-07 at 18:05:37 +0200, Ludovic Courtès wrote:
>> The attached patch makes ‘tftpd’ IPv6-compatible.  As a side effect, it
>> makes it usable (!) on current GNU/Linux when a client connects via an
>> address other than the loopback address (FROM is always AF_INET6 on my
>> machine.)
>> 
>> The patch assumes that IPv6 support is available.  Besides, it’d be
>> better to import Gnulib’s ‘inet_ntop’ modules.
>
> Mats was already working on this, see also my review on that thread.

Oops, sorry!  I had overlooked this thread...

Mats’ patch set seems to be more generic, with additions to
libinetutils, so it’s probably the way to go.

What do you think?

> I'll comment on this one anyway, as I've seen some problems.

Thanks for taking the time.

>> --- a/src/tftpd.c
>> +++ b/src/tftpd.c
>> @@ -124,7 +124,8 @@ static int logging;
>>  
>>  static const char *errtomsg (int);
>>  static void nak (int);
>> -static const char *verifyhost (struct sockaddr_in *);
>> +static char *verify_host (struct sockaddr_storage *, socklen_t,
>> +                      char *, size_t);
>
> Why remove the constness? the returned string is not supposed to be
> modified nor freed.

The returned pointer is the third argument, hence the removal of the
‘const’ qualifier.

>> @@ -268,18 +269,19 @@ main (int argc, char *argv[])
>
>>    memset (&sin, 0, sizeof (sin));
>> -  sin.sin_family = AF_INET;
>> +  sin.ss_family = from.ss_family;
>>    if (bind (peer, (struct sockaddr *) &sin, sizeof (sin)) < 0)
>
> The socklen_t argument should be fromlen so that this works on the
> BSDs.

OK.

>> @@ -360,8 +362,10 @@ again:
>>    ecode = (*pf->f_validate) (&filename, tp->th_opcode);
>>    if (logging)
>>      {
>> +      char node[1024];
>
> This should be NI_MAXHOST instead.

Oh, right.

>>        syslog (LOG_INFO, "%s: %s request for %s: %s",
>> -          verifyhost (&from),
>> +          verify_host (&from, sizeof from,
>
> Here fromlen too.

OK.

[...]

>> +  int err;
>> +  char host[512], service[256];
>
> Here NI_MAXHOST and NI_MAXSERV instead of magic values.

Yes.

>> +  err = getnameinfo ((struct sockaddr *) addr, addr_len,
>> +                 host, sizeof host,
>> +                 service, sizeof service,
>> +                 0);
>> +
>> +  if (err)
>> +    inet_ntop (addr->ss_family,
>> +           addr->ss_family == AF_INET
>> +           ? (void *) &((struct sockaddr_in *) addr)->sin_addr
>> +           : (void *) &((struct sockaddr_in6 *) addr)->sin6_addr,
>> +           host_and_service, len);
>
> The inet_ntop call should not be needed, getnameinfo should be able to
> handle numeric addresses just fine.

Hmm I think ‘getnameinfo’ could fail, e.g., with ‘EAI_FAIL’,
‘EAI_AGAIN’, or ‘EAI_NONAME’, in which case it would make to fall back
to use ‘inet_ntop’ (the original code had that logic too.)

Anyway I’ll probably wait and see what happens to Mats’ patches.  The
test case from my patch may still be relevant, though.

Thanks,
Ludo’.

pgpY4Qmtn6V2m.pgp
Description: PGP signature