getaddrinfo(3) / AI_ADDRCONFIG

getaddrinfo(3) / AI_ADDRCONFIG

[Christopher Friedt]

Hi list,

I have a patch for getaddrinfo(3) so that AI_ADDRCONFIG is properly
supported. Currently, if user code passes in the aforementioned flag,
there is no check to see whether a non-loopback network interface is
configured with the specified (or unspecified) address family before
returning results, which is what getaddrinfo(3) should do according to
[1].

As a result of the current behaviour, musl's getaddrinfo(3) would
return e.g. "::1" to user code even when no interfaces (including lo)
were configured with IPv6 addresses. I've documented this to some
extent here [2].

Please see the patch at [3] for my fix. Any feedback is welcome. The
patch It applies against master and 1.1.19, but possibly other
releases.

C

[1] http://man7.org/linux/man-pages/man3/getaddrinfo.3.html
[2] https://issues.apache.org/jira/browse/THRIFT-4594
[3] https://patch-diff.githubusercontent.com/raw/cfriedt/musl/pull/1
https://patch-diff.githubusercontent.com/raw/cfriedt/musl/pull/1.diff

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Rich Felker]

On Mon, Jul 09, 2018 at 11:16:04AM -0400, Christopher Friedt wrote:
> Hi list,
> 
> I have a patch for getaddrinfo(3) so that AI_ADDRCONFIG is properly
> supported. Currently, if user code passes in the aforementioned flag,
> there is no check to see whether a non-loopback network interface is
> configured with the specified (or unspecified) address family before
> returning results, which is what getaddrinfo(3) should do according to
> [1].
> 
> As a result of the current behaviour, musl's getaddrinfo(3) would
> return e.g. "::1" to user code even when no interfaces (including lo)
> were configured with IPv6 addresses. I've documented this to some
> extent here [2].
> 
> Please see the patch at [3] for my fix. Any feedback is welcome. The
> patch It applies against master and 1.1.19, but possibly other
> releases.
> 
> C
> 
> [1] http://man7.org/linux/man-pages/man3/getaddrinfo.3.html
> [2] https://issues.apache.org/jira/browse/THRIFT-4594
> [3] https://patch-diff.githubusercontent.com/raw/cfriedt/musl/pull/1
> https://patch-diff.githubusercontent.com/raw/cfriedt/musl/pull/1.diff

AI_ADDRCONFIG was discussed in 2014 as part of the "Remaining tasks on
resolver overhaul" thread:

http://openwall.com/lists/musl/2014/06/02/1

The consensus at the time was that it should be left as-is, in
accordance with "Current implementation of AI_ADDRCONFIG considered
harmful" (re: glibc implementation):

https://fedoraproject.org/wiki/QA/Networking/NameResolution/ADDRCONFIG

POSIX does not clearly specify how "only if an IPv6 address is
configured on the local system" is determined, but the glibc behavior
of ignoring ::1 on lo seems clearly non-conforming. My assumption at
the time was that ::1 would always be configured and available unless
IPv6 support was omitted from the kernel, so that any test involving
iteration of interfaces would be meaningless; at most probing ::1, or
probably just trying socket(AF_INET6,...) would suffice to determine
what to do.

It's unclear to me (and I think to everyone) what an application
actually wants when it uses AI_ADDRCONFIG. Neither knowing whether
IPv6 is supported at all on the host, nor whether there happens to be
*some* interface or non-lo interface with an IPv6 address (think: it
might just be a private-network VPN), tells you anything about whether
the IPv6 addresses for the particular hostname you're looking up with
getaddrinfo is routable. The more likely thing an application might
want is to request whichever result is routable, but THAT ALREADY
HAPPENS without AI_ADDRCONFIG: the results are sorted such that
routable ones come before non-routable ones, so if you try them in
order, you'll never hit a non-routable address family unless all the
results for the other family fail to be reachable.

So at this point my leaning is somewhere between:

1. Saying it's 2018 and having a system without IPv6 support (at least
   ::1) is an unsupported configuration.

2. Implementing AI_ADDRCONFIG as detection for the case where IPv6 has
   been completely disabled at the kernel or container level.
   
I'm not sure what option 2 entails if IPv6 is disabled at the
container level but socket(AF_INET6,...) still succeeds, so we should
perhaps look into that if you or other users feel strongly that
AI_ADDRCONFIG should do something here. But it shouldn't involve any
O(n) iteration of interfaces, allocation, or pulling in other heavy
code.

Rich

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Christopher Friedt]

On Mon, Jul 9, 2018 at 6:38 PM Rich Felker <dalias@libc.org> wrote:
> POSIX does not clearly specify how "only if an IPv6 address is
> configured on the local system" is determined, but the glibc behavior
> of ignoring ::1 on lo seems clearly non-conforming. My assumption a
> the time was that ::1 would always be configured and available unless
> IPv6 support was omitted from the kernel, so that any test involving
> iteration of interfaces would be meaningless; at most probing ::1, or
> probably just trying socket(AF_INET6,...) would suffice to determine
> what to do.
>
> It's unclear to me (and I think to everyone) what an application
> actually wants when it uses AI_ADDRCONFIG. Neither knowing whether

You did point out that glibc is non-conforming, and I assumed that
musl would prefer to be more conforming as well. The patch I provided
*is* conforming in that it does not assume AI_ADDRCONFIG is present
when hints are NULL.

Your assumption actually missed the mark a bit though. If IPv6 is
initially configured for an interface, even if no routable address is
assigned, it will get a link-local address (which is not a loopback
address, and so is not skipped by getaddrinfo). So it will not be ::1
that comes back by default but something in the fe80::/10 range.

The description of AI_ADDRCONFIG below is also fairly straightforward,
and tackles the exact problem I encountered (see the last sentence of
the paragraph below).

 http://man7.org/linux/man-pages/man3/getaddrinfo.3.html

If hints.ai_flags includes the AI_ADDRCONFIG flag, then IPv4
       addresses are returned in the list pointed to by res only if the
       local system has at least one IPv4 address configured, and IPv6
       addresses are returned only if the local system has at least one IPv6
       address configured.  The loopback address is not considered for this
       case as valid as a configured address.  This flag is useful on, for
       example, IPv4-only systems, to ensure that getaddrinfo() does not
       return IPv6 socket addresses that would always fail in connect(2) or
       bind(2).

At least 4 majorly (ubuquitously?) adopted libc's have implemented
AI_ADDRCONFIG this way (glibc, uclibc, Apple libc, BSD libc). My
suggestion would be to do what everyone else does until there is
better clarification rather than try and be a snowflake.

> IPv6 is supported at all on the host, nor whether there happens to be
> *some* interface or non-lo interface with an IPv6 address (think: it
> might just be a private-network VPN), tells you anything about whether
> the IPv6 addresses for the particular hostname you're looking up with

IPv6 support does not imply that a network interface needs to be
configured with an IPv6 address.

The point you are trying to make seems to be addressing glibc's
non-conformance as opposed to AI_ADDRCONFIG's intended functionality.
Since the patch does not introduce non-conformance, and actually does
not negatively alter musl's behaviour.

> getaddrinfo is routable. The more likely thing an application might
> want is to request whichever result is routable

^^
PRECISELY

> want is to request whichever result is routable, but THAT ALREADY
> HAPPENS without AI_ADDRCONFIG: the results are sorted such that

In fact IT DOES NOT ALREADY HAPPEN. Musl's getaddrinfo does not
provide a routable socket at all in the example I provided. In fact,
there are exactly zero network interfaces with an IPv6 address in this
case, but somehow musl comes back with "::1" for localhost, which is
more damaging than useful in this use case. You could almost say that
musl is currently also non-conformant in that sense.

