Skip to content
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

cmake: restrict static CRT builds to static curl exe, test in CI #16456

Closed
wants to merge 1 commit into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Feb 24, 2025

Static CRT crashes MSVCR* MSVC builds (in VS2008, VS2010, VS2012,
VS2013) according to CI and local tests. The reproducible crash happens
in curl_mfprintf() → fputc(s, stderr) when trying to display the
warning message in curl -V. stderr is non-NULL and resolves to 2.
This reproducer needs a debug-enabled build, but it's unrelated to debug
features or curl's memory tracker. It happens regardless of unity build,
CPU architecture or DllMain() use. Example from VS2013:

+ _bld/src/Debug/curl.exe --disable --version
./appveyor.sh: line 124:   203 Segmentation fault      "${curl}" --disable --version

Ref: https://ci.appveyor.com/project/curlorg/curl/builds/51570451/job/ojpdqrsm1hmpmq6a#L210

Another crash happened in an UCRT build (VS2017) with a couple of
printf()s added to curl's main() function:

Microsoft Visual C++ Runtime Library
Debug Assertion Failed!
Program: C:/projects/curl/bld/src/Debug/curl.exe
File: minkernel/crts/ucrt/src/appcrt/heap/debug_heap.cpp
Line: 996
Expression: _act_first_block == header

(it hangs the job in CI due to the GUI popup)
Ref: #16394 (comment)

To avoid actual and potential issues, this patch issues a warning on
the shared-libcurl + static-CRT combination and falls back to the
default, shared CRT. IOW a static CRT build now requires a static curl
exe when using the CURL_STATIC_CRT=ON option.

Follow-up to 4fc6ebe #1621
Cherry-picked from #16394 (with more details there)

Static CRT crashes both MSVCR* and UCRT MSVC builds according to CI and
local tests. This was the reason why we had to skip `curl -V` for VS2008
in CI.

Cherry-picked from curl#16394
@vszakats vszakats added build cmake Windows Windows-specific CI Continuous Integration labels Feb 24, 2025
@vszakats vszakats closed this in edfa537 Feb 24, 2025
@vszakats vszakats deleted the cm-sh-stacrt branch February 24, 2025 20:02
vszakats added a commit that referenced this pull request Feb 24, 2025
- appveyor: restore VS2008 job, after fixing its issues.
  Enable OpenSSL in it. It takes 1 minute.
  Follow-up to 9b0467b #16453
  Follow-up to edfa537 #16456
- appveyor: make a copy of OpenSSL DLLs to have them picked up as an
  artifact (disabled by default) to aid local tests.
- appveyor: dump CMake configuration logs on failure.
- appveyor: tidy up job parameter defaults.
- GHA/windows: add pre-fill check option for dl-mingw jobs.
- GHA/windows: fix pre-fill check option for MSYS jobs by installing
  `diffutils`.
  Follow-up to e7adf3e #15841
- GHA/windows: de-duplicate to `PATH` commands for Cygwin.
- GHA/windows: drop `$SYSTEMROOT/System32` from `PATH` for Cygwin
  configure. It's not needed.
  Follow-up to 36fd2dd #13599
- list `.pdb` files in curl version step for MSVC.
  Ref: #16439

Cherry-picked from #16394
Closes #16458
@jay
Copy link
Member

jay commented Feb 25, 2025

If there are bugs with static CRT in libcurl.dll then we should address them rather than ban the combo. I am still pondering about the first bug with the stderr redirection. I think I may propose banning the curl tool from setting the option on builds that use a shared libcurl unless I can think of something better. Regarding the other bug I would need a way to reproduce out of CI so that I can debug it.

edit: note tool_init_stderr() must be called before writing to stderr (defined as tool_stderr in every unit except tool_stderr.c). the function initializes tool_stderr to the real stderr. could that be the problem? it is called in main before anything else:

curl/src/tool_main.c

Lines 234 to 243 in 57495c6

