[Bug-gnulib] Re: getaddrinfo

[Simon Josefsson]

Bruno Haible <address@hidden> writes:

> Hi Simon,
>
> A few comments.
>
>> +#ifndef     GETADDRINFO_H
>> +# define GETADDRINFO_H
>
> Tabs inside #ifndef statements. Argh, why this cruelty?

I didn't see it before I posted the code, but fixed it immediately.

>> +#include <gettext.h>
>
> I'd suggest #include "gettext.h", so that
>   1. the Makefile doesn't need a -I. for gettext.h to be found,
>   2. people are not misled into thinking that gettext.h were a public
>      header file.

Done.

>> +  if (hints &&
>
> GNU coding style (section "Formatting Your Source Code") recommends
> to put the operator && into the next line.

I'm using "indent", which seem to miss that.  Fixed manually.

>> +    case PF_INET6:
>
> Can you protect this with  #if HAVE_IPV6, to avoid compilation errors
> on hosts that don't support IPv6 sockets? You find an autoconf macro for
> HAVE_IPV6 in ...

...in?  Can't seem to find it in gnulib at all.

>> +    memcpy (&sinp->sin_addr, he->h_addr_list[0], he->h_length);
>
> I'm a bit worried that this could overwrite innocent memory if the
> gethostbyname() function has returned semantically incorrect data.
> I'd add a safety check here before the memcpy:
>
>         if (he->h_length != sizeof (sinp->sin_addr))
>           abort ();

Done.

>> +    tmp->ai_addrlen = sizeof (sin);
>
> 'sin' is not a defined variable.

Oops.  So it cannot ever have compiled.  So it seems none of my
platform lack getaddrinfo...  That's good, but make it difficult to
test..  I'll try to find some ancient host somewhere.

>> +      free (cur);
>
> As Paul already noted, a memory leak:
>     free (cur->ai_addr);
> is missing here.

I'm leaning towards fixing this in the way Paul suggested.

>> +values[] =
>> +  {
>> +    { EAI_ADDRFAMILY, N_("Address family for hostname not supported") },
>
> This table has no entry for EAI_OVERFLOW; it would be worth providing such
> an entry in comments, with an explanatory comment that EAI_OVERFLOW is not
> used.

Yes, I think we should use glibc's gai_strerror.c instead.

Thanks.