asyn-thread: don't use PF_INET6 for getaddrinfo on Windows

[bagder]

It seems to not be able to resolve IPv6 addresses!

Fixes #12134
Reported-by: zhengqwe on github

[vszakats]

If we knew which Windows version fixed this (if), we might use:

#if (defined(WIN32) && \
  defined(_WIN32_WINNT) && (_WIN32_WINNT >= _WIN32_WINNT_WIN8))

or something.

Nothing readily popped out from the docs suggesting such behaviour for Win7:
https://learn.microsoft.com/en-us/windows/win32/api/ws2tcpip/nf-ws2tcpip-getaddrinfo
https://learn.microsoft.com/en-us/windows/win32/api/ws2def/ns-ws2def-addrinfoa

For XP, IPv6 support must be installed, but that's expected.

Perhaps logging the error code might help digging deeper.

[bagder]

Nothing readily popped out from the docs suggesting such behaviour for Win7:

The original report is also on Win 10.

[vszakats]

Ah, okay, missed that!

But then, is it possible this is actually working correctly, and in these env the name simply cannot be resolved to IPv6? This seems to be the case on the machine I run the tests. It has a local IPv6 address, but no IPv6 support upstream. If this is the case, this isn't an error, because what happened was that PF_UNSPEC returned an invalid IP or no IPv6 (?) and then it couldn't connect. With 8.3.0, it returns an error right away at resolution. Which seems to be the correct behaviour.

2 cents, I might be missing a lot of things here.

[bagder]

this is actually working correctly

First, the original user said in the report they could ping -6 to the host so it could be resolved to a IPv6 address with ping but not with curl.

Then you said you could reproduce the problem and reverting a commit fixed it. Now you seem to be reverting that statement a little bit so I'm no longer exactly sure what you meant...? 🤔

[vszakats]

In my tests I could never actually download the URL. Here are the results:
Stable:

Test curl.exe with reverted patch:

So mine wasn't a useful test after all. Due to lack of IPv6 internet
I believe it works as expected with all versions.

Let's stick to what the OP says if they could test the curl.exe
with reverted patch on a machine with an IPv6 resolver.

(sorry for the noise!)

[vszakats]

The revert fixed it for OP.

This leaves me curious what is going on with this API.

[jay]

On Windows if you do not have what it thinks is working IPv6 then it strips AAAA from the results. Their check is apparently different than our check. For example

You call getaddrinfo set to PF_INET6 and query AAAA google.com, the request is sent [1] and the answer contains a single AAAA record. but because windows has determined there's no working IPv6 it won't actually provide those records. This is probably a good thing because on Windows a lot of curl will have "working" IPv6 according to our own check even if it's not actually able to connect to anything remotely.

They definitely should have documented this behavior. I tried in curl just now on a computer with IPv4 and Link-local IPv6 and Curl_ipv6works is true so PF_INET6 is set, and it does what I described in the paragraph above. getaddrinfo returned WSANO_DATA

Ref: https://stackoverflow.com/questions/66755681/getaddrinfo-c-on-windows-not-handling-ipv6-correctly-returning-error-code-1

[1]: There's a local caching DNS that has to be disabled to force the request to be sent (like if you wanted to monitor in wireshark for testing purposes) but the answer is the same, just the aaaa. you can disable the dns caching service net stop dnscache

[jay]

more info, taken from msdn getaddrinfo and the windows 7 sdk source code comments

AI_ADDRCONFIG:

If the AI_ADDRCONFIG bit is set, getaddrinfo will resolve only if a global address is configured. If 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. The IPv4 or IPv6 loopback address is not considered a valid global address.

we are not setting that. i searched for it and found this in a local ms sdk source:

//  Flags used in "hints" argument to getaddrinfo()
//      - AI_ADDRCONFIG is supported starting with Vista
//      - default is AI_ADDRCONFIG ON whether the flag is set or not
//        because the performance penalty in not having ADDRCONFIG in
//        the multi-protocol stack environment is severe;
//        this defaulting may be disabled by specifying the AI_ALL flag,
//        in that case AI_ADDRCONFIG must be EXPLICITLY specified to
//        enable ADDRCONFIG behavior
//

So it was indeed intentional, they just didn't document it (on their website).

[bagder]

On Windows if you do not have what it thinks is working IPv6

But I have working IPv6, so clearly it it is a broken IPv6 detection. If this is what prevents it from working for me.

