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

[Guillem Jover]

Hi!

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.
I'll comment on this one anyway, as I've seen some problems.


> From 3be7f44305103b39a0e1bc79a9960feb5d142485 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <address@hidden>
> Date: Tue, 7 Sep 2010 17:53:23 +0200
> Subject: [PATCH] Make `tftpd' IPv6-compatible.

> diff --git a/src/tftpd.c b/src/tftpd.c
> index 70602fa..719db81 100644
> --- 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.

> @@ -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.

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

This should be NI_MAXHOST instead.

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

Here fromlen too.

> +                        node, sizeof node),
>             tp->th_opcode == WRQ ? "write" : "read",
>             filename, errtomsg (ecode));
>      }
> @@ -736,17 +740,29 @@ nak (int error)
>      syslog (LOG_ERR, "nak: %m\n");
>  }
>  
> -static const char *
> -verifyhost (struct sockaddr_in *fromp)
> +static char *
> +verify_host (struct sockaddr_storage *addr, socklen_t addr_len,
> +          char *host_and_service, size_t len)
>  {
> -  struct hostent *hp;
> -
> -  hp = gethostbyaddr ((char *) &fromp->sin_addr, sizeof (fromp->sin_addr),
> -                   fromp->sin_family);
> -  if (hp)
> -    return hp->h_name;
> +  int err;
> +  char host[512], service[256];

Here NI_MAXHOST and NI_MAXSERV instead of magic values.

> +
> +  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.

>    else
> -    return inet_ntoa (fromp->sin_addr);
> +    snprintf (host_and_service, len, "%s%s%s",
> +           host, *service ? "/" : "", service);
> +
> +  return host_and_service;
>  }

regards,
guillem