Conversation
cb9e41b to
15fb76e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Schannel ALPN support when building with older mingw-w64 and Microsoft SDK headers.
Changes:
- Adds missing ALPN declarations for older headers.
- Enables existing ALPN logic across supported Windows toolchains.
- Retains runtime OS/Wine capability detection.
File summaries
| File | Description |
|---|---|
lib/vtls/schannel.c |
Adds compatibility declarations and removes compile-time ALPN gating. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
(something is wonky with the version checks tldr) I'm seeing some weird behavior with this. I built this branch in VS2010 and then I ran it in Windows 7 (ok - no alpn and no errors) and Windows 11 (not ok - no alpn and no errors). I can't figure out is why it's not showing alpn results in Windows 11. So far I traced it to s_pRtlVerifyVersionInfo being NULL (in this case curl falls back on the regular verify version info which will not return Windows 11 so it thinks there is no ALPN). But this makes no sense. I put a printf in curlx_verify_windows_version I get weird results where it's valid but then nulled and then valid again which makes no sense I did a "find all references" on s_pRtlVerifyVersionInfo in VS and I can't tell where it's being nulled. Also reproducible in Windows 7. Something is up here I'm not sure it's even related to this branch https://github.com/vszakats/curl/compare/w-try-old-alpn...jay:curl:w-try-old-alpn_SPRINKLE?expand=1 next I'll try a data breakpoint but out of time for now |
|
alright I think I'm on to something, i logged curlx_verify_windows_init and and I see it's called twice, but the second time it's called after the null result, which might mean s_pRtlVerifyVersionInfo is used before it's initialized in the library (like there's two of them?), i'll look into it more later |
|
Just reading the code, the issue may be that If this theory holds, this patch may fix it: --- a/lib/easy.c
+++ b/lib/easy.c
@@ -136,6 +136,11 @@ static CURLcode global_init(long flags, bool memoryfuncs)
Curl_ccalloc = (curl_calloc_callback)calloc;
}
+ if(Curl_win32_init(flags)) {
+ DEBUGF(curl_mfprintf(stderr, "Error: win32_init failed\n"));
+ goto fail;
+ }
+
if(Curl_trc_init()) {
DEBUGF(curl_mfprintf(stderr, "Error: Curl_trc_init failed\n"));
goto fail;
@@ -151,11 +156,6 @@ static CURLcode global_init(long flags, bool memoryfuncs)
goto fail;
}
- if(Curl_win32_init(flags)) {
- DEBUGF(curl_mfprintf(stderr, "Error: win32_init failed\n"));
- goto fail;
- }
-
if(Curl_amiga_init()) {
DEBUGF(curl_mfprintf(stderr, "Error: Curl_amiga_init failed\n"));
goto fail; |
- In easy.c global_init, initialize win32 before ssl. Schannel SSL init depends on win32 initialization. Prior to this change applications with no compatibility manifest using a libcurl with Schannel had ALPN erroneously disabled. During Schannel init there is a version info check so that ALPN is only enabled for Windows 8.1 or later (>= NT 6.3), but since win32 was not yet initialized the version check would fail even if the OS was more recent. This change ensures that the pointer to function RtlVerifyVersionInfo is initialized before SSL initialization so that version checks during SSL initialization do not use VerifyVersionInfo as a fallback. On Windows RtlVerifyVersionInfo compares against the actual OS version info whereas VerifyVersionInfo has behavior that varies depending on whether the application (eg curl tool) has a compatibility manifest. If there is no manifest, VerifyVersionInfo treats the OS version to compare against as Windows 8 (NT 6.2) even on later versions. So basically NT 6.2 < NT 6.3 caused ALPN to be erroneously disabled. Reported-by: Jay Satiro Ref: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-verifyversioninfoa Fixes #22739 (comment) Closes #22746
To bring these builds on par with the rest of supported Windows
toolchains.
Also: drop dead MS documentation URL.