If I use the curl tool Windows 10 ships, it connects to and uses IPv6.

Curl_ipv6works is true so PF_INET6 is set

Sure, but that's a failure that just makes IPv6 connects fail so it's a benign error. It's not even actually an error, because clearly IPv6 works on your system, you just can't access remote hosts over it.

[bagder]

but yes, when I use 8.4.0, I can see that it does not try IPv6 even with a regular command line. Which makes me wonder why the windows-provided version does?

[jay]

Yes but the default as I quoted above is actually "getaddrinfo will resolve only if a global address is configured". So are you saying that you have that and it works?

[bagder]

When I run curl curl.se, it resolves IPv6 addresses, connects over IPv6 and transfers HTTP. I would presume that means I have "a global address is configured".

[jay]

When I run curl curl.se, it resolves IPv6 addresses, connects over IPv6 and transfers HTTP. I would presume that means I have "a global address is configured".

I would agree. Windows has special checks for network connectivity so maybe it's more complicated than whether you have a global IPv6 address, perhaps the OS tried to connect over that address and couldn't.

[bagder]

The two different curl versions behave differently and that's what triggers my curiosity the most.

[bagder]

I think we can safely conclude that this PR is not the right thing to do anyway.

[jay]

I think we need more testers with working global IPv6 to tell us if this is a problem for them.

The two different curl versions behave differently and that's what triggers my curiosity the most.

If you're going to bisect use winbuild, it's way faster than mingw-w64. Also sanity check that IPv6 is in the features list.

[bagder]

our build 8.0.1_9 resolves and uses IPv6 fine

This is clearly something we broke since.

[bagder]

The 8.3.0_3 build works. 8.4.0_1 does not

[bagder]

I don't have a windows development environment setup so I can't bisect this closer right now.

[jay]

I don't have a global IPv6 address (my ISP doesn't support IPv6) so hopefully someone else can step in.

[0xdeadbad]

Hello, my ISP provides global IPv6 IPs. I only use Void Linux and FreeBSD most of the time, but I also have a Windows 11 installed on my machine. I want to help if possible, I'm just a bit clumsy with Windows...

[jay]

I want to help if possible, I'm just a bit clumsy with Windows...

Ok, here is a step-by-step tested in Windows 11 x64 21H2:

• Open a command prompt.
  Win+R to open Run prompt, then type cmd. Alternately, type cmd in search and choose Command Prompt app.

• Switch to the desktop directory.
  cd %USERPROFILE%\desktop

• Use the curl included with Windows 11 (curl 8.0.1) to download the latest curl (curl 8.4.0).
  curl -fL -O https://curl.se/windows/dl-8.4.0_4/curl-8.4.0_4-win64-mingw.zip

• Unzip it.
  tar -vxf curl-8.4.0_4-win64-mingw.zip

• Switch to the extracted bin directory, which contains the curl 8.0.4 executable.
  cd %USERPROFILE%\desktop\curl-8.4.0_4-win64-mingw\bin

• Since your working directory contains a curl executable, Windows will run that instead of the system curl. Confirm by checking the curl version. You should see curl 8.4.0 (x86_64-w64-mingw32) libcurl/8.4.0.
  curl -V

• Try both versions of curl to resolve example.com as IPv6 (-6) and fetch the headers (--head).
  curl --head -6 https://example.com
c:\windows\system32\curl.exe --head -6 https://example.com

• Try ping to resolve example.com as IPv6 and ping it.
  ping -6 example.com

• Respond with the output.

[0xdeadbad]

• ping -6 example.com

Thank you very much for the detailed instructions. I was trying to build it on Windows but no luck. Here is the results:

C:\Users\Garicas\Desktop>cd %USERPROFILE%\desktop\curl-8.4.0_4-win64-mingw\bin

C:\Users\Garicas\Desktop\curl-8.4.0_4-win64-mingw\bin>curl -V
curl 8.4.0 (x86_64-w64-mingw32) libcurl/8.4.0 OpenSSL/3.1.3 (Schannel) zlib/1.3 brotli/1.1.0 zstd/1.5.5 WinIDN libssh2/1.11.0 nghttp2/1.57.0 ngtcp2/1.0.1 nghttp3/1.0.0
Release-Date: 2023-10-11
Protocols: dict file ftp ftps gopher gophers http https imap imaps ldap ldaps mqtt pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp ws wss
Features: alt-svc AsynchDNS brotli HSTS HTTP2 HTTP3 HTTPS-proxy IDN IPv6 Kerberos Largefile libz MultiSSL NTLM SPNEGO SSL SSPI threadsafe UnixSockets zstd

