Skip to content

windows: reduce/stop loading DLLs at runtime #17413

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

Closed
wants to merge 15 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented May 21, 2025

  • replace dynamic InitSecurityInterface() call with early binding and
    link secur32 system DLL.
    The library and function are available in all supported curl Windows
    targets, meaning WinXP or newer. Add small hack for mingw32ce to
    make it build.

  • detect and use if_nametoindex() on Windows when available. Link
    iphlpapi system DLL. Requires targeting Vista or newer.
    Replacing the dynamic call and the pre-load optimization for lib3026.

Suggested-by: Jay Satiro


@vszakats vszakats added tests Windows Windows-specific tidy-up labels May 21, 2025
@vszakats vszakats marked this pull request as draft May 21, 2025 17:44
@vszakats vszakats force-pushed the w-dedupe-loadlib branch 3 times, most recently from dbc8ebe to 28c0786 Compare May 21, 2025 18:21
@vszakats vszakats marked this pull request as ready for review May 21, 2025 19:01
@jay
Copy link
Member

jay commented May 21, 2025

Also, do we need to dynamically load any of these libraries anymore, I have a Windows XP VM and I see them there, and some of them look really old I don't know how anyone would not have them at this point, the only thing that might be an issue is Windows CE which is being deprecated so I think maybe we can link to these libraries

curl/lib/curl_sspi.c

Lines 81 to 93 in 1c31498

