-
Notifications
You must be signed in to change notification settings - Fork 38.7k
depends: Patch libevent build to fix IPv6 -rpcbind on Windows #18287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
libevent uses getaddrinfo when available, and falls back to gethostbyname Windows has both, but gethostbyname only supports IPv4 libevent fails to detect Windows's getaddrinfo due to not including the right headers This patches libevent's configure script to check it correctly
libevent uses getaddrinfo when available, and falls back to gethostbyname Windows has both, but gethostbyname only supports IPv4 libevent fails to detect Windows's getaddrinfo due to not including the right headers This patches libevent's configure script to check it correctly Github-Pull: bitcoin#18287 Rebased-From: e918138
|
Good catch. It's really sneaky that including a header can make this difference. I think it'd be good to have a test for this case to detect regression. |
|
Who maintains the Windows CI stuff? That's the only place I can see a test making sense... |
|
How would a test case look like? Can't it be added as a normal unit or functional test? |
|
I think just specifying |
|
Appveyor uses msvc to compile, not sure if that changes anything. @sipsorcery Though, we have a windows cross build that runs the unit tests: bitcoin/ci/test/00_setup_env_win64.sh Line 12 in 58c7288
|
|
I've just tested starting bitcoind built with msvc using:
It failed with the relevant log messages being: This does appear to support Luke's diagnosis. The patch won't solve the problem for msvc builds because the If it's possible it would seem sensible to add a unit test that attempts to start |
Gitian builds
|
|
@luke-jr I'm attempting to verify this fix but so far the Windows executable I produce still fails to bind the RPC port on It looks like the patch is working insofar as if I do: the output does indicate I'll keep digging but if you know of any obvious things I should check I'd be happy to hear them. |
|
This patch (part of libevent/libevent@6d54be2) looks ok and the inclusion of the additional headers definitely fixes libevents detection of However I have also tried to verify the fix, and after cross-compiling this PR and taking Obviously there is an issue here, but at this point I don't think this can be a blocker for 0.20.0 unless someone can verify a fix. |
|
Going to close this in favour of #19375, which includes this change. |
…ipv6 usage eb6b735 build: pass _WIN32_WINNT=0x0601 when building libevent for Windows (fanquake) 03e056e depends: Patch libevent build to fix IPv6 -rpcbind on Windows (Luke Dashjr) Pull request description: TLDR: This poaches a commit from #18287 and adds one more to adjust the Windows version targeted when building libevent. These changes combined should fully fix ipv6 usage with the RPC server on Windows. --- Binding the RPC server to a ipv6 address does not currently work on Windows. We currently try and bind to `127.0.0.1` and `::1` [by default](https://github.com/bitcoin/bitcoin/blob/master/src/httpserver.cpp#L304). On Windows you'll see lines like this in debug.log: ```bash 2020-06-24T01:49:04Z libevent: getaddrinfo: nodename nor servname provided, or not known 2020-06-24T01:49:04Z Binding RPC on address ::1 port 8332 failed ``` This issue was bought up in, and supposedly fixed by #18287, however the two people that tested it, both said that it didn't fix the problem. I think I now understand why that change alone is incomplete. Our call into libevent starts with [evhttp_bind_socket_with_handle()](https://github.com/bitcoin/bitcoin/blob/master/src/httpserver.cpp#L325): ```bash evhttp_bind_socket_with_handle() bind_socket() make_addrinfo() evutil_getaddrinfo() if #USE_NATIVE_GETADDRINFO #ifndef AI_ADDRCONFIG evutil_adjust_hints_for_addrconfig_() evutil_check_interfaces() evutil_check_ifaddrs() evutil_found_ifaddr() // miss identifies ipv6 as ipv4? #endif evutil_getaddrinfo_common_() ``` The problem is falling into ["#ifndef AI_ADDRCONFIG"](https://github.com/libevent/libevent/blob/master/evutil.c#L1580): ```cpp #ifndef AI_ADDRCONFIG /* Not every system has AI_ADDRCONFIG, so fake it. */ if (hints.ai_family == PF_UNSPEC && (hints.ai_flags & EVUTIL_AI_ADDRCONFIG)) { evutil_adjust_hints_for_addrconfig_(&hints); } #endif ``` When this occurs, hints end up being adjusted, and it seems that ipv6 addresses end up being mis-identified as ipv4? However this shouldn't happen, as these `AI_` definitions are available on Windows. The issue is that in evutil.c, `_WIN32_WINNT` [is set to `0x501`](https://github.com/libevent/libevent/blob/master/evutil.c#L45) (XP). This obviously predates Vista (`0x0600`), which is when the `AI_ADDRCONFIG` definition (and others) became [available](https://docs.microsoft.com/en-us/windows/win32/api/ws2def/ns-ws2def-addrinfoa). The change here will override libevents internal D_WIN32_WINNT defines. This should be ok, because it's only making "more" of the Windows API available. It's also aligned with what we do in our own configure, we pass [`D_WIN32_WINNT=0x0601`](https://github.com/bitcoin/bitcoin/blob/master/configure.ac#L610). We also now use linker flags to restrict our binary from running on a Windows version [earlier than Windows 7](https://github.com/bitcoin/bitcoin/blob/master/configure.ac#L621). The combined fixes can be tested by running: `bitcoind -rpcbind=::1 rpcallowip='0.0.0.0/0' -debug=http` and then querying it using: `bitcoin-cli -rpcconnect=::1 getblockchaininfo` TODO: - [x] Open an issue upstream. libevent/libevent#1041 ACKs for top commit: laanwj: ACK eb6b735 Tree-SHA512: e1e50f194911301981edaed0c216ed4efb9ebd4a1f9bc9b9f85bec7140b66c45c8666fd5db4aad359596559d4a08ab7c920e9d9736f3ecdbb841afc54e40586e
libevent uses getaddrinfo when available, and falls back to gethostbyname
Windows has both, but gethostbyname only supports IPv4
libevent fails to detect Windows's getaddrinfo due to not including the right headers
This patches libevent's configure script to check it correctly
Upstream issue: libevent/libevent#966