C:\Users\Garicas\Desktop\curl-8.4.0_4-win64-mingw\bin>curl --head -6 https://example.com
curl: (6) Could not resolve host: example.com

C:\Users\Garicas\Desktop\curl-8.4.0_4-win64-mingw\bin>c:\windows\system32\curl.exe --head -6 https://example.com
HTTP/1.1 200 OK
Content-Encoding: gzip
Accept-Ranges: bytes
Age: 524714
Cache-Control: max-age=604800
Content-Type: text/html; charset=UTF-8
Date: Tue, 24 Oct 2023 03:16:33 GMT
Etag: "3147526947+gzip"
Expires: Tue, 31 Oct 2023 03:16:33 GMT
Last-Modified: Thu, 17 Oct 2019 07:18:26 GMT
Server: ECS (mic/9A9D)
X-Cache: HIT
Content-Length: 648


C:\Users\Garicas\Desktop\curl-8.4.0_4-win64-mingw\bin>ping -6 example.com

Pinging example.com [2606:2800:220:1:248:1893:25c8:1946] with 32 bytes of data:
Reply from 2606:2800:220:1:248:1893:25c8:1946: time=132ms
Reply from 2606:2800:220:1:248:1893:25c8:1946: time=133ms
Reply from 2606:2800:220:1:248:1893:25c8:1946: time=135ms
Reply from 2606:2800:220:1:248:1893:25c8:1946: time=134ms

Ping statistics for 2606:2800:220:1:248:1893:25c8:1946:
    Packets: Sent = 4, Received = 4, Lost = 0 (0% loss),
Approximate round trip times in milli-seconds:
    Minimum = 132ms, Maximum = 135ms, Average = 133ms

[jay]

C:\Users\Garicas\Desktop\curl-8.4.0_4-win64-mingw\bin>curl --head -6 https://example.com
curl: (6) Could not resolve host: example.com

Thanks. That confirms what was reported.

@DHowett do you have any insight why PF_INET6 does not work to resolve IPv6 addresses when there is an IPv6 address configured? All I've been able to find is this.

[gix]

I don't see getaddrinfo being used at all by 8.4.0:

> dumpbin /imports curl-8.4.0_5-win64-mingw\bin\curl.exe | rg 'addr|host'
                          A9 gethostbyaddr
                          AA gethostbyname
                          AB gethostname
                          B6 inet_addr
> dumpbin /imports curl-8.3.0_3-win64-mingw\bin\curl.exe | rg 'addr|host'
                          A7 freeaddrinfo
                          A8 getaddrinfo
                          A9 gethostbyaddr
                          AA gethostbyname
                          AB gethostname
                          B6 inet_addr

Is this expected?

The curl-8.4.0_5 mingw binary fails here too. So far I couldn't reproduce the issue with a self-built curl: Neither building master nor 8.4.0, with llvm-mingw or with MSVC, with ares or without. Do certain optional dependencies influence the behavior?

[bagder]

Nice find @gix, this seems suspicious. It is certainly expected to use getaddrinfo!

@vszakats can you understand why this happens?

[jay]

I don't see getaddrinfo being used at all by 8.4.0:
Is this expected?

No, it's unexpected. curl can be built with IPv6 support but without IPv6 resolver support and that might be the problem.

curl/lib/curl_setup.h

Lines 582 to 586 in d755a5f

	#if defined(ENABLE_IPV6) && defined(HAVE_GETADDRINFO)
	# define CURLRES_IPV6
	#else
	# define CURLRES_IPV4
	#endif

[gvanem]

My build (w/o Cares) shows for curl.exe -6 https://www.example.com this:

(using my Wsock-trace).

[vszakats]

@gix: Nice catch! Can confirm that getaddrinfo stopped being referenced since 8.4.0_1.

curl-for-win (incl. dependencies) did not change between 8.3.0_3 and this, suggesting the change happened in curl sources. curl-for-win used Makefile.mk for that build. In such cases getaddrinfo is enabled via lib/config-win32.h by looking at _WIN32_WINNT.

