net: Allow -proxy=[::1] on nodes with IPV6 lo only

m3dwards commented at 12:53 pm on June 7, 2024: contributor

This is similar to (but does not fix) #13155 which I believe is the same issue but in libevent.

The issue is on a host that has IPV6 enabled but only a loopback IP address -proxy=[::1] will fail as [::1] is not considered valid by getaddrinfo with AI_ADDRCONFIG flag. I think the loopback interface should be considered valid and we have a functional test that will try to test this: feature_proxy.py.

To replicate the issue, run feature_proxy.py inside a docker container that has IPV6 loopback ::1 address without specifically giving that container an external IPV6 address. This should be the default with recent versions of docker. IPV6 on loopback interface was enabled in docker engine 26 and later (https://docs.docker.com/engine/release-notes/26.0/#bug-fixes-and-enhancements-2).

AI_ADDRCONFIG was introduced to prevent slow DNS lookups on systems that were IPV4 only.

References:

Man section on AI_ADDRCONFIG:

0If 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
1       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 ex‐
2       ample, IPv4-only systems, to ensure that getaddrinfo() does not return IPv6 socket addresses that would always fail in connect(2) or bind(2).

AI_ADDRCONFIG considered harmful Wiki entry by Fedora

Mozilla discussing slow DNS without AI_ADDRCONFIG and also localhost issues with it

DrahtBot commented at 12:53 pm on June 7, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	tdb3, pinheadmz, achow101
Stale ACK	vasild

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label P2P on Jun 7, 2024

in src/netbase.cpp:57 in 84e63f34af outdated

53@@ -54,7 +54,7 @@ std::vector<CNetAddr> WrappedGetAddrInfo(const std::string& name, bool allow_loo
54     // If we don't allow lookups, then use the AI_NUMERICHOST flag for
55     // getaddrinfo to only decode numerical network addresses and suppress
56     // hostname lookups.
57-    ai_hint.ai_flags = allow_lookup ? AI_ADDRCONFIG : AI_NUMERICHOST;

vasild commented at 9:38 am on June 14, 2024:

There is a comment above which has to be removed if this patch makes it as it is:

0// If we allow lookups of hostnames, use the AI_ADDRCONFIG flag to only
1// return addresses whose family we have an address configured for.

vasild commented at 11:35 am on June 14, 2024: contributor

Concept ACK on allowing usage of [::1] on nodes with IPv6 lo only.

A workaround is to use -dns=0 (which will avoid AI_ADDRCONFIG but will make it impossible to use hostnames).

I can reproduce the problem on Linux. FreeBSD treats the loopback address as valid, so this problem is non-existent on FreeBSD.

I think there is a merit in this for our use cases of getaddrinfo() / Lookup*():

IPv4 addresses are returned in the list pointed to by res only if the local system has at least one IPv4 address configured … (and same for IPv6)

But not this:

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

This is even bogus to some extent - if the system has only [::1] configured and we want to connect(2) or bind(2) to [::1] that will work. So the “always” is too strong / untrue.

Now, can we have the AI_ADDRCONFIG behavior but get it to consider loopback addresses as valid? For example, after running getaddrinfo() with AI_ADDRCONFIG run it again without AI_ADDRCONFIG and append any loopback addresses from the second run to the results?

m3dwards commented at 4:39 pm on June 14, 2024: contributor

Now, can we have the AI_ADDRCONFIG behavior but get it to consider loopback addresses as valid? For example, after running getaddrinfo() with AI_ADDRCONFIG run it again without AI_ADDRCONFIG and append any loopback addresses from the second run to the results?

I like this idea, I’ll have a go at implementing it.

m3dwards force-pushed on Jun 18, 2024

in src/netbase.cpp:63 in d1337d575c outdated

61+
62     addrinfo* ai_res{nullptr};
63-    const int n_err{getaddrinfo(name.c_str(), nullptr, &ai_hint, &ai_res)};
64-    if (n_err != 0) {
65-        return {};
66-    }

m3dwards commented at 10:00 am on June 18, 2024:

I removed the error handling here for two reasons:

Firstly it just returns an empty vector which is what I believe this function will do anyway if it fails to get address info.

Secondly, if getaddrinfo doesn’t allow loopback only IPV6 then that is returned as an error code 1 “address family for hostname not supported” so this specific case would have to be allowed.

vasild commented at 10:57 am on June 21, 2024:

Are you sure that getaddrinfo() would return error (ie n_err != 0) and still set something useful in ai_res? That sounds strange.

I am worried that it may return an error (for whatever reason) and set ai_res to a bogus pointer which we later try to dereference.

Just returning from the lambda and not from WrappedGetAddrInfo() if getaddrinfo() returns an error seems reasonable to me.

m3dwards commented at 4:08 pm on June 21, 2024:

It was working but I agree handling the error is safer.

Also I was wrong that it would need to handle the specific “address family for hostname not supported” error.

Implemented your suggestion of just returning from the lambda.

m3dwards commented at 10:28 am on June 18, 2024: contributor

Pushed new approach of calling getaddrinfo twice. First pass is unchanged from original behaviour, second pass adds local IP addresses should they have not been found on the first pass.

m3dwards force-pushed on Jun 18, 2024

DrahtBot added the label CI failed on Jun 18, 2024

DrahtBot removed the label CI failed on Jun 19, 2024

in src/netbase.cpp:64 in a7cbc27101 outdated

60+
61+    // getaddrinfo is called twice and to prevent duplicates a set is used
62+    // std::set used over a std::unordered_set as CNetAddr does not have a hash function
63+    std::set<CNetAddr> resolved_addresses_set;
64+
65+    auto callgetaddrinfo = [&](const int flags, const bool only_add_local_addr) {

vasild commented at 10:26 am on June 21, 2024:

nit: function arguments that are passed by value need not be const

0    auto callgetaddrinfo = [&](int flags, bool only_add_local_addr) {

http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-in

in src/netbase.cpp:82 in a7cbc27101 outdated

78+            if (ai_trav->ai_family == AF_INET6) {
79+                assert(ai_trav->ai_addrlen >= sizeof(sockaddr_in6));
80+                const sockaddr_in6* s6{reinterpret_cast<sockaddr_in6*>(ai_trav->ai_addr)};
81+                addr = CNetAddr{s6->sin6_addr, s6->sin6_scope_id};
82+            }
83+            if (!only_add_local_addr || (only_add_local_addr && addr.IsLocal())) {

vasild commented at 10:40 am on June 21, 2024:

nit, feel free to ignore if you find the current one more readable, but this is equivalent to:

0            if (!only_add_local_addr || addr.IsLocal()) {

in src/netbase.cpp:93 in a7cbc27101 outdated

119+    for (auto it = resolved_addresses_set.begin(); it != resolved_addresses_set.end(); ) {
120+        resolved_addresses.push_back(std::move(resolved_addresses_set.extract(it++).value()));
121     }
122-    freeaddrinfo(ai_res);
123 
124     return resolved_addresses;

vasild commented at 10:51 am on June 21, 2024:

Simpler:

0-    // Convert the set to a vector
1-    std::vector<CNetAddr> resolved_addresses;
2-    resolved_addresses.reserve(resolved_addresses_set.size());
3-    for (auto it = resolved_addresses_set.begin(); it != resolved_addresses_set.end(); ) {
4-        resolved_addresses.push_back(std::move(resolved_addresses_set.extract(it++).value()));
5-    }
6-
7-    return resolved_addresses;
8+    return {resolved_addresses_set.begin(), resolved_addresses_set.end()};

vasild commented at 10:52 am on June 21, 2024: contributor

Approach ACK a7cbc27101677ee5ff357da9595f386b03b4756d

m3dwards force-pushed on Jun 21, 2024

m3dwards commented at 4:10 pm on June 21, 2024: contributor

Thanks for your review @vasild, should have addressed all of your comments.

vasild approved

vasild commented at 3:31 pm on June 25, 2024: contributor

ACK 60a753b7b38fbe89494f8df66f13a84f28af244b

Thank you!

in src/netbase.cpp:97 in 60a753b7b3 outdated

112+
113+    callgetaddrinfo(ai_flags, false);
114+
115+    // AI_ADDRCONFIG on some systems may exclude loopback only addresses, especially IPV6
116+    // We perform a second lookup with ai_flags set to 0 and add local addresses to the set
117+    if (ai_flags == AI_ADDRCONFIG) {

pinheadmz commented at 3:23 pm on July 1, 2024:

Might consider using & here since we’re dealing with bitfield flags and not absolute values

in src/netbase.cpp:69 in 60a753b7b3 outdated

79+    auto callgetaddrinfo = [&](int flags, bool only_add_local_addr) {
80+        ai_hint.ai_flags = flags;
81+        addrinfo* ai_res{nullptr};
82+        const int n_err{getaddrinfo(name.c_str(), nullptr, &ai_hint, &ai_res)};
83+        if (n_err != 0) {
84+            return;

pinheadmz commented at 4:35 pm on July 1, 2024:

You could check specifically for EAI_FAMILY and then only recheck without AI_ADDRCONFIG in that condition. I think the code you have now will always check everything twice, but ignore duplicates?

tdb3 commented at 2:19 am on July 2, 2024:

This comment from @pinheadmz about checking for the specific error we care about makes sense to me as well. It might make for a significantly smaller code change and prevent extraneous calls to getaddrinfo.

m3dwards commented at 12:00 pm on July 4, 2024:

I looked into catching specifically EAI_ADDRFAMILY errors but this only applies on GNU Libc and is not POSIX. I checked windows and they have different errors. Rather than try to catalogue every possible system and libc implementation, the code now retries on any error (assuming you first tried with AI_ADDRCONFIG in the first place).

pinheadmz approved

pinheadmz commented at 4:40 pm on July 1, 2024: member

ACK 60a753b7b38fbe89494f8df66f13a84f28af244b

Tested in an ubuntu docker container with local ipv6 but not external ipv6:

0# ip address | grep inet 
1    inet 127.0.0.1/8 scope host lo
2    inet6 ::1/128 scope host 
3    inet 172.17.0.2/16 brd 172.17.255.255 scope global eth0

Confirmed the feature_proxy test fails without the patch. Added extra logging to confirm the error:

0getaddrinfo: ::1 error: Address family for hostname not supported

With the patch, getaddrinfo is called a second time with flags=0 and the call succeeds.

A few questions below.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 60a753b7b38fbe89494f8df66f13a84f28af244b
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmaC23IACgkQ5+KYS2KJ
 7yTrxpQ/8Cz+gj6bv5wNZ/Nuf3N4uV1I2xScdU8LEjt9RfkppplQaDvExq7Bv3rBt
 87eSOeuODMWmy+yjSS69NYcuHIg79mDWHNz7XVFX2uqQ0dpix+CaQPYvGDMfiOYR0
 9XbXNTcx8tCID+TdvPE1QC4iLQitjwwAtD8c9CNupzEy0r5z5JtyxMjUCWcB68LJa
10NvKDjRQtO/+D4I7uv0U0Mq+lDFloRYr6byWP9US6Z/mBeBdlvev/82+jkrhstB3q
11tYspNR725QBxh5rLjgxoBGjPmjJfyLBpwCGqGFzLVI8BM1oGaa/YYFLE9NK0WRuk
128xKAbXwIu7CkjF859vlgthzROJPq3FHcK/8T9wZFpv6QvDZbaaN4WwpDjI8zFgqI
13BH658s+7yNlrjM6JJL5qHD71h3bztvYp6EpzUD1eTqHTY9OumDPc7av4E2ooDDFv
14a3xjokiP1JB4qmjqkXExqwdPQ8WayXAOolMAHCu7+IqvhWjzSFBoovbdpChz/kIX
15F3fJXbF1qEM8ybjk0Oroja6jVIeCjdKlDajR3ZUmFevkTTMRy7kDPktlbKXKBFz7
16uItf5JZ94aQg9TobJLbKXAkTUeFY57rt1Ucxn0vs7zGJQCLVloxOXNhc13FX8iD8
17KoUULcryDATRMNxKWieBKMA7RnEOAiJjEZmj47X73pnuxsIL9kc=
18=zVvS
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

in src/netbase.cpp:98 in 60a753b7b3 outdated

113+    callgetaddrinfo(ai_flags, false);
114+
115+    // AI_ADDRCONFIG on some systems may exclude loopback only addresses, especially IPV6
116+    // We perform a second lookup with ai_flags set to 0 and add local addresses to the set
117+    if (ai_flags == AI_ADDRCONFIG) {
118+        callgetaddrinfo(0, true);

tdb3 commented at 2:24 am on July 2, 2024:

Instead of using 0 as the first argument, might be better to remove only AI_ADDRCONFIG, e.g. something like

0callgetaddrinfo(ai_flags & (~AI_ADDRCONFIG), true);

m3dwards commented at 1:51 pm on July 3, 2024:

ai_flags only had AI_ADDRCONFIG set so without it it would be 0 anyway right?

tdb3 commented at 10:30 pm on July 3, 2024:

Yeah, that’s true for the time being. The thought here is to increase resilience in the code, so that if ai_flags changes we’re not hard-coded to 0. Seems like the scope of this if condition is intended as “if AI_ADDRCONFIG set” rather than “is only AI_ADDRCONFIG set” (which is now updated in f95fb4a1d826765553f3319ee6f1024094954073). I’m thinking we would want callgetaddrinfo() to use similar logic (i.e. focus on the specific flag of interest and keep existing flags defined except for AI_ADDRCONFIG).

m3dwards commented at 12:01 pm on July 4, 2024:

Good suggestion thanks, implemented.

tdb3 commented at 2:27 am on July 2, 2024: contributor

Concept ACK Good find. Left a couple of comments.

m3dwards force-pushed on Jul 3, 2024

DrahtBot commented at 3:44 pm on July 3, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/27000088118

DrahtBot added the label CI failed on Jul 3, 2024

m3dwards force-pushed on Jul 3, 2024

m3dwards force-pushed on Jul 4, 2024

DrahtBot removed the label CI failed on Jul 4, 2024

m3dwards commented at 8:52 am on July 9, 2024: contributor

@pinheadmz @tdb3 I should have addressed all of your comments. @vasild the change since your review is rather than always check twice and discard duplicates, we now only check on error. I chose not to look for the specific error as it was not POSIX and was different on different platforms.

in src/netbase.cpp:62 in e047f4e166 outdated

70-    while (ai_trav != nullptr) {
71-        if (ai_trav->ai_family == AF_INET) {
72-            assert(ai_trav->ai_addrlen >= sizeof(sockaddr_in));
73-            resolved_addresses.emplace_back(reinterpret_cast<sockaddr_in*>(ai_trav->ai_addr)->sin_addr);
74+
75+    auto callgetaddrinfo = [&](int flags) -> int {

tdb3 commented at 10:12 pm on July 9, 2024:

Unless I’m overlooking something, might be able to simplify/shorten the change by calling getaddrinfo() again just when needed. ai_res should be set up leaving the if either way (or we return an empty vector). Would avoid the lambda function and might also get rid of the copy/standalone ai_flags.

Here’s a commit on a fork: https://github.com/bitcoin/bitcoin/commit/bba1b42a8e533aa33319c7c9ca471dc0fab2ddd8

 0diff --git a/src/netbase.cpp b/src/netbase.cpp
 1index e231766487..8914bbb2c4 100644
 2--- a/src/netbase.cpp
 3+++ b/src/netbase.cpp
 4@@ -59,7 +59,17 @@ std::vector<CNetAddr> WrappedGetAddrInfo(const std::string& name, bool allow_loo
 5     addrinfo* ai_res{nullptr};
 6     const int n_err{getaddrinfo(name.c_str(), nullptr, &ai_hint, &ai_res)};
 7     if (n_err != 0) {
 8-        return {};
 9+        if ((ai_hint.ai_flags & AI_ADDRCONFIG) == AI_ADDRCONFIG) {
10+            // AI_ADDRCONFIG on some systems may exclude loopback-only addresses
11+            // If first lookup failed we perform a second lookup without AI_ADDRCONFIG
12+            ai_hint.ai_flags = (ai_hint.ai_flags & ~AI_ADDRCONFIG);
13+            const int n_err_retry{getaddrinfo(name.c_str(), nullptr, &ai_hint, &ai_res)};
14+            if (n_err_retry != 0) {
15+                return {};
16+            }
17+        } else {
18+            return {};
19+        }
20     }
21 
22     // Traverse the linked list starting with ai_trav.

Note: I did not heavily test this yet.

m3dwards commented at 5:18 pm on July 11, 2024:

If pursuing this approach do we think it would be prudent to reset ai_res to a nullptr before the second call?

tdb3 commented at 11:52 pm on July 11, 2024:

Good thought. At least from an initial look, makes sense to check if it’s already null and if not call freeaddrinfo() (getaddrinfo() handles allocation and freeaddrinfo() handles cleanup). I suspect this could be somewhat overkill (or not happen if unsuccessful getaddrinfo() never sets ai_res to non-null), but we don’t want a memory leak. I haven’t seen concrete guarantees yet in documentation (and we’d need to potentially compare guarantees for different platforms), so the extra check seems safer.

As a side note, freeaddrinfo(NULL) doesn’t crash/segfault (at least on Debian/glibc), so the null check before freeaddrinfo() might be redundant (but defensive).

m3dwards commented at 9:39 am on July 12, 2024:

I think it’s overkill. In all the examples and documentation I’ve read, freeaddrinfo() is only called when getaddrinfo() succeeds.

tdb3 commented at 3:37 pm on July 12, 2024:

Probably, but the examples I’ve seen are all typically small and exit the program if getaddrinfo() returns non-zero.

tdb3 commented at 10:12 pm on July 9, 2024: contributor

I think this is really close. Left one more comment.

net: Allow DNS lookups on nodes with IPV6 lo only

AI_ADDRCONFIG prevents ::1 from being considered a valid address on hosts that have a IPV6 loopback IP address but no other IPV6 interfaces.

23333b7ed2

m3dwards force-pushed on Jul 15, 2024

tdb3 approved

tdb3 commented at 4:16 pm on July 15, 2024: contributor

ACK 23333b7ed243071c9b4e4f04c727556d8065acbb

Tested with a similar approach to #30245#pullrequestreview-2151672225

Created an LXC container with an external and loopback IPv4 address, and with a loopback IPv6 address.

0root@test30245:~/bitcoin# ip a | grep inet
1    inet 127.0.0.1/8 scope host lo
2    inet6 ::1/128 scope host noprefixroute 
3    inet 192.168.10.81/24 brd 192.168.10.255 scope global eth0

Inserted some extra log statments:

02024-07-15T12:06:09Z =============First try
12024-07-15T12:06:09Z =============n_err != 0
22024-07-15T12:06:09Z =============Trying a second time
32024-07-15T12:06:09Z =============Second time worked!

Appeared to work well.

Also synced approx 8000 blocks on signet with a tor proxy set up to listen on [::1]:9050 (-proxy=[::1] with bitcoind).

DrahtBot requested review from pinheadmz on Jul 15, 2024

DrahtBot requested review from vasild on Jul 15, 2024

m3dwards commented at 6:26 pm on July 15, 2024: contributor

@pinheadmz @vasild

Ready for you to take a second look if you have any time.

in src/netbase.cpp:66 in 23333b7ed2

59@@ -59,7 +60,17 @@ std::vector<CNetAddr> WrappedGetAddrInfo(const std::string& name, bool allow_loo
60     addrinfo* ai_res{nullptr};
61     const int n_err{getaddrinfo(name.c_str(), nullptr, &ai_hint, &ai_res)};
62     if (n_err != 0) {
63-        return {};
64+        if ((ai_hint.ai_flags & AI_ADDRCONFIG) == AI_ADDRCONFIG) {
65+            // AI_ADDRCONFIG on some systems may exclude loopback-only addresses
66+            // If first lookup failed we perform a second lookup without AI_ADDRCONFIG
67+            ai_hint.ai_flags = (ai_hint.ai_flags & ~AI_ADDRCONFIG);

pinheadmz commented at 7:44 pm on July 15, 2024:

If you need to retouch, you might consider using a local addrinfo variable for the second call since it serves a specific purpose only inside this if condition. It doesn’t matter because it’s not like it gets used again later, just something I noticed.

m3dwards commented at 8:13 pm on July 15, 2024:

Don’t have strong feeling either way but there is an argument that doing it with one variable means any changes to flags passed to the first call get automatically added to the second call which is I think better default behaviour.

pinheadmz approved

pinheadmz commented at 7:50 pm on July 15, 2024: member

ACK 23333b7ed243071c9b4e4f04c727556d8065acbb

Reviewed updated code change and tested in linux VM with local ipv6 only.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 23333b7ed243071c9b4e4f04c727556d8065acbb
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmaVfcgACgkQ5+KYS2KJ
 7yTorBw/7BFoooqdMN+Zb000m/qKsUoMhW36vZBdGTuxt3xkWrRHlggm8ym/fQfen
 8jt9VcWYl6vmZFnU+nUvJbdN+GLTdf+fDT8vdJdYKjKSxmflEIGLRnf5Pktf2E8lG
 9rycNQqa6rxcMcnHu0p69VWUU2Ay3vXTV3OOZ/UJFMTLo1twLyljxsSwu6JvMSKgM
104qOqFnqxVeJ1fq3Lw7TtjbwvKweDU8iw2B/YE6kXg711u0qdbSHe3MiPiXKl0sRp
11RCkFXuO/q0GICUfW+XHqfs4tDikD+5TRhZ4bBRond7KIdYSdizZzgkNdI1elcI9B
12/exMlaLD4cJAEeqovhgNpSByI3rJKQMmzUWS2GrzU78Rsk1zg3a0L+XioM7BMrMm
13ClZs82CER6VDNApCv1pEZkdF2AsrbCXDT9GmmpIearoTWAKhR2GP9hccUxKOeZB6
14qtb5ep5SvyjM+hIj4IBTM6g0i24SvfdP3/9WcuYj5G1Cl7sWVI1tdXJIDP2hMCij
157J9yC9YznQpvx9626yASNpFq/WRcC4Iug7WdTrSlnIBCQDJ9wMt1uzK6Aj/SdDI+
16vKE45GqpYag7MPKnNSobDSe83JWssPy8ZNP1W8FmBV1Vd4yRMdKJea4yrmNYDCwC
17CiC31ZufOkWBdb4n+rduarigMQxEOQMm/8nqFCKdl7GFaIcUjPQ=
18=wUuO
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

achow101 commented at 9:41 pm on July 18, 2024: member

ACK 23333b7ed243071c9b4e4f04c727556d8065acbb

achow101 merged this on Jul 18, 2024

achow101 closed this on Jul 18, 2024

net: Allow -proxy=[::1] on nodes with IPV6 lo only #30245

Code Coverage

Reviews