/* If security interface is not yet initialized try to do this */
if(!Curl_hSecDll) {
/* Security Service Provider Interface (SSPI) functions are located in
* security.dll on WinNT 4.0 and in secur32.dll on Win9x. Win2K and XP
* have both these DLLs (security.dll forwards calls to secur32.dll) */
/* Load SSPI dll into the address space of the calling process */
if(curlx_verify_windows_version(4, 0, 0, PLATFORM_WINNT, VERSION_EQUAL))
Curl_hSecDll = Curl_load_library(TEXT("security.dll"));
else
Curl_hSecDll = Curl_load_library(TEXT("secur32.dll"));
if(!Curl_hSecDll)
return CURLE_FAILED_INIT;

$ git grep Curl_load_library
lib/curl_sspi.c:      Curl_hSecDll = Curl_load_library(TEXT("security.dll"));
lib/curl_sspi.c:      Curl_hSecDll = Curl_load_library(TEXT("secur32.dll"));
lib/system_win32.c:  s_hIpHlpApiDll = Curl_load_library(TEXT("iphlpapi.dll"));

@vszakats
Copy link
Member Author

vszakats commented May 21, 2025

Also, do we need to dynamically load any of these libraries anymore, I have a Windows XP VM and I see them there, and some of them look really old I don't know how anyone would not have them at this point, the only thing that might be an issue is Windows CE which is being deprecated so I think maybe we can link to these libraries

curl/lib/curl_sspi.c

Lines 81 to 93 in 1c31498

/* If security interface is not yet initialized try to do this */
if(!Curl_hSecDll) {
/* Security Service Provider Interface (SSPI) functions are located in
* security.dll on WinNT 4.0 and in secur32.dll on Win9x. Win2K and XP
* have both these DLLs (security.dll forwards calls to secur32.dll) */
/* Load SSPI dll into the address space of the calling process */
if(curlx_verify_windows_version(4, 0, 0, PLATFORM_WINNT, VERSION_EQUAL))
Curl_hSecDll = Curl_load_library(TEXT("security.dll"));
else
Curl_hSecDll = Curl_load_library(TEXT("secur32.dll"));
if(!Curl_hSecDll)
return CURLE_FAILED_INIT;

$ git grep Curl_load_library
lib/curl_sspi.c:      Curl_hSecDll = Curl_load_library(TEXT("security.dll"));
lib/curl_sspi.c:      Curl_hSecDll = Curl_load_library(TEXT("secur32.dll"));
lib/system_win32.c:  s_hIpHlpApiDll = Curl_load_library(TEXT("iphlpapi.dll"));

As per a quick local search WinCE doesn't have security.dll, but that's
only loaded for WinNT 4.0 and newer, meaning it's never hit in WinCE builds.
That said, maybe this should not be a runtime check, because curl requires
WinXP as a minimum. At build time it can fall back to secur32.dll for WinCE.

Another thing to check is if these DLLs do actually export the functions
looked up, on all supported OS versions, i.e. InitSecurityInterface in
secur32 and if_nametoindex in iphlpapi. In many (most?) cases this
pattern is used to make binaries run seamlessly on older OS versions where
symbols may be absent.

@vszakats
Copy link
Member Author

vszakats commented May 21, 2025

MS docs say that InitSecurityInterface is there since XP, so that's fine to make a static call:
https://learn.microsoft.com/en-us/windows/win32/api/sspi/nf-sspi-initsecurityinterfacea

if_nametoindex was introduced in Vista:
https://learn.microsoft.com/en-us/windows/win32/api/netioapi/nf-netioapi-if_nametoindex

@vszakats
Copy link
Member Author

vszakats commented May 21, 2025

The lib3026 load library calls were added in 856b133 #9412
to fix slow test runs. The commit says it's for legacy mingw. We no longer support that, but
the PR thread seems to suggest this applies to all MinGW. (possibly to all Windows builds regardless
of compiler.)

It pre-loads the DLLs to avoid Windows loading them on each curl_global_init() call, which is done
for all 100 threads in this test.

One call is gone in this PR by making the SSPI call non-dynamic.

To avoid the other one, an option is to make it a non-dynamic call when building for Vista or upper,
which would make this test fast on modern envs, including CI. It'd allow dropping the pre-load.
(edit: but that'd just add complexity, with almost zero advantage. edit2: it actually doesn't, because
the codepath was already present.)

@vszakats vszakats force-pushed the w-dedupe-loadlib branch 5 times, most recently from 7a0b7e4 to 00c5c5b Compare May 22, 2025 00:20
@vszakats vszakats added the feature-window A merge of this requires an open feature window label May 22, 2025
@vszakats
Copy link
Member Author

vszakats commented May 22, 2025

On a thorough re-read of #9412, the issue experienced (and mitigated via DLL pre-load)
was very slow execution of test3026 (meaning 15+ minutes), and only seen on old mingw,
which curl no longer supports. In current CI, with supported compilers, this test takes
a fraction of a second.

Attempt to address in #17414.

@vszakats vszakats marked this pull request as draft May 22, 2025 08:08
@vszakats vszakats changed the title lib3026: use Curl_load_library from lib, drop local implementation windows: reduce/stop loading DLLs at runtime May 22, 2025
@vszakats vszakats force-pushed the w-dedupe-loadlib branch from 00c5c5b to 1b4058c Compare May 22, 2025 09:56
@github-actions github-actions bot added the tests label May 22, 2025
@vszakats vszakats marked this pull request as ready for review May 22, 2025 10:10
@github-actions github-actions bot added the CI Continuous Integration label May 22, 2025
vszakats added a commit that referenced this pull request May 22, 2025
- appveyor: make a job target Windows XP.

- examples/block_ip: force this specific example to target Vista to make
  it compile when building curl for Windows XP. Fixing:
  ```
  docs\examples\block_ip.c(157): warning C4013: 'inet_pton' undefined; assuming extern returning int
  docs\examples\block_ip.c(272): warning C4013: 'inet_ntop' undefined; assuming extern returning int
  ```
  Ref: https://ci.appveyor.com/project/curlorg/curl/builds/52102142/job/2ajdluhc20r4gmmw#L530

Cherry-picked from #17413
Closes #17415
@vszakats vszakats force-pushed the w-dedupe-loadlib branch from 8522e7c to 97fbacc Compare May 22, 2025 11:06
vszakats added a commit that referenced this pull request May 23, 2025
curl no longer supports old/legacy/classic mingw.

This mitigation was addressing slow perf seen in CI with old mingw.
The slow perf is not seen in current CI with supported compilers.

Remove the duplicate DLL load function from libtest. It's no longer
used after this patch.

Current CI run times for test3026 on GHA/windows:
```
test 3026...[curl_global_init thread-safety]
 # mingw, CM clang-x86_64 gnutls libssh
 -------e--- OK (1715 out of 1738, remaining: 00:02, took 0.196s, duration: 02:55)
 # dl-mingw, CM 9.5.0-x86_64 schannel
 -------e--- OK (1554 out of 1577, remaining: 00:02, took 0.217s, duration: 02:29)
 # msvc, CM x64-windows schannel +examples
 -------e--- OK (1578 out of 1601, remaining: 00:02, took 0.205s, duration: 02:50)
```

Follow-up to 3802910 #11625
Follow-up to 856b133 #9412
Ref: #17413

Closes #17414
@vszakats vszakats force-pushed the w-dedupe-loadlib branch from c8f8af6 to 8325dc6 Compare May 23, 2025 10:00
@vszakats
Copy link
Member Author

@jay: Transformed this PR based on your suggestion to avoid loading DLLs dynamically
where possible. This means dropping the secur32 load, because XP has those
and that's the minimum curl requires. For the iphlpapi the dynamic call is necessary
when targeting pre-Vista, but for Vista and newer this PR now binds the DLL at link time
and drops the dynamic call. It also allowed to make the DLL load function static and omit
it from Vista+ builds. I've also added a WinXP build test to build the dynamic
codepath (though without running tests at this time).

This reverts commit 3d44751.

Due to slow performance. run tests step is reaching almost 10 minutes,
compared to normal 2.5 to 3 minutes.
@vszakats vszakats closed this in 0d71b18 Jun 11, 2025
@vszakats vszakats deleted the w-dedupe-loadlib branch June 11, 2025 03:41
denandz pushed a commit to denandz/curl that referenced this pull request Jun 21, 2025
- replace dynamic `InitSecurityInterface()` call with early binding and
  link `secur32` system DLL.
  The library and function are available in all supported curl Windows
  targets, meaning WinXP or newer.  Add small hack for mingw32ce to
  make it build.

- detect and use `if_nametoindex()` on Windows when available. Link
  `iphlpapi` system DLL. Requires targeting Vista or newer.
  Replacing the dynamic call and the pre-load optimization for lib3026.

Suggested-by: Jay Satiro

Closes curl#17413
vszakats pushed a commit that referenced this pull request Jun 24, 2025
Instead of CURL_WINDOWS_SSPI.

When running CMake on Windows with no additional parameters (ie default
build configuration), the generated project files do not include the
`secur32.lib` library in the linker settings. This is because
the relevant check was looking at `CURL_WINDOWS_SSPI` instead of
`USE_WINDOWS_SSPI`.

`USE_WINDOWS_SSPI` is enabled when building with SChannel (the default
on Windows), or if `CURL_WINDOWS_SSPI` is specified on the command line.

Follow-up to 0d71b18 #17413

Closes #17728
vszakats added a commit to curl/curl-for-win that referenced this pull request Jul 6, 2025
Static libcurl on Windows, built for Vista and up requires the iphlpapi
system DLL since this curl commit:

curl/curl@0d71b18
curl/curl#17413

Reported-by: Jacob Mealey
Bug: curl/trurl#395 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration feature-window A merge of this requires an open feature window tests Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

2 participants