int wmain(int argc, wchar_t *argv[])
#else
int main(int argc, char *argv[])
#endif
{
CURLcode result = CURLE_OK;
struct GlobalConfig global;
memset(&global, 0, sizeof(global));
tool_init_stderr();

curl/src/tool_stderr.c

Lines 31 to 37 in 57495c6

FILE *tool_stderr;
void tool_init_stderr(void)
{
/* !checksrc! disable STDERR 1 */
tool_stderr = stderr;
}

@vszakats
Copy link
Member Author

Try reversing this commit to reproduce: d5e55fb

I added 4 printf()s in total, of which one of the first two caused this, i think the first one was before the stderr init, this one was after. But to repeat, this was printf and unless it internally wants use stderr somehow, this all goes to stdout.

As for the stderr case, the -V output is after stderr init.

If this can be refined, fine, but after 3 18 hour days chasing these I run out of interest to narrow it down further. Doing it locally can make this much faster though. For shared exe the static CRT option could indeed be limited to libcurl itself. Whether this is useful and worthy the complexity? Hard to say.

icing pushed a commit to icing/curl that referenced this pull request Feb 25, 2025
Static CRT crashes MSVCR* MSVC builds (in VS2008, VS2010, VS2012,
VS2013) according to CI and local tests. The reproducible crash happens
in `curl_mfprintf() -> fputc(s, stderr)` when trying to display the
warning message in `curl -V`. `stderr` is non-NULL and resolves to `2`.
This reproducer needs a debug-enabled build, but it's unrelated to debug
features or curl's memory tracker. It happens regardless of unity build,
CPU architecture or `DllMain()` use. Example from VS2013:

```
+ _bld/src/Debug/curl.exe --disable --version
./appveyor.sh: line 124:   203 Segmentation fault      "${curl}" --disable --version
```
Ref: https://ci.appveyor.com/project/curlorg/curl/builds/51570451/job/ojpdqrsm1hmpmq6a#L210

Another crash happened in an UCRT build (VS2017) with a couple of
`printf()`s added to curl's `main()` function:

```
Microsoft Visual C++ Runtime Library
Debug Assertion Failed!
Program: C:/projects/curl/bld/src/Debug/curl.exe
File: minkernel/crts/ucrt/src/appcrt/heap/debug_heap.cpp
Line: 996
Expression: _act_first_block == header
```
(it hangs the job in CI due to the GUI popup)
Ref: curl#16394 (comment)

To avoid actual and potential issues, this patch issues a warning on
the shared-libcurl + static-CRT combination and falls back to the
default, shared CRT. IOW a static CRT build now requires a static curl
exe when using the `CURL_STATIC_CRT=ON` option.

Follow-up to 4fc6ebe curl#1621
Cherry-picked from curl#16394 (with more details there)

Closes curl#16456
icing pushed a commit to icing/curl that referenced this pull request Feb 25, 2025
- appveyor: restore VS2008 job, after fixing its issues.
  Enable OpenSSL in it. It takes 1 minute.
  Follow-up to 9b0467b curl#16453
  Follow-up to edfa537 curl#16456
- appveyor: make a copy of OpenSSL DLLs to have them picked up as an
  artifact (disabled by default) to aid local tests.
- appveyor: dump CMake configuration logs on failure.
- appveyor: tidy up job parameter defaults.
- GHA/windows: add pre-fill check option for dl-mingw jobs.
- GHA/windows: fix pre-fill check option for MSYS jobs by installing
  `diffutils`.
  Follow-up to e7adf3e curl#15841
- GHA/windows: de-duplicate to `PATH` commands for Cygwin.
- GHA/windows: drop `$SYSTEMROOT/System32` from `PATH` for Cygwin
  configure. It's not needed.
  Follow-up to 36fd2dd curl#13599
- list `.pdb` files in curl version step for MSVC.
  Ref: curl#16439

Cherry-picked from curl#16394
Closes curl#16458
@vszakats
Copy link
Member Author

vszakats commented Feb 28, 2025

MSDN says this:
"Because a DLL built by linking to a static CRT has its own CRT state, we don't recommend you link statically to the CRT in a DLL unless the consequences are understood and desired. For example, if you call _set_se_translator in an executable that loads the DLL linked to its own static CRT, any hardware exceptions generated by the code in the DLL won't be caught by the translator, but hardware exceptions generated by code in the main executable will be caught."
https://learn.microsoft.com/en-us/cpp/c-runtime-library/crt-library-features?view=msvc-170

Also, would this help? I'm think maybe non-curl tool libcurl users might work fine with a statically linked libcurl DLL.

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -352,7 +352,7 @@ endif()
 if(WIN32)
   option(CURL_STATIC_CRT "Build libcurl with static CRT with MSVC (/MT)" OFF)
   if(CURL_STATIC_CRT AND MSVC)
-    if(BUILD_STATIC_CURL)
+    if(BUILD_STATIC_CURL OR NOT BUILD_CURL_EXE)
       set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
       string(APPEND CMAKE_C_FLAGS_RELEASE " -MT")
       string(APPEND CMAKE_C_FLAGS_DEBUG " -MTd")

@jay
Copy link
Member

jay commented Feb 28, 2025

Sure, there are disadvantages to using a static CRT. I certainly wouldn't recommend it but I don't want to ban it if we don't have to. If it doesn't work then probably there is a bug somewhere. Or CI weirdness? Don't know. That printf causing a crash makes no sense to me. I haven't tried to repro yet. It's taking a little more time than I thought to get it set up.

AFAICT we make no change to stdout except in tool_operate to set it to binary mode during transfer. The libcurl stdout is not modified in this case since it's separate crt, but it's not used so it should be fine.

For future reference I had some trouble figuring out how to retrieve the "unreachable" commit from your repo because those commits are not normally downloaded with the git fetch command. And I didn't realize this, or maybe I forgot, but the commit has to be the full commit to retrieve it. I tried the short version several times and was scratching my head. Fortunately I found this example, so basically it's this:

git fetch vszakats d5e55fb34b0e9d518235aa660886d5fda2b7fc0f:refs/remotes/vszakats/foo-commit
git checkout foo-commit
git checkout HEAD~1

(Newer git versions may allow the short format, I don't know.)

That's where I'm at so far but out of time tonight. I will look into it as time allows. No objections to your related pr, cheers

vszakats added a commit that referenced this pull request Feb 28, 2025
@vszakats
Copy link
Member Author

It also works to append a .diff or .patch extension to the URL, and curling it down:
curl -LO https://github.com/curl/curl/commit/d5e55fb34b0e9d518235aa660886d5fda2b7fc0f.patch

Barefoot, but works.

More follow-ups under #16522, with the reproducer and trying to narrow it down further in CI.

vszakats added a commit that referenced this pull request Mar 4, 2025
After this patch, we're back to 8.12.1, but disallowing
`CURL_STATIC_CRT=ON` with shared curl exe built with VS2013 or older.
Because those may crash. A stable reprducer is with `ENABLE_DEBUG=ON`
and calling `curl.exe -V`.

You can pass the necessary CMake and MSVC linker options manually,
to get around this condition.

Shared build with static UCRT may be crashing too, depending on
conditions. Consult the documentation about limitations of static CRT:
https://learn.microsoft.com/cpp/c-runtime-library/crt-library-features

Follow-up to 049352d #16516
Follow-up to edfa537 #16456
Ref: #16394
Closes #16522
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build CI Continuous Integration cmake Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

2 participants