Currently the HTTP server initialization (HTTPBindAddresses) fails only when all bindings fail. So if multiple binds are specified (127.0.0.1 and ::1 by defeault) and one succeeds and the other fails, the latter is essentially ignored.
This commit changes the error behavior to fail if not all binds could be performed, which I think is more in line with how software normally handles this and what users expect.
Needs mention in release notes.
Doesn’t binding 127.0.0.1 fail if ::1 is already bound, on some systems? (Because ::1 includes 127.0.0.1 in practice.)
No, that’s definitely not true. No localhost address or subnet should include each other.
0lo: flags=73<UP,LOOPBACK,RUNNING> mtu 65536
1 inet 127.0.0.1 netmask 255.0.0.0
2 inet6 ::1 prefixlen 128 scopeid 0x10<host>
so:
0127.0.0.1/8 (0:0:0:0:0:0:7f00:0001/96 if embedded in IPv6)
10:0:0:0:0:0:0:1/128
None of these overlaps.
You’re mixing it up with ::0 / INADDR_ANY, which can be all-inclusive in some systems (sometimes depending on dual-stack settings).
Concept ACK
The error message here also needs updating for the new behaviour:
bind_to_localhost_only? Not sure if self.bind_to_localhost_only even makes sense after the recent changes to how rpc binds.
bind_to_localhost_only has to do with P2P, not RPC. IIRC RPC was always already bound to localhost only for the tests, apart from the bind test.
I guess this has to do with Travis not supporting (even local!) IPv6 at all, so the IPv6 bind fails and it doesn’t start.
I suppose not being able to auto-bind on either IPv4 or IPv6 is valid if there is no such interface, at least if -rpcbind was not explicitly given.
OK, per suggestion by @gmaxwell I’ve changed the logic. By default, if no explicit bind configuration is given, the EADDRNOTAVAIL error while binding is ignored (but not, say, EADDRINUSE). This is user-friendly as it means the default setting will auto-detect based on available IPv4/IPv6. In all other cases all errors are fatal.
This can be tested using linux network namespaces
0ip netns add dumb
1ip netns exec dumb ip addr add 127.0.0.1/8 dev lo # adds both IPv4 and IPv6
2ip netns exec dumb ip addr del ::1/128 dev lo # remove IPv6
3ip netns exec dumb ip link set lo up
4ip netns exec dumb src/bitcoind -regtest
343+ LogPrintf("Binding RPC on address %s port %i failed, error ignored because interface was unavailable.\n", i->first, i->second);
344+ }
345+ }
346+ }
347+ if (num_fail != 0) {
348+ // In case of an error, clean up listening sockets that succeeded to
StopHTTPServer (because the owning evhttp is freed but the listeners are not).
Currently the HTTP server initialization (`HTTPBindAddresses`) fails
only when *all* bindings fail. So if multiple binds are specified
(`127.0.0.1` and `::1` by defeault) and one succeeds and the other
fails, the latter is essentially ignored.
This commit changes the error behavior to fail *if not all* binds could
be performed, which I think is more in line with how software normally
handles this and what users expect.
At least on Travis, the failure isn’t caused by listen()’s errno being lost.
Instead, what’s happening is that getaddrinfo is returning EVUTIL_EAI_ADDRFAMILY. These errors are borrowed from the system’s EAI_* constants, which [can] overlap with errno’s constants, so it’s not practical to just stuff them into errno from evhttp_bind_socket_with_handle… Opened a libevent issue for this: https://github.com/libevent/libevent/issues/740
Added a somewhat-ugly hack on top of my branch: https://github.com/bitcoin/bitcoin/commit/d2d51e7aedc85f50dcc3a94d220500cb3bfe10f9
Unfortunately, evhttp_bind_socket_with_handle returns a 0/Success errno when listen fails.
Strange, this worked for me, and it seems to work on Travis. As stated I’ve tested this (at least on Linux) in various ways.
An alternative solution here (which I initially wanted to do, but this seemed like a workable suggestion at the time if of course C code didn’t mess up error handling everywhere) is to test which local interfaces are available, and only
This avoids having to check error codes, and could be more robust. I might give this a try. Though I’m not sure there’s a good platform-independent way of doing this.
I do not think this is worth adding a forest of hacks cloning parts of code from libevent for. Better to leave it as-is then, it wasn’t that bad.
WAIT: it’s the bind in evhttp_bind_socket_with_handle failing that we’re looking for here, not the listen. This seems to not touch the error code, so it can be retrieved succesfully:
0 if ((fd = bind_socket(address, port, 1 /*reuse*/)) == -1)
1 return (NULL);
I don’t think we care about specific errno when listen fails, do we? This is always fatal.
getaddrinfo; no idea why it’s passing on master. :/
Yes, tried in #15050, really strange. I think specific error code checking is a dead end, just too fragile.
Looking for a platform independent way now to check if there is a local IPv4 or local IPv6 interface without actually binding to it. libevent internally checks for a nonlocal interface in evutil_found_ifaddr but this is not what we want to know in this case.
Too bad the logic in evutil_check_ifaddrs is not exposed externally.
Looks like we have vaguely analogous logic in void Discover() in net.cpp, though for windows it falls back to ’look up localhost'.
Just reading the libevent2 code, I did not try this, but the following may be a way to pass IPV6_V6ONLY to libevent2:
Instead of evhttp_bind_socket_with_handle() (called from src/httpserver.cpp) call evconnlistener_new_bind(flags|=LEV_OPT_BIND_IPV6ONLY), it returns evconnlistener. Pass that to evhttp_bind_listener().
Just reading the libevent2 code, I did not try this, but the following may be a way to pass IPV6_V6ONLY to libevent2:
To provide context: being able to set IPV6_V6ONLY on the evhttp listening socket to a fixed value (either on or off will do) independent of operating system will allow to do consistent error checking on the binding to localhost, as the the uncertainty in behavior is gone.
Labels
RPC/REST/ZMQ
Milestone
0.18.0