> routable ones come before non-routable ones, so if you try them in
> order, you'll never hit a non-routable address family unless all the
> results for the other family fail to be reachable.

Want proof? Download the tarball I provided here:
https://issues.apache.org/jira/browse/THRIFT-4594

Using a default Docker installation, run it with "docker build -t foo
." (where . is the directory with the Dockerfile).

The C++ / c_glib unit tests will work in this case (with the fix) (the
python unit tests are currently not compiling).

Then, comment-out the line that applies the patch to musl in the Dockerfile.
# RUN patch -p1 < musl-1.1.19-getaddrinfo-ai-addrconfig.patch

The C++ / c_glib unit tests will break in this case. You can
step-through the code yourself, or just trust the instrumentation I've
already done detailed at issues.apache.org.

So there you have it. A use case with a demonstrated bug and fix that
is 100% reproducible.

> So at this point my leaning is somewhere between:
>
> 1. Saying it's 2018 and having a system without IPv6 support (at least
>    ::1) is an unsupported configuration.

That's a slippery-slope argument. Just because a system supports IPv6
does not imply that a network interface needs to be configured with an
IPv6 address. As an example, my lame cable ISP does not route IPv6
traffic nor does it provide an IPv6 prefix. They are IPv4-only (you
may think I live in the dark ages). Therefore, it does not even make
sense for the average customer to configure IPv6 on their LAN. Does my
OS support IPv6? Of course. ... Did I bypass them with a 6in4 tunnel -
you can be damn sure ;-) But many people wouldn't bother with that.

> 2. Implementing AI_ADDRCONFIG as detection for the case where IPv6 has
>    been completely disabled at the kernel or container level.

That's not at all the case here.

3. Where a user's OS supports IPv6 but they have simply opted not to
assign an IPv6 address to their network interface or use a link-local
address.

> I'm not sure what option 2 entails if IPv6 is disabled at the
> container level but socket(AF_INET6,...) still succeeds, so we should
> perhaps look into that if you or other users feel strongly that
> AI_ADDRCONFIG should do something here. But it shouldn't involve any
> O(n) iteration of interfaces, allocation, or pulling in other heavy
> code.

My only emphasis would be to support it to the end that it does what
is expected when passed in, and to not make it a default. Review the
patch, because AI_ADDRCONFIG is not a default flag.

It's O(n) in the worst case. In the best case, it's O(1). In the
average case (probabilistically speaking, where network interfaces
each have v4 and v6 addresses), it's still O(1). In the default case,
the only overhead is the time it takes to perform a mask and compare.

C

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Rich Felker]

On Mon, Jul 09, 2018 at 08:11:48PM -0400, Christopher Friedt wrote:
> > getaddrinfo is routable. The more likely thing an application might
> > want is to request whichever result is routable
> 
> ^^
> PRECISELY
> 
> > want is to request whichever result is routable, but THAT ALREADY
> > HAPPENS without AI_ADDRCONFIG: the results are sorted such that
> 
> In fact IT DOES NOT ALREADY HAPPEN. Musl's getaddrinfo does not
> provide a routable socket at all in the example I provided. In fact,
> there are exactly zero network interfaces with an IPv6 address in this
> case, but somehow musl comes back with "::1" for localhost, which is
> more damaging than useful in this use case. You could almost say that
> musl is currently also non-conformant in that sense.
> 
> > routable ones come before non-routable ones, so if you try them in
> > order, you'll never hit a non-routable address family unless all the
> > results for the other family fail to be reachable.
> 
> Want proof? Download the tarball I provided here:
> https://issues.apache.org/jira/browse/THRIFT-4594
> [...]
> So there you have it. A use case with a demonstrated bug and fix that
> is 100% reproducible.

Can you provide a minimized test case (short single C source file) to
reproduce this, or an strace log of the test that fails? The latter is
probably actually be better if the behavior is dependent on the Docker
network configuration. Assuming the test is attempting to lookup and
bind on "localhost" by name, which is what it appears to be doing
here:

https://github.com/apache/thrift/blob/82ae9575cdc112088771fc7b876f75e1e4d85ebb/lib/cpp/test/TServerSocketTest.cpp

the behavior you're experiencing is not what I expect from musl;
rather my expectation is that you would get 127.0.0.1 as the first
result and ::1 as the second, and this is exactly what I see if I do:

ip addr del ::1 dev lo

on my laptop running Alpine, then call getaddrinfo for localhost with
a small test program.

The logic to sort results does gratuitously depend on v4mapped
addresses working to do the IPv4 routability probing; if something
about the configuration suppresses their ability to work, it will
break. This is a known open issue I want to go back and fix. Seeing
the strace would show me right away if it's the source of your
problem, and even if not chances are very good that it would point out
whatever the cause is.

> > So at this point my leaning is somewhere between:
> >
> > 1. Saying it's 2018 and having a system without IPv6 support (at least
> >    ::1) is an unsupported configuration.
> 
> That's a slippery-slope argument. Just because a system supports IPv6
> does not imply that a network interface needs to be configured with an
> IPv6 address. As an example, my lame cable ISP does not route IPv6
> traffic nor does it provide an IPv6 prefix.

Right. But unless you actively suppress it you'll have ::1 and
link-local IPv6 addresses, which would cause AI_ADDRCONFIG to produce
IPv6 results.

> They are IPv4-only (you
> may think I live in the dark ages). Therefore, it does not even make
> sense for the average customer to configure IPv6 on their LAN. Does my
> OS support IPv6? Of course. ... Did I bypass them with a 6in4 tunnel -
> you can be damn sure ;-) But many people wouldn't bother with that.
> 
> > 2. Implementing AI_ADDRCONFIG as detection for the case where IPv6 has
> >    been completely disabled at the kernel or container level.
> 
> That's not at all the case here.
> 
> 3. Where a user's OS supports IPv6 but they have simply opted not to
> assign an IPv6 address to their network interface or use a link-local
> address.

AI_ADDRCONFIG is not designed to help you here, but getaddrinfo
already does help you (with or without passing that flag) by sorting
results by routability.

Rich

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Christopher Friedt]

[-- Attachment #1: Type: text/plain, Size: 1398 bytes --]

On Mon, Jul 9, 2018, 9:00 PM Rich Felker, <dalias@libc.org> wrote:

> Can you provide a minimized test case (short single C source file) to
> reproduce this, or an strace log of the test that fails? The latter is
> probably actually be better if the behavior is dependent on the Docker
> network configuration. Assuming the test is attempting to lookup and
> bind on "localhost" by name, which is what it appears to be doing
> here:
>
>
> https://github.com/apache/thrift/blob/82ae9575cdc112088771fc7b876f75e1e4d85ebb/lib/cpp/test/TServerSocketTest.cpp
>
> the behavior you're experiencing is not what I expect from musl;
> rather my expectation is that you would get 127.0.0.1 as the first
> result and ::1 as the second, and this is exactly what I see if I do:
>
> ip addr del ::1 dev lo
>
> on my laptop running Alpine, then call getaddrinfo for localhost with
> a small test program.
>
> The logic to sort results does gratuitously depend on v4mapped
> addresses working to do the IPv4 routability probing; if something
> about the configuration suppresses their ability to work, it will
> break. This is a known open issue I want to go back and fix. Seeing
> the strace would show me right away if it's the source of your
> problem, and even if not chances are very good that it would point out
> whatever the cause is.
>

Strange - yeah, I'll write up a small program in C to demonstrate.

>

[-- Attachment #2: Type: text/html, Size: 2132 bytes --]

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Rich Felker]

On Tue, Jul 10, 2018 at 08:05:50AM -0400, Christopher Friedt wrote:
> On Mon, Jul 9, 2018, 9:00 PM Rich Felker, <dalias@libc.org> wrote:
> 
> > Can you provide a minimized test case (short single C source file) to
> > reproduce this, or an strace log of the test that fails? The latter is
> > probably actually be better if the behavior is dependent on the Docker
> > network configuration. Assuming the test is attempting to lookup and
> > bind on "localhost" by name, which is what it appears to be doing
> > here:
> >
> >
> > https://github.com/apache/thrift/blob/82ae9575cdc112088771fc7b876f75e1e4d85ebb/lib/cpp/test/TServerSocketTest.cpp
> >
> > the behavior you're experiencing is not what I expect from musl;
> > rather my expectation is that you would get 127.0.0.1 as the first
> > result and ::1 as the second, and this is exactly what I see if I do:
> >
> > ip addr del ::1 dev lo
> >
> > on my laptop running Alpine, then call getaddrinfo for localhost with
> > a small test program.
> >
> > The logic to sort results does gratuitously depend on v4mapped
> > addresses working to do the IPv4 routability probing; if something
> > about the configuration suppresses their ability to work, it will
> > break. This is a known open issue I want to go back and fix. Seeing
> > the strace would show me right away if it's the source of your
> > problem, and even if not chances are very good that it would point out
> > whatever the cause is.
> >
> 
> Strange - yeah, I'll write up a small program in C to demonstrate.

OK. Can you also post the results of:

	strace -o logfile your_test_prog

This avoids the need for me or someone else to reproduce the full
configuration to see what's going on. strace output is almost
certainly the fastest path to a solution.

Rich

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Christopher Friedt]