I've switched to CMake two days ago and consistent with that, getaddrinfo re-appeared in the daily (8.5.0-DEV) builds since 2023-10-25: https://github.com/curl/curl-for-win/actions/runs/6645832682

Ok, so this hunk caused the regression:
3802910#diff-d5391cab46f1c676396eaccc46ad04b7cff0302897908ac5478697225599f016L31-L35

Surprisingly, _mingw.h had the side-effect to set a default _WIN32_WINNT (which is what we use to auto-enable getaddrinfo), even though we specifically added (6b0f217 / 3733717) that header to add mingw-w64 version information. Since we did not need that anymore after dropping old-mingw, I deleted it, leaving no _WIN32_WINNT value. This only affected mingw-w64 builds done via Makefile.mk which in practice is probably just curl-for-win.

This can be fixed three ways:

1. switch to CMake for curl in curl-for-win. This is already done for 8.5.0. Can work for 8.4.0 too.
2. restore _mingw.h for this side-effect. → Makefile.mk: restore _mingw.h for default _WIN32_WINNT #12217
3. fixup headers to include windows.h (or winsock2.h) before using _WIN32_WINNT for this feature check. This might cause a build performance hit, so probably would need moving detection closer to use or other solution.

[vszakats]

Test binary here attempting to fix this with #12217:
https://ci.appveyor.com/api/buildjobs/tmqd8tt2cu25snjt/artifacts/all-mingw-8.4.0_5.zip

$ x86_64-w64-mingw32-objdump --all-headers curl.exe | grep getaddrinfo
	5a1644	  168  getaddrinfo

Can you verify if this build fixes the issue?

[gix]

Shouldn't curl explicitly set _WIN32_WINNT to whatever your oldest supported Windows release is? Either via the curl-for-win build or in the source. There's no need to include _mingw.h for this, just conditionally define _WIN32_WINNT. If you don't you also risk having it auto-defined by Windows SDK headers, and they choose the latest version, which may make the binary fail to run on older Windows versions.

[vszakats]

I think if we start overriding it or setting it ourselves it might make things more confusing. We did so earlier in CMake.

If we rely on this value, it's our sources responsibility to include the SDK before using it.

We also have this value pre-detected inside CMake, which does rely on SDK headers, not _mingw.h. I think we should aim to switch to that in the sources as well. I haven't verified, but presumably mingw-w64 always sets the same default value in both its Windows SDK headers and _mingw.h.

[gix]

If we rely on this value, it's our sources responsibility to include the SDK before using it.

It seems you have it backwards. _WIN32_WINNT is the sanctioned way with which you as a developer control which API level the Windows SDK makes available in its headers. This is best done as part of global compiler settings in the build. If you want the official curl binary to support Windows 7 or later set it to 0x0601. Not defining it is IMO more confusing since you get unpredictable behavior: mingw seems to be conservative and just defaults it to Windows 7 if undefined. The Windows SDK chooses the latest version by default. There may be other code that sets it if undefined.

You are right that the curl source itself (especially libcurl) should only use the macro to enable/disable features depending on the targeted OS version. It should never define it, so re-including _mingw.h arguably makes it worse.

This just leaves the case where someone else is using libcurl in his project. libcurl may opt to error if _WIN32_WINNT is undefined (or below a minimum supported value) to force the user to make a decision. Otherwise I'd suggest using the recommended header sdkddkver.h (see the docs). That header is super lightweight, just some defines with no other includes. But as said above, it defaults to Windows 10.

[vszakats]

I don't think I "have it backwards", I think we're saying the same thing: _WIN32_WINNT is defined by the SDK, always, to a default. For that to work, we must #include the SDK before using the value. Furthermore, anybody is free to override it to whatever Windows version they wish to target, in which case this overrides the default set by the SDK.

It's not curl's job to set any default, neither on behalf of the SDK nor on behalf of the person building curl.

Erroring out if the value isn't defined is an option, though I think it's more helpful to initialize this value to the SDK default, otherwise it will also lead to confusion.

The root issue here is that we were using _WIN32_WINNT without letting the SDK set the value to a default, and accidentally relying on _mingw.h to set it. FWIW MSVC builds are also affected, but in that case we have a fallback logic that enables these functions based on MSVC version (which works in most cases, but this is really the function of the SDK used and not the MSVC version even though these are often bound together).

