* getaddrinfo(3) / AI_ADDRCONFIG @ 2018-07-09 15:16 Christopher Friedt 2018-07-09 22:38 ` Rich Felker 0 siblings, 1 reply; 28+ messages in thread From: Christopher Friedt @ 2018-07-09 15:16 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-09 15:16 getaddrinfo(3) / AI_ADDRCONFIG Christopher Friedt @ 2018-07-09 22:38 ` Rich Felker 2018-07-10 0:11 ` Christopher Friedt 0 siblings, 1 reply; 28+ messages in thread From: Rich Felker @ 2018-07-09 22:38 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-09 22:38 ` Rich Felker @ 2018-07-10 0:11 ` Christopher Friedt 2018-07-10 0:59 ` Rich Felker 0 siblings, 1 reply; 28+ messages in thread From: Christopher Friedt @ 2018-07-10 0:11 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-10 0:11 ` Christopher Friedt @ 2018-07-10 0:59 ` Rich Felker 2018-07-10 12:05 ` Christopher Friedt 0 siblings, 1 reply; 28+ messages in thread From: Rich Felker @ 2018-07-10 0:59 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-10 0:59 ` Rich Felker @ 2018-07-10 12:05 ` Christopher Friedt 2018-07-10 15:08 ` Rich Felker 0 siblings, 1 reply; 28+ messages in thread From: Christopher Friedt @ 2018-07-10 12:05 UTC (permalink / raw) To: musl [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-10 12:05 ` Christopher Friedt @ 2018-07-10 15:08 ` Rich Felker 2018-07-10 23:21 ` Christopher Friedt 0 siblings, 1 reply; 28+ messages in thread From: Rich Felker @ 2018-07-10 15:08 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-10 15:08 ` Rich Felker @ 2018-07-10 23:21 ` Christopher Friedt 2018-07-10 23:30 ` Christopher Friedt 0 siblings, 1 reply; 28+ messages in thread From: Christopher Friedt @ 2018-07-10 23:21 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-10 23:21 ` Christopher Friedt @ 2018-07-10 23:30 ` Christopher Friedt 2018-07-10 23:42 ` Christopher Friedt 2018-07-11 0:38 ` Rich Felker 0 siblings, 2 replies; 28+ messages in thread From: Christopher Friedt @ 2018-07-10 23:30 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-10 23:30 ` Christopher Friedt @ 2018-07-10 23:42 ` Christopher Friedt 2018-07-11 0:38 ` Rich Felker 1 sibling, 0 replies; 28+ messages in thread From: Christopher Friedt @ 2018-07-10 23:42 UTC (permalink / raw) To: musl 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-10 23:30 ` Christopher Friedt 2018-07-10 23:42 ` Christopher Friedt @ 2018-07-11 0:38 ` Rich Felker 2018-07-11 1:01 ` Christopher Friedt 1 sibling, 1 reply; 28+ messages in thread From: Rich Felker @ 2018-07-11 0:38 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-11 0:38 ` Rich Felker @ 2018-07-11 1:01 ` Christopher Friedt 2018-07-11 1:26 ` Rich Felker 0 siblings, 1 reply; 28+ messages in thread From: Christopher Friedt @ 2018-07-11 1:01 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-11 1:01 ` Christopher Friedt @ 2018-07-11 1:26 ` Rich Felker 2018-07-11 10:12 ` Christopher Friedt 0 siblings, 1 reply; 28+ messages in thread From: Rich Felker @ 2018-07-11 1:26 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-11 1:26 ` Rich Felker @ 2018-07-11 10:12 ` Christopher Friedt 2018-07-11 16:44 ` Rich Felker 0 siblings, 1 reply; 28+ messages in thread From: Christopher Friedt @ 2018-07-11 10:12 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-11 10:12 ` Christopher Friedt @ 2018-07-11 16:44 ` Rich Felker 2018-07-11 16:50 ` Christopher Friedt 0 siblings, 1 reply; 28+ messages in thread From: Rich Felker @ 2018-07-11 16:44 UTC (permalink / raw) To: musl [-- 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 ); + } +} ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-11 16:44 ` Rich Felker @ 2018-07-11 16:50 ` Christopher Friedt 2018-07-11 17:00 ` Rich Felker 0 siblings, 1 reply; 28+ messages in thread From: Christopher Friedt @ 2018-07-11 16:50 UTC (permalink / raw) To: musl [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-11 16:50 ` Christopher Friedt @ 2018-07-11 17:00 ` Rich Felker 2018-07-12 1:20 ` Christopher Friedt 0 siblings, 1 reply; 28+ messages in thread From: Rich Felker @ 2018-07-11 17:00 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-11 17:00 ` Rich Felker @ 2018-07-12 1:20 ` Christopher Friedt 2018-07-13 1:49 ` Rich Felker 0 siblings, 1 reply; 28+ messages in thread From: Christopher Friedt @ 2018-07-12 1:20 UTC (permalink / raw) To: musl [-- 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; ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-12 1:20 ` Christopher Friedt @ 2018-07-13 1:49 ` Rich Felker 2018-07-13 2:53 ` Christopher Friedt 0 siblings, 1 reply; 28+ messages in thread From: Rich Felker @ 2018-07-13 1:49 UTC (permalink / raw) To: musl [-- 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; ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-13 1:49 ` Rich Felker @ 2018-07-13 2:53 ` Christopher Friedt 2018-07-14 2:31 ` Rich Felker 0 siblings, 1 reply; 28+ messages in thread From: Christopher Friedt @ 2018-07-13 2:53 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-13 2:53 ` Christopher Friedt @ 2018-07-14 2:31 ` Rich Felker 2018-07-14 23:53 ` Christopher Friedt 0 siblings, 1 reply; 28+ messages in thread From: Rich Felker @ 2018-07-14 2:31 UTC (permalink / raw) To: musl [-- 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; ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-14 2:31 ` Rich Felker @ 2018-07-14 23:53 ` Christopher Friedt 2018-07-15 0:07 ` Rich Felker 0 siblings, 1 reply; 28+ messages in thread From: Christopher Friedt @ 2018-07-14 23:53 UTC (permalink / raw) To: musl 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-14 23:53 ` Christopher Friedt @ 2018-07-15 0:07 ` Rich Felker 2018-07-15 0:19 ` Rich Felker 0 siblings, 1 reply; 28+ messages in thread From: Rich Felker @ 2018-07-15 0:07 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-15 0:07 ` Rich Felker @ 2018-07-15 0:19 ` Rich Felker 2018-07-15 0:52 ` Rich Felker 0 siblings, 1 reply; 28+ messages in thread From: Rich Felker @ 2018-07-15 0:19 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-15 0:19 ` Rich Felker @ 2018-07-15 0:52 ` Rich Felker 2018-07-15 1:17 ` Christopher Friedt 0 siblings, 1 reply; 28+ messages in thread From: Rich Felker @ 2018-07-15 0:52 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-15 0:52 ` Rich Felker @ 2018-07-15 1:17 ` Christopher Friedt 2018-07-19 0:14 ` Christopher Friedt 0 siblings, 1 reply; 28+ messages in thread From: Christopher Friedt @ 2018-07-15 1:17 UTC (permalink / raw) To: musl [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-15 1:17 ` Christopher Friedt @ 2018-07-19 0:14 ` Christopher Friedt 2018-07-19 0:49 ` Rich Felker 0 siblings, 1 reply; 28+ messages in thread From: Christopher Friedt @ 2018-07-19 0:14 UTC (permalink / raw) To: musl [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-19 0:14 ` Christopher Friedt @ 2018-07-19 0:49 ` Rich Felker 2018-07-19 0:57 ` Christopher Friedt 0 siblings, 1 reply; 28+ messages in thread From: Rich Felker @ 2018-07-19 0:49 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: getaddrinfo(3) / AI_ADDRCONFIG 2018-07-19 0:49 ` Rich Felker @ 2018-07-19 0:57 ` Christopher Friedt 0 siblings, 0 replies; 28+ messages in thread From: Christopher Friedt @ 2018-07-19 0:57 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 28 bytes --] Awesome. I'll give it a go. [-- Attachment #2: Type: text/html, Size: 54 bytes --] ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2018-07-19 0:57 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-09 15:16 getaddrinfo(3) / AI_ADDRCONFIG Christopher Friedt 2018-07-09 22:38 ` Rich Felker 2018-07-10 0:11 ` Christopher Friedt 2018-07-10 0:59 ` Rich Felker 2018-07-10 12:05 ` Christopher Friedt 2018-07-10 15:08 ` Rich Felker 2018-07-10 23:21 ` Christopher Friedt 2018-07-10 23:30 ` Christopher Friedt 2018-07-10 23:42 ` Christopher Friedt 2018-07-11 0:38 ` Rich Felker 2018-07-11 1:01 ` Christopher Friedt 2018-07-11 1:26 ` Rich Felker 2018-07-11 10:12 ` Christopher Friedt 2018-07-11 16:44 ` Rich Felker 2018-07-11 16:50 ` Christopher Friedt 2018-07-11 17:00 ` Rich Felker 2018-07-12 1:20 ` Christopher Friedt 2018-07-13 1:49 ` Rich Felker 2018-07-13 2:53 ` Christopher Friedt 2018-07-14 2:31 ` Rich Felker 2018-07-14 23:53 ` Christopher Friedt 2018-07-15 0:07 ` Rich Felker 2018-07-15 0:19 ` Rich Felker 2018-07-15 0:52 ` Rich Felker 2018-07-15 1:17 ` Christopher Friedt 2018-07-19 0:14 ` Christopher Friedt 2018-07-19 0:49 ` Rich Felker 2018-07-19 0:57 ` Christopher Friedt
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).