On Tue, Jul 10, 2018 at 11:09 AM Rich Felker <dalias@libc.org> wrote:
> OK. Can you also post the results of:
>
>         strace -o logfile your_test_prog

test.c, output, strace log, and ifconfig -a output here:

https://pastebin.com/UmJi02px

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Christopher Friedt]

On Tue, Jul 10, 2018 at 7:21 PM Christopher Friedt
<chrisfriedt@gmail.com> wrote:
> test.c, output, strace log, and ifconfig -a output here:
>
> https://pastebin.com/UmJi02px

So it's definitely returning an IPv4 socket (which Thrift throws away
in favour of the IPv6 socket). Since no adapter has the IPv6 address
returned,  bind(2) would fail on a subsequent call.

After applying the patch [1] to musl,

# ./test -v
struct addrinfo {
   ai_flags:        0
   ai_family:       2
   ai_socktype:     1
   ai_protocol:     6
   ai_addrlen:      16
   ai_addr:
      family:       2
      addr:         127.0.0.1
      port:         0
   ai_canonname:    localhost
   ai_next:         0
}

[1] https://patch-diff.githubusercontent.com/raw/cfriedt/musl/pull/1.diff

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Christopher Friedt]

On Tue, Jul 10, 2018 at 7:30 PM Christopher Friedt
<chrisfriedt@gmail.com> wrote:
> [1] https://patch-diff.githubusercontent.com/raw/cfriedt/musl/pull/1.diff

The one shortcoming of that patch, that I can tell, is when nais == 0
directly before the end of gataddrinfo. If that is the case, it should
likely free the allocated memory and then return EAI_FAIL.

I'll update my branch.

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Rich Felker]

On Tue, Jul 10, 2018 at 07:30:05PM -0400, Christopher Friedt wrote:
> On Tue, Jul 10, 2018 at 7:21 PM Christopher Friedt
> <chrisfriedt@gmail.com> wrote:
> > test.c, output, strace log, and ifconfig -a output here:
> >
> > https://pastebin.com/UmJi02px
> 
> So it's definitely returning an IPv4 socket (which Thrift throws away
> in favour of the IPv6 socket). Since no adapter has the IPv6 address
> returned,  bind(2) would fail on a subsequent call.

Yes, musl seems to be behaving as expected here, and it's thrift
that's choosing to use the first v6 result, or the last result if
there are no v6 results. Even ignoring issues with inability to use
ipv6, this is probably not desirable on v4-only systems since it may
choose the worst, rather than the best, result, if the order was
meaningful to begin with.

> After applying the patch [1] to musl,
> 
> # ./test -v
> struct addrinfo {
>    ai_flags:        0
>    ai_family:       2
>    ai_socktype:     1
>    ai_protocol:     6
>    ai_addrlen:      16
>    ai_addr:
>       family:       2
>       addr:         127.0.0.1
>       port:         0
>    ai_canonname:    localhost
>    ai_next:         0
> }
> 
> [1] https://patch-diff.githubusercontent.com/raw/cfriedt/musl/pull/1.diff

This patch isn't acceptable as-is, and I don't think it would even fix
your problem if you just did "ip addr del ::1 dev lo" but had an ipv6
address available on some other interface. The only thing that would
help is having AI_ADDRCONFIG omit non-routable results, which is
"morally" what it should do, but contrary to what it's specified to
do.

Maybe you're okay with saying "having some ipv6 addresses but not
supporting ::1 is a pathological configuration and doesn't need to be
supported". If so, I'm happy to have AI_ADDRCONFIG check whether ::1
is routable, and disable IPv6 if it's not. This would basically look
like:

	if (family != AF_INET && (flags & AI_ADDRCONFIG)) {
		/* attempt to connect a UDP socket to ::1 */
		/* ... */
		if (failed && errno==ENETUNREACH) {
			if (family == AF_INET6) return EAI_NONAME;
			family = AF_INET;
		}
	}

Regardless of what's done on the musl side, I think it would make
sense for thrift to change its strategy for selecting an address to
use. If IPv6 is supported, getaddrinfo should always return ::1 before
127.0.0.1 (see RFC 3484/6724), so just using the first address
returned should give the best behavior. For addresses other than
localhost, I'm not sure what you want to happen, but I think if the
hostname has both IPv4 and IPv6 addresses, just binding to the v6 one
will _not_ get you the ability to accept v4 connections (since the
address is different) and you need to bind to both. In general the
correct pattern for listening with getaddrinfo is probably to attempt
to bind each of the results.

Rich

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Christopher Friedt]

On Tue, Jul 10, 2018 at 8:38 PM Rich Felker <dalias@libc.org> wrote:
> Regardless of what's done on the musl side, I think it would make
> sense for thrift to change its strategy for selecting an address to

Let's take thrift out of the equation and read the POSIX spec.
Directrly from [1]:

"If the AI_ADDRCONFIG flag is specified, IPv4 addresses shall be
returned only if an IPv4 address is configured on the local system,
and IPv6 addresses shall be returned only if an IPv6 address is
configured on the local system."

There are precisely zero IPv6 addresses configured on the local
system. There is precisely two IPv4 address configured on the local
system (127.0.0.1, and 172.17.0.2 in this case).

Regardless of whether loopbacks are ignored, you are breaking POSIX
spec with your current implementation. Period.

Regards,

C

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/getaddrinfo.html

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Rich Felker]

On Tue, Jul 10, 2018 at 09:01:19PM -0400, Christopher Friedt wrote:
> On Tue, Jul 10, 2018 at 8:38 PM Rich Felker <dalias@libc.org> wrote:
> > Regardless of what's done on the musl side, I think it would make
> > sense for thrift to change its strategy for selecting an address to
> 
> Let's take thrift out of the equation and read the POSIX spec.
> Directrly from [1]:
> 
> "If the AI_ADDRCONFIG flag is specified, IPv4 addresses shall be
> returned only if an IPv4 address is configured on the local system,
> and IPv6 addresses shall be returned only if an IPv6 address is
> configured on the local system."
> 
> There are precisely zero IPv6 addresses configured on the local
> system. There is precisely two IPv4 address configured on the local
> system (127.0.0.1, and 172.17.0.2 in this case).
> 
> Regardless of whether loopbacks are ignored, you are breaking POSIX
> spec with your current implementation. Period.

What you're asking for does not solve your problem, but does solve a
particular special case of it, which is why I asked if you're happy
with that and suggested that there's probably additional stuff to do
on your side. But I'm happy to put that aside and focus just on musl.

The patch you provided mimics the glibc behavior and does not give
results any more conformant than the current musl behavior; it's just
differently non-conformant. In particular it will wrongly suppress
IPv6 results when the only interfaces with v6 addresses are loopback.

With the current musl behavior, there is only a conformance question
at all if no-IPv6 (i.e. not even loopback) is a supported
configuration. It probably should be, since some embedded (and as you
mentioned now, container) users setup environments where v6 is not
supported.

Pulling in large amounts of additional code and O(n) runtime cost
(some container users have hundreds or even thousands of interfaces)
for AI_ADDRCONFIG is not an acceptable appproach to this, and I don't
think it gives meaningfully-better behavior than just probing
routability of ::1. Are you asking that "no IPv6 on loopback, but IPv6
present on other interfaces" be a supported configuration? Or
something else?

Rich

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Christopher Friedt]

On Tue, Jul 10, 2018 at 9:26 PM Rich Felker <dalias@libc.org> wrote:
> Pulling in large amounts of additional code and O(n) runtime cost

Latest patch [1] addresses

1) not ignoring loopback
2) using routability of udp packets vs O(n) lookup on network interfaces

Any other concerns, Rich?

[1]
https://github.com/cfriedt/musl/pull/1
https://github.com/cfriedt/musl/pull/1.diff

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]

On Wed, Jul 11, 2018 at 06:12:31AM -0400, Christopher Friedt wrote:
> On Tue, Jul 10, 2018 at 9:26 PM Rich Felker <dalias@libc.org> wrote:
> > Pulling in large amounts of additional code and O(n) runtime cost
> 
> Latest patch [1] addresses
> 
> 1) not ignoring loopback
> 2) using routability of udp packets vs O(n) lookup on network interfaces
> 
> Any other concerns, Rich?

You seem to have deleted the original patch and replaced it with a new
version. My first concern is *please* send all patches as attachments
to the list, not transient links like github or pastebins. Even if it
were still there now, it would likely not be there 5 years later when
someone is reading list archives. I'm attaching your v2 patch here now
for reference.

With that said, it still makes sprawling changes and intraduces a
gratuitous new file with external interface for something that
fundamentally takes only a few lines in one place and no external
interface at all. Formatting is also inconsistent with musl (spaces
after opening and before closing paren, etc.). And addition of the
nonstandard EAI_NODATA is an independent change that, if it makes
sense at all, needs to be discussed separately, and would need
corresponding changes elsewhere (e.g. gai_strerror).

Rich