Thanks for the sdkddkver.h suggestion, this might be the solution to avoid pulling in a heavy Windows header or _mingw.h (which we never #included for that purpose anyway).

[gix]

The root issue here is that we were using _WIN32_WINNT without letting the SDK the set value to a default

The other issue is that the official curl-for-win builds don't set it. If you just let the SDK set the default you get a curl binary that inherits the Windows version of whatever SDK your build server used which is most likely not what you want. If you do that with a recent SDK you target Windows 11 22H2, and nothing earlier.

[vszakats]

Yes, setting that in curl-for-win might be a good idea, though a different issue. The build should work consistently without setting a value manually. I remember the default used to be ~Vista (supporting getaddrinfo).

sdkddkver.h seems to be missing from some (old) C compilers, that needs more investigation. In mingw-w64 it's always available, which means it's most certainly also there in MSVC versions we support. Question if we're concerned about Pelles C, I lost track of that compiler, but the source references it. Watcom C 1.9 got it misspelled, but we don't seem to support it anyway.

In my build of mingw-w64 v11.0.1, sdkddkver.h defaults _WIN32_WINNT to 0x0502 (WS2003). _mingw.h defaults it to 0x0a00 (Win10) (it is mingw-w64 build-dependent), so even that is inconsistent. For even more fun, windows.h includes _mingw.h first, so sdkddkver.h never gets the chance to set its older value.

curl-for-win CMake builds detect 0x0a00 so Win10 seems to be the new default also in the release env using mingw-w64 11.0.1-3.

This suggests that we cannot use sdkddkver.h and using _mingw.h is the next best thing. Even better would be using windows.h, as it'd work with all compilers, but this needs extra effort to avoid a build-perf regression.

[vszakats]

I re-verified and curl isn't using any feature specific to higher Windows versions than 0x0600 (Vista). It means that targeting the default 0x0a00 (Win10) results in a binary running fine on pre-Win10 versions (for sure it runs on Win7).

Speaking of future curl versions and especially dependencies, setting the target version might still be useful. This will need further tests to see how they all behave in that case.

[vszakats]

These two tests confirm that explicitly setting _WIN32_WINNT=0x0600 did not affect the curl-for-win binaries created:
https://ci.appveyor.com/project/curlorg/curl-for-win/builds/48385327/artifacts
https://ci.appveyor.com/project/curlorg/curl-for-win/builds/48385091/artifacts

I plan to set it for an assurance to keep it that way. Thanks for the suggestion @gix.

[vszakats]

8.4.0_6 is out which sets the target Windows version explicitly. This acts as a workaround for this issue while also ensures Vista compatibility going forward. Aside this regression, this change tested as a no-op, meaning curl-for-win was/is Vista compatible as advertised. The new release also comes with nghttp2 1.78.0 (which builds faster, amongst others), adds dumping of curl.exe imports, and fixes a recent packaging regression where .def timestamps broke reproducibility of the tarballs:

curl/curl-for-win@55ccad2
https://ci.appveyor.com/project/curlorg/curl-for-win/builds/48386365
https://curl.se/windows/

Can't test for this issue but getaddrinfo is enabled correctly again.

[gix]

I can confirm it works:

> .\curl-8.4.0_6-win64-mingw\bin\curl.exe -V
curl 8.4.0 (x86_64-w64-mingw32) libcurl/8.4.0 OpenSSL/3.1.4 (Schannel) zlib/1.3 brotli/1.1.0 zstd/1.5.5 WinIDN libssh2/1.11.0 nghttp2/1.58.0 ngtcp2/1.0.1 nghttp3/1.0.0
Release-Date: 2023-10-11
Protocols: dict file ftp ftps gopher gophers http https imap imaps ldap ldaps mqtt pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp ws wss
Features: alt-svc AsynchDNS brotli HSTS HTTP2 HTTP3 HTTPS-proxy IDN IPv6 Kerberos Largefile libz MultiSSL NTLM SPNEGO SSL SSPI threadsafe UnixSockets zstd
> .\curl-8.4.0_6-win64-mingw\bin\curl.exe -6 -v curl.se
*   Trying [2a04:4e42:c00::347]:80...
* Connected to curl.se (2a04:4e42:c00::347) port 80
* ...

[vszakats]

Thank you @gix!

[DHowett]

Thanks for driving this down! My critical mistake in testing was using the one from the Windows (inbox) build server at 8.4.0, but that one's built with an explicit Win10 SDK reference so it was not susceptible.