[-- Attachment #2: v2.diff --]
[-- Type: text/plain, Size: 4683 bytes --]

diff --git a/include/netdb.h b/include/netdb.h
index adde2c5e..73d850f6 100644
--- a/include/netdb.h
+++ b/include/netdb.h
@@ -44,6 +44,7 @@ struct addrinfo {
 #define EAI_NONAME     -2
 #define EAI_AGAIN      -3
 #define EAI_FAIL       -4
+#define EAI_NODATA     -5
 #define EAI_FAMILY     -6
 #define EAI_SOCKTYPE   -7
 #define EAI_SERVICE    -8
diff --git a/src/network/getaddrinfo.c b/src/network/getaddrinfo.c
index b9439f77..2bfba18f 100644
--- a/src/network/getaddrinfo.c
+++ b/src/network/getaddrinfo.c
@@ -19,6 +19,7 @@ int getaddrinfo(const char *restrict host, const char *restrict serv, const stru
 			struct sockaddr_in6 sin6;
 		} sa;
 	} *out;
+	struct addrconfig addrconfig;
 
 	if (!host && !serv) return EAI_NONAME;
 
@@ -33,6 +34,10 @@ int getaddrinfo(const char *restrict host, const char *restrict serv, const stru
 		if ((flags & mask) != flags)
 			return EAI_BADFLAGS;
 
+		if (flags & AI_ADDRCONFIG) {
+			__lookup_addrconfig(&addrconfig);
+		}
+
 		switch (family) {
 		case AF_INET:
 		case AF_INET6:
@@ -61,30 +66,43 @@ int getaddrinfo(const char *restrict host, const char *restrict serv, const stru
 		outcanon = 0;
 	}
 
-	for (k=i=0; i<naddrs; i++) for (j=0; j<nservs; j++, k++) {
-		out[k].ai = (struct addrinfo){
-			.ai_family = addrs[i].family,
-			.ai_socktype = ports[j].socktype,
-			.ai_protocol = ports[j].proto,
-			.ai_addrlen = addrs[i].family == AF_INET
-				? sizeof(struct sockaddr_in)
-				: sizeof(struct sockaddr_in6),
-			.ai_addr = (void *)&out[k].sa,
-			.ai_canonname = outcanon,
-			.ai_next = &out[k+1].ai };
+	for (k=i=0; i<naddrs; i++) for (j=0; j<nservs; j++) {
 		switch (addrs[i].family) {
 		case AF_INET:
+			if ((flags & AI_ADDRCONFIG) && !addrconfig.af_inet) {
+				nais--;
+				continue;
+			}
 			out[k].sa.sin.sin_family = AF_INET;
 			out[k].sa.sin.sin_port = htons(ports[j].port);
 			memcpy(&out[k].sa.sin.sin_addr, &addrs[i].addr, 4);
 			break;
 		case AF_INET6:
+			if ((flags & AI_ADDRCONFIG ) && !addrconfig.af_inet6) {
+				nais--;
+				continue;
+			}
 			out[k].sa.sin6.sin6_family = AF_INET6;
 			out[k].sa.sin6.sin6_port = htons(ports[j].port);
 			out[k].sa.sin6.sin6_scope_id = addrs[i].scopeid;
 			memcpy(&out[k].sa.sin6.sin6_addr, &addrs[i].addr, 16);
-			break;			
+			break;
 		}
+		out[k].ai = (struct addrinfo){
+			.ai_family = addrs[i].family,
+			.ai_socktype = ports[j].socktype,
+			.ai_protocol = ports[j].proto,
+			.ai_addrlen = addrs[i].family == AF_INET
+				? sizeof(struct sockaddr_in)
+				: sizeof(struct sockaddr_in6),
+			.ai_addr = (void *)&out[k].sa,
+			.ai_canonname = outcanon,
+			.ai_next = &out[k+1].ai };
+		k++;
+	}
+	if ( nais < 1 ) {
+		free( out );
+		return EAI_NODATA;
 	}
 	out[nais-1].ai.ai_next = 0;
 	*res = &out->ai;
diff --git a/src/network/lookup.h b/src/network/lookup.h
index 0468edbc..32afd545 100644
--- a/src/network/lookup.h
+++ b/src/network/lookup.h
@@ -1,6 +1,7 @@
 #ifndef LOOKUP_H
 #define LOOKUP_H
 
+#include <stdbool.h>
 #include <stdint.h>
 #include <stddef.h>
 
@@ -30,9 +31,15 @@ struct resolvconf {
 #define MAXADDRS 48
 #define MAXSERVS 2
 
+struct addrconfig {
+	bool af_inet  : 1;
+	bool af_inet6 : 1;
+};
+
 int __lookup_serv(struct service buf[static MAXSERVS], const char *name, int proto, int socktype, int flags);
 int __lookup_name(struct address buf[static MAXADDRS], char canon[static 256], const char *name, int family, int flags);
 int __lookup_ipliteral(struct address buf[static 1], const char *name, int family);
+void __lookup_addrconfig( struct addrconfig *cfg );
 
 int __get_resolv_conf(struct resolvconf *, char *, size_t);
 
diff --git a/src/network/lookup_addrconfig.c b/src/network/lookup_addrconfig.c
new file mode 100644
index 00000000..427d9299
--- /dev/null
+++ b/src/network/lookup_addrconfig.c
@@ -0,0 +1,40 @@
+#include "lookup.h"
+
+#include <arpa/inet.h>
+#include <netinet/in.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <unistd.h>
+
+void __lookup_addrconfig( struct addrconfig *cfg ) {
+	int r;
+	int fd;
+
+	struct sockaddr_in sai = {
+		.sin_family = AF_INET,
+		.sin_port = htons( 42 ),
+		.sin_addr.s_addr = INADDR_LOOPBACK,
+	};
+	cfg->af_inet = false;
+	r = socket( AF_INET, SOCK_DGRAM, 0 );
+	if ( -1 != r ) {
+		fd = r;
+		r = connect( fd, (struct sockaddr *) & sai, sizeof( sai ) );
+		cfg->af_inet = 0 == r;
+		close( fd );
+	}
+
+	struct sockaddr_in6 sai6 = {
+		.sin6_family = AF_INET6,
+		.sin6_port = htons( 42 ),
+		.sin6_addr = IN6ADDR_LOOPBACK_INIT,
+	};
+	cfg->af_inet6 = false;
+	r = socket( AF_INET6, SOCK_DGRAM, 0 );
+	if ( -1 != r ) {
+		fd = r;
+		r = connect( fd, (struct sockaddr *) & sai6, sizeof( sai6 ) );
+		cfg->af_inet6 = 0 == r;
+		close( fd );
+	}
+}

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Christopher Friedt]

[-- Attachment #1: Type: text/plain, Size: 1203 bytes --]

On Wed, Jul 11, 2018, 12:44 PM Rich Felker, <dalias@libc.org> wrote:

> On Wed, Jul 11, 2018 at 06:12:31AM -0400, Christopher Friedt wrote:
> > On Tue, Jul 10, 2018 at 9:26 PM Rich Felker <dalias@libc.org> wrote:
> > > Pulling in large amounts of additional code and O(n) runtime cost
> >
> > Latest patch [1] addresses
> >
> > 1) not ignoring loopback
> > 2) using routability of udp packets vs O(n) lookup on network interfaces
> >
> > Any other concerns, Rich?
>
> You seem to have deleted the original patch and replaced it with a new
>


Some lists prefer patches to be inline. I wasn't sure if you wanted the
attachment. That clarifies that.

With that said, it still makes sprawling changes and intraduces a
> gratuitous new file with external interface for something that
> fundamentally takes only a few lines in one place and no external
> interface at all. Formatting is also


I'll put it directly inside of getaddrinfo.

inconsistent with musl (spaces
> after opening and before closing paren, etc.).


Ok, will reformat.

And addition of the
> nonstandard EAI_NODATA is an independent change that, if it makes
> sense at all, needs to be discussed


Right. What error would you prefer?

C

[-- Attachment #2: Type: text/html, Size: 2463 bytes --]

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Rich Felker]

On Wed, Jul 11, 2018 at 12:50:53PM -0400, Christopher Friedt wrote:
> On Wed, Jul 11, 2018, 12:44 PM Rich Felker, <dalias@libc.org> wrote:
> 
> > On Wed, Jul 11, 2018 at 06:12:31AM -0400, Christopher Friedt wrote:
> > > On Tue, Jul 10, 2018 at 9:26 PM Rich Felker <dalias@libc.org> wrote:
> > > > Pulling in large amounts of additional code and O(n) runtime cost
> > >
> > > Latest patch [1] addresses
> > >
> > > 1) not ignoring loopback
> > > 2) using routability of udp packets vs O(n) lookup on network interfaces
> > >
> > > Any other concerns, Rich?
> >
> > You seem to have deleted the original patch and replaced it with a new
> >
> 
> 
> Some lists prefer patches to be inline. I wasn't sure if you wanted the
> attachment. That clarifies that.
> 
> With that said, it still makes sprawling changes and intraduces a
> > gratuitous new file with external interface for something that
> > fundamentally takes only a few lines in one place and no external
> > interface at all. Formatting is also
> 
> 
> I'll put it directly inside of getaddrinfo.

It could probably go inside __lookup_name, but maybe in getaddrinfo is
better since that would avoid linking it for gethostbyname, etc.
(which don't need it).

> inconsistent with musl (spaces
> > after opening and before closing paren, etc.).
> 
> 
> Ok, will reformat.
> 
> And addition of the
> > nonstandard EAI_NODATA is an independent change that, if it makes
> > sense at all, needs to be discussed
> 
> 
> Right. What error would you prefer?

I think the one mandated by POSIX is EAI_NONAME ("The name does not
resolve for the supplied parameters").

Rich

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Christopher Friedt]

[-- Attachment #1: Type: text/plain, Size: 453 bytes --]

On Wed, Jul 11, 2018 at 1:00 PM Rich Felker <dalias@libc.org> wrote:
> It could probably go inside __lookup_name, but maybe in getaddrinfo is
> better since that would avoid linking it for gethostbyname, etc.
> (which don't need it).

Done

> > inconsistent with musl (spaces
> > > after opening and before closing paren, etc.).

Done

> I think the one mandated by POSIX is EAI_NONAME ("The name does not
> resolve for the supplied parameters").

Done

[-- Attachment #2: v3.diff --]
[-- Type: application/octet-stream, Size: 3981 bytes --]

diff --git a/src/network/getaddrinfo.c b/src/network/getaddrinfo.c
index b9439f77..72276ddc 100644
--- a/src/network/getaddrinfo.c
+++ b/src/network/getaddrinfo.c
@@ -1,8 +1,10 @@
+#include <stdbool.h>
 #include <stdlib.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
 #include <netdb.h>
 #include <string.h>
+#include <unistd.h>
 #include "lookup.h"
 
 int getaddrinfo(const char *restrict host, const char *restrict serv, const struct addrinfo *restrict hint, struct addrinfo **restrict res)
@@ -10,7 +12,7 @@ int getaddrinfo(const char *restrict host, const char *restrict serv, const stru
 	struct service ports[MAXSERVS];
 	struct address addrs[MAXADDRS];
 	char canon[256], *outcanon;
-	int nservs, naddrs, nais, canon_len, i, j, k;
+	int nservs, naddrs, nais, canon_len, i, j, k, fd, r;
 	int family = AF_UNSPEC, flags = 0, proto = 0, socktype = 0;
 	struct aibuf {
 		struct addrinfo ai;
@@ -19,6 +21,11 @@ int getaddrinfo(const char *restrict host, const char *restrict serv, const stru
 			struct sockaddr_in6 sin6;
 		} sa;
 	} *out;
+	struct addrconfig {
+		bool af_inet;
+		bool af_inet6;
+	} addrconfig;
+	struct sockaddr_storage sas;
 
 	if (!host && !serv) return EAI_NONAME;
 
@@ -33,6 +40,35 @@ int getaddrinfo(const char *restrict host, const char *restrict serv, const stru
 		if ((flags & mask) != flags)
 			return EAI_BADFLAGS;
 
+		if (flags & AI_ADDRCONFIG) {
+			*((struct sockaddr_in *)(&sas)) = (struct sockaddr_in){
+				.sin_family = AF_INET,
+				.sin_port = htons(42),
+				.sin_addr.s_addr = INADDR_LOOPBACK,
+			};
+			addrconfig.af_inet = false;
+			r = socket(AF_INET, SOCK_DGRAM, 0);
+			if (-1 != r) {
+				fd = r;
+				r = connect(fd, (struct sockaddr *) & sas, sizeof( struct sockaddr_in ));
+				addrconfig.af_inet = 0 == r;
+				close(fd);
+			}
+			*((struct sockaddr_in6 *)(&sas)) = (struct sockaddr_in6){
+				.sin6_family = AF_INET6,
+				.sin6_port = htons(42),
+				.sin6_addr = IN6ADDR_LOOPBACK_INIT,
+			};
+			addrconfig.af_inet6 = false;
+			r = socket(AF_INET6, SOCK_DGRAM, 0);
+			if (-1 != r) {
+				fd = r;
+				r = connect(fd, (struct sockaddr *) & sas, sizeof(struct sockaddr_in6));
+				addrconfig.af_inet6 = 0 == r;
+				close(fd);
+			}
+		}
+
 		switch (family) {
 		case AF_INET:
 		case AF_INET6:
@@ -61,30 +97,43 @@ int getaddrinfo(const char *restrict host, const char *restrict serv, const stru
 		outcanon = 0;
 	}
 
-	for (k=i=0; i<naddrs; i++) for (j=0; j<nservs; j++, k++) {
-		out[k].ai = (struct addrinfo){
-			.ai_family = addrs[i].family,
-			.ai_socktype = ports[j].socktype,
-			.ai_protocol = ports[j].proto,
-			.ai_addrlen = addrs[i].family == AF_INET
-				? sizeof(struct sockaddr_in)
-				: sizeof(struct sockaddr_in6),
-			.ai_addr = (void *)&out[k].sa,
-			.ai_canonname = outcanon,
-			.ai_next = &out[k+1].ai };
+	for (k=i=0; i<naddrs; i++) for (j=0; j<nservs; j++) {
 		switch (addrs[i].family) {
 		case AF_INET:
+			if ((flags & AI_ADDRCONFIG) && !addrconfig.af_inet) {
+				nais--;
+				continue;
+			}
 			out[k].sa.sin.sin_family = AF_INET;
 			out[k].sa.sin.sin_port = htons(ports[j].port);
 			memcpy(&out[k].sa.sin.sin_addr, &addrs[i].addr, 4);
 			break;
 		case AF_INET6:
+			if ((flags & AI_ADDRCONFIG ) && !addrconfig.af_inet6) {
+				nais--;
+				continue;
+			}
 			out[k].sa.sin6.sin6_family = AF_INET6;
 			out[k].sa.sin6.sin6_port = htons(ports[j].port);
 			out[k].sa.sin6.sin6_scope_id = addrs[i].scopeid;
 			memcpy(&out[k].sa.sin6.sin6_addr, &addrs[i].addr, 16);
-			break;			
+			break;
 		}
+		out[k].ai = (struct addrinfo){
+			.ai_family = addrs[i].family,
+			.ai_socktype = ports[j].socktype,
+			.ai_protocol = ports[j].proto,
+			.ai_addrlen = addrs[i].family == AF_INET
+				? sizeof(struct sockaddr_in)
+				: sizeof(struct sockaddr_in6),
+			.ai_addr = (void *)&out[k].sa,
+			.ai_canonname = outcanon,
+			.ai_next = &out[k+1].ai };
+		k++;
+	}
+	if ( nais < 1 ) {
+		free( out );
+		return EAI_NONAME;
 	}
 	out[nais-1].ai.ai_next = 0;
 	*res = &out->ai;

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 5714 bytes --]

On Wed, Jul 11, 2018 at 09:20:20PM -0400, Christopher Friedt wrote:
> On Wed, Jul 11, 2018 at 1:00 PM Rich Felker <dalias@libc.org> wrote:
> > It could probably go inside __lookup_name, but maybe in getaddrinfo is
> > better since that would avoid linking it for gethostbyname, etc.
> > (which don't need it).
> 
> Done
> 
> > > inconsistent with musl (spaces
> > > > after opening and before closing paren, etc.).
> 
> Done
> 
> > I think the one mandated by POSIX is EAI_NONAME ("The name does not
> > resolve for the supplied parameters").
> 
> Done

Something's not going right with our communication about this. I've
attached an untested patch that's closer to what I've been looking
for. It corrects an oversight I'd made, that we need to block
cancellation during the probe, and localizes the change as originally
requested. Please let me know if it works. Arguably it might be nicer
to replace the repeated code with a table and 2-iteration for loop.

Also:

> diff --git a/src/network/getaddrinfo.c b/src/network/getaddrinfo.c
> index b9439f77..72276ddc 100644
> --- a/src/network/getaddrinfo.c
> +++ b/src/network/getaddrinfo.c
> @@ -1,8 +1,10 @@
> +#include <stdbool.h>
>  #include <stdlib.h>
>  #include <sys/socket.h>
>  #include <netinet/in.h>
>  #include <netdb.h>
>  #include <string.h>
> +#include <unistd.h>
>  #include "lookup.h"
>  
>  int getaddrinfo(const char *restrict host, const char *restrict serv, const struct addrinfo *restrict hint, struct addrinfo **restrict res)
> @@ -10,7 +12,7 @@ int getaddrinfo(const char *restrict host, const char *restrict serv, const stru
>  	struct service ports[MAXSERVS];
>  	struct address addrs[MAXADDRS];
>  	char canon[256], *outcanon;
> -	int nservs, naddrs, nais, canon_len, i, j, k;
> +	int nservs, naddrs, nais, canon_len, i, j, k, fd, r;
>  	int family = AF_UNSPEC, flags = 0, proto = 0, socktype = 0;
>  	struct aibuf {
>  		struct addrinfo ai;
> @@ -19,6 +21,11 @@ int getaddrinfo(const char *restrict host, const char *restrict serv, const stru
>  			struct sockaddr_in6 sin6;
>  		} sa;
>  	} *out;
> +	struct addrconfig {
> +		bool af_inet;
> +		bool af_inet6;
> +	} addrconfig;

This struct, and use of bool, is completely gratuitous.

> +	struct sockaddr_storage sas;

Use of sockaddr_storage is pretty much always a bug.

>  	if (!host && !serv) return EAI_NONAME;
>  
> @@ -33,6 +40,35 @@ int getaddrinfo(const char *restrict host, const char *restrict serv, const stru
>  		if ((flags & mask) != flags)
>  			return EAI_BADFLAGS;
>  
> +		if (flags & AI_ADDRCONFIG) {
> +			*((struct sockaddr_in *)(&sas)) = (struct sockaddr_in){
> +				.sin_family = AF_INET,
> +				.sin_port = htons(42),
> +				.sin_addr.s_addr = INADDR_LOOPBACK,
> +			};

And indeed here undefined behavior is invoked by storing with an
lvalue whose type does not match the type of the object.

> +			addrconfig.af_inet = false;
> +			r = socket(AF_INET, SOCK_DGRAM, 0);
> +			if (-1 != r) {
> +				fd = r;
> +				r = connect(fd, (struct sockaddr *) & sas, sizeof( struct sockaddr_in ));
> +				addrconfig.af_inet = 0 == r;
> +				close(fd);
> +			}
> +			*((struct sockaddr_in6 *)(&sas)) = (struct sockaddr_in6){
> +				.sin6_family = AF_INET6,
> +				.sin6_port = htons(42),
> +				.sin6_addr = IN6ADDR_LOOPBACK_INIT,
> +			};
> +			addrconfig.af_inet6 = false;
> +			r = socket(AF_INET6, SOCK_DGRAM, 0);
> +			if (-1 != r) {
> +				fd = r;
> +				r = connect(fd, (struct sockaddr *) & sas, sizeof(struct sockaddr_in6));
> +				addrconfig.af_inet6 = 0 == r;
> +				close(fd);
> +			}
> +		}
> +
>  		switch (family) {
>  		case AF_INET:
>  		case AF_INET6:
> @@ -61,30 +97,43 @@ int getaddrinfo(const char *restrict host, const char *restrict serv, const stru
>  		outcanon = 0;
>  	}
>  
> -	for (k=i=0; i<naddrs; i++) for (j=0; j<nservs; j++, k++) {
> -		out[k].ai = (struct addrinfo){
> -			.ai_family = addrs[i].family,
> -			.ai_socktype = ports[j].socktype,
> -			.ai_protocol = ports[j].proto,
> -			.ai_addrlen = addrs[i].family == AF_INET
> -				? sizeof(struct sockaddr_in)
> -				: sizeof(struct sockaddr_in6),
> -			.ai_addr = (void *)&out[k].sa,
> -			.ai_canonname = outcanon,
> -			.ai_next = &out[k+1].ai };
> +	for (k=i=0; i<naddrs; i++) for (j=0; j<nservs; j++) {
>  		switch (addrs[i].family) {
>  		case AF_INET:
> +			if ((flags & AI_ADDRCONFIG) && !addrconfig.af_inet) {
> +				nais--;
> +				continue;
> +			}
>  			out[k].sa.sin.sin_family = AF_INET;
>  			out[k].sa.sin.sin_port = htons(ports[j].port);
>  			memcpy(&out[k].sa.sin.sin_addr, &addrs[i].addr, 4);
>  			break;
>  		case AF_INET6:
> +			if ((flags & AI_ADDRCONFIG ) && !addrconfig.af_inet6) {
> +				nais--;
> +				continue;
> +			}
>  			out[k].sa.sin6.sin6_family = AF_INET6;
>  			out[k].sa.sin6.sin6_port = htons(ports[j].port);
>  			out[k].sa.sin6.sin6_scope_id = addrs[i].scopeid;
>  			memcpy(&out[k].sa.sin6.sin6_addr, &addrs[i].addr, 16);
> -			break;			
> +			break;
>  		}
> +		out[k].ai = (struct addrinfo){
> +			.ai_family = addrs[i].family,
> +			.ai_socktype = ports[j].socktype,
> +			.ai_protocol = ports[j].proto,
> +			.ai_addrlen = addrs[i].family == AF_INET
> +				? sizeof(struct sockaddr_in)
> +				: sizeof(struct sockaddr_in6),
> +			.ai_addr = (void *)&out[k].sa,
> +			.ai_canonname = outcanon,
> +			.ai_next = &out[k+1].ai };
> +		k++;
> +	}
> +	if ( nais < 1 ) {
> +		free( out );
> +		return EAI_NONAME;
>  	}
>  	out[nais-1].ai.ai_next = 0;
>  	*res = &out->ai;

All of this is unnecessary, and fails the main legitimate purpose of
AI_ADDRCONFIG, which is suppressing DNS queries that are known not to
be needed. Instead family should simply be remapped before calling
__lookup_name, as I suggested a few emails back in this thread.

Rich

[-- Attachment #2: ai_addrconfig.diff --]
[-- Type: text/plain, Size: 1637 bytes --]

diff --git a/src/network/getaddrinfo.c b/src/network/getaddrinfo.c
index b9439f7..664a72a 100644
--- a/src/network/getaddrinfo.c
+++ b/src/network/getaddrinfo.c
@@ -3,6 +3,8 @@
 #include <netinet/in.h>
 #include <netdb.h>
 #include <string.h>
+#include <pthread.h>
+#include <unistd.h>
 #include "lookup.h"
 
 int getaddrinfo(const char *restrict host, const char *restrict serv, const struct addrinfo *restrict hint, struct addrinfo **restrict res)
@@ -43,6 +45,46 @@ int getaddrinfo(const char *restrict host, const char *restrict serv, const stru
 		}
 	}
 
+	if (flags & AI_ADDRCONFIG) {
+		int cs;
+		pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
+		if (family != AF_INET6) {
+			struct sockaddr_in sa4 = {
+				.sin_family = AF_INET,
+				.sin_port = 65535,
+				.sin_addr.s_addr = INADDR_LOOPBACK
+			};
+			int s = socket(AF_INET, SOCK_CLOEXEC|SOCK_DGRAM,
+				IPPROTO_UDP);
+			if (s>=0) {
+				int r = connect(s, (void *)&sa4, sizeof sa4);
+				close(s);
+				if (r) {
+					if (family == AF_INET) return EAI_NONAME;
+					family = AF_INET6;
+				}
+			}
+		}
+		if (family != AF_INET) {
+			struct sockaddr_in6 sa6 = {
+				.sin6_family = AF_INET6,
+				.sin6_port = 65535,
+				.sin6_addr = IN6ADDR_LOOPBACK_INIT
+			};
+			int s = socket(AF_INET6, SOCK_CLOEXEC|SOCK_DGRAM,
+				IPPROTO_UDP);
+			if (s>=0) {
+				int r = connect(s, (void *)&sa6, sizeof sa6);
+				close(s);
+				if (r) {
+					if (family == AF_INET6) return EAI_NONAME;
+					family = AF_INET;
+				}
+			}
+		}
+		pthread_setcancelstate(cs, 0);
+	}
+
 	nservs = __lookup_serv(ports, serv, proto, socktype, flags);
 	if (nservs < 0) return nservs;
 

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Christopher Friedt]

On Thu, Jul 12, 2018 at 9:49 PM Rich Felker <dalias@libc.org> wrote:
> Something's not going right with our communication about this. I've
> attached an untested patch that's closer to what I've been looking
> for. It corrects an oversight I'd made, that we need to block
> cancellation during the probe, and localizes the change as originally
> requested. Please let me know if it works. Arguably it might be nicer
> to replace the repeated code with a table and 2-iteration for loop.

I originally wrote my patch with the intention of being as unobtrusive
as possible but rather than disagree realized it was better to just do
what you wanted me to.

The struct was a better solution for when the addrconfig logic lived
in a separate function. It probably could have even been a separate
static function inside of getaddrinfo.c, but I anticipated that you
would not have liked that.

Definitely correct to disable pthread cancellation.

I used struct sockaddr_storage to avoid declaring more than one
sockaddr because I thought you would have preferred that. Personally,
I would have preferred to use two separate sockaddr too. Solves that
problem.

Originally, I wanted to use a loop over the length of a table, but
figured you would dislike that in favour of readability. Assuming
there will only be ever be AF_INET and AF_INET6 support for
getaddrinfo(3), handling it this or that way is fine.

The patch works for me as is or with the loop.

C

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 1894 bytes --]

On Thu, Jul 12, 2018 at 10:53:12PM -0400, Christopher Friedt wrote:
> On Thu, Jul 12, 2018 at 9:49 PM Rich Felker <dalias@libc.org> wrote:
> > Something's not going right with our communication about this. I've
> > attached an untested patch that's closer to what I've been looking
> > for. It corrects an oversight I'd made, that we need to block
> > cancellation during the probe, and localizes the change as originally
> > requested. Please let me know if it works. Arguably it might be nicer
> > to replace the repeated code with a table and 2-iteration for loop.
> 
> I originally wrote my patch with the intention of being as unobtrusive
> as possible but rather than disagree realized it was better to just do
> what you wanted me to.
> 
> The struct was a better solution for when the addrconfig logic lived
> in a separate function. It probably could have even been a separate
> static function inside of getaddrinfo.c, but I anticipated that you
> would not have liked that.
> 
> Definitely correct to disable pthread cancellation.
> 
> I used struct sockaddr_storage to avoid declaring more than one
> sockaddr because I thought you would have preferred that. Personally,
> I would have preferred to use two separate sockaddr too. Solves that
> problem.
> 
> Originally, I wanted to use a loop over the length of a table, but
> figured you would dislike that in favour of readability. Assuming
> there will only be ever be AF_INET and AF_INET6 support for
> getaddrinfo(3), handling it this or that way is fine.
> 
> The patch works for me as is or with the loop.

Here's a version with the loop. I've tested it now with ::1 removed
from device lo, but the connect to ::1 still succeeds; I suspect
presence of a default route for IPv6 makes it work since ::1 is
"routable" then. Can you confirm that it actually suppresses IPv6 in
your purely-no-IPv6 environment, as intended?

Rich

[-- Attachment #2: ai_addrconfig2.diff --]
[-- Type: text/plain, Size: 1558 bytes --]

diff --git a/src/network/getaddrinfo.c b/src/network/getaddrinfo.c
index b9439f7..91b4d30 100644
--- a/src/network/getaddrinfo.c
+++ b/src/network/getaddrinfo.c
@@ -3,6 +3,9 @@
 #include <netinet/in.h>
 #include <netdb.h>
 #include <string.h>
+#include <pthread.h>
+#include <unistd.h>
+#include <endian.h>
 #include "lookup.h"
 
 int getaddrinfo(const char *restrict host, const char *restrict serv, const struct addrinfo *restrict hint, struct addrinfo **restrict res)
@@ -43,6 +46,35 @@ int getaddrinfo(const char *restrict host, const char *restrict serv, const stru
 		}
 	}
 
+	if (flags & AI_ADDRCONFIG) {
+		static const struct sockaddr_in lo4 = {
+			.sin_family = AF_INET, .sin_port = 65535,
+			.sin_addr.s_addr = __BYTE_ORDER == __BIG_ENDIAN
+				? 0x7f000001 : 0x0100007f
+		};
+		static const struct sockaddr_in6 lo6 = {
+			.sin6_family = AF_INET6, .sin6_port = 65535,
+			.sin6_addr = IN6ADDR_LOOPBACK_INIT
+		};
+		int tf[2] = { AF_INET, AF_INET6 };
+		const void *ta[2] = { &lo4, &lo6 };
+		socklen_t tl[2] = { sizeof lo4, sizeof lo6 };
+		int cs;
+		pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
+		for (i=0; i<2; i++) {
+			if (family==tf[1-i]) continue;
+			int s = socket(tf[i], SOCK_CLOEXEC|SOCK_DGRAM,
+				IPPROTO_UDP);
+			if (s<0) continue;
+			int r = connect(s, ta[i], tl[i]);
+			close(s);
+			if (!r) continue;
+			if (family == tf[i]) return EAI_NONAME;
+			family = tf[1-i];
+		}
+		pthread_setcancelstate(cs, 0);
+	}
+
 	nservs = __lookup_serv(ports, serv, proto, socktype, flags);
 	if (nservs < 0) return nservs;
 

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Christopher Friedt]

On Fri, Jul 13, 2018 at 10:31 PM Rich Felker <dalias@libc.org> wrote:
> Here's a version with the loop. I've tested it now with ::1 removed
> from device lo, but the connect to ::1 still succeeds; I suspect
> presence of a default route for IPv6 makes it work since ::1 is
> "routable" then. Can you confirm that it actually suppresses IPv6 in
> your purely-no-IPv6 environment, as intended?

Works for me.

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Rich Felker]

On Sat, Jul 14, 2018 at 07:53:53PM -0400, Christopher Friedt wrote:
> On Fri, Jul 13, 2018 at 10:31 PM Rich Felker <dalias@libc.org> wrote:
> > Here's a version with the loop. I've tested it now with ::1 removed
> > from device lo, but the connect to ::1 still succeeds; I suspect
> > presence of a default route for IPv6 makes it work since ::1 is
> > "routable" then. Can you confirm that it actually suppresses IPv6 in
> > your purely-no-IPv6 environment, as intended?
> 
> Works for me.

Thanks!

Rich

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Rich Felker]

On Sat, Jul 14, 2018 at 08:07:17PM -0400, Rich Felker wrote:
> On Sat, Jul 14, 2018 at 07:53:53PM -0400, Christopher Friedt wrote:
> > On Fri, Jul 13, 2018 at 10:31 PM Rich Felker <dalias@libc.org> wrote:
> > > Here's a version with the loop. I've tested it now with ::1 removed
> > > from device lo, but the connect to ::1 still succeeds; I suspect
> > > presence of a default route for IPv6 makes it work since ::1 is
> > > "routable" then. Can you confirm that it actually suppresses IPv6 in
> > > your purely-no-IPv6 environment, as intended?
> > 
> > Works for me.
> 
> Thanks!

Minor issue: as written the patch left the cancel state disabled if it
exited early via return EAI_NONAME. Un-hoisting the setcancelstate
calls to fix.

Rich

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Rich Felker]

On Sat, Jul 14, 2018 at 08:19:50PM -0400, Rich Felker wrote:
> On Sat, Jul 14, 2018 at 08:07:17PM -0400, Rich Felker wrote:
> > On Sat, Jul 14, 2018 at 07:53:53PM -0400, Christopher Friedt wrote:
> > > On Fri, Jul 13, 2018 at 10:31 PM Rich Felker <dalias@libc.org> wrote:
> > > > Here's a version with the loop. I've tested it now with ::1 removed
> > > > from device lo, but the connect to ::1 still succeeds; I suspect
> > > > presence of a default route for IPv6 makes it work since ::1 is
> > > > "routable" then. Can you confirm that it actually suppresses IPv6 in
> > > > your purely-no-IPv6 environment, as intended?
> > > 
> > > Works for me.
> > 
> > Thanks!
> 
> Minor issue: as written the patch left the cancel state disabled if it
> exited early via return EAI_NONAME. Un-hoisting the setcancelstate
> calls to fix.

And a couple other issues: socket() may fail with EAFNOSUPPORT, and in
that case, the family needs to be rejected rather than accepted like
it was with the continue. For other socket() failures (like
EMFILE/ENFILE), the result is indeterminate and we need to return
EAI_SYSTEM rather than wrong results. Making these changes and I think
it will be ready to commit.

Rich

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Christopher Friedt]

[-- Attachment #1: Type: text/plain, Size: 467 bytes --]

On Sat, Jul 14, 2018, 8:53 PM Rich Felker, <dalias@libc.org> wrote:

> And a couple other issues: socket() may fail with EAFNOSUPPORT, and in
> that case, the family needs to be rejected rather than accepted like
> it was with the continue. For other socket() failures (like
> EMFILE/ENFILE), the result is indeterminate and we need to return
> EAI_SYSTEM rather than wrong results. Making these changes and I think
> it will be ready to commit.
>

Very thorough!

>

[-- Attachment #2: Type: text/html, Size: 947 bytes --]

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Christopher Friedt]

[-- Attachment #1: Type: text/plain, Size: 91 bytes --]

I could probably give it another spin with the suggestions you made if
you're preoccupied.

[-- Attachment #2: Type: text/html, Size: 498 bytes --]

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Rich Felker]

On Wed, Jul 18, 2018 at 08:14:22PM -0400, Christopher Friedt wrote:
> I could probably give it another spin with the suggestions you made if
> you're preoccupied.

Commit 187bcc3bf40bf187c5d76d206b04028fa8ca403b is upstream now based
on what we discussed. Let me know if you have any problems with it
still.

Rich

Re: getaddrinfo(3) / AI_ADDRCONFIG

[Christopher Friedt]

[-- Attachment #1: Type: text/plain, Size: 28 bytes --]

Awesome. I'll give it a go.

[-- Attachment #2: Type: text/html, Size: 54 bytes --]