dllmain: Call OpenSSL thread cleanup for Windows and Cygwin #12408

jay · 2023-11-27T06:42:31Z

Call OPENSSL_thread_stop on thread termination (DLL_THREAD_DETACH) to prevent a memory leak in case OpenSSL is linked statically.
Warn in libcurl-thread.3 that in some cases OpenSSL may need a call to OPENSSL_thread_stop before thread termination in order to stop a memory leak.

OpenSSL may need per-thread cleanup to stop a memory leak. For Windows and Cygwin if libcurl was built as a DLL then we can do that for the user by calling OPENSSL_thread_stop on thread termination. However, if libcurl was built statically then we do not have notification of thread termination and cannot do that for the user.

Also, there are several other unusual cases where it may be necessary for the user to call OPENSSL_thread_stop, so in the libcurl-thread warning I added a link to the OpenSSL documentation.

Reported-by: [email protected]
Reported-by: [email protected]

Ref: https://www.openssl.org/docs/man3.0/man3/OPENSSL_thread_stop.html#NOTES

Fixes #12327
Closes #xxxx

jay · 2023-11-27T08:29:13Z

This is on hold because we're still investigating #12327

jay · 2023-11-28T08:45:27Z

@vszakats can you take a look at the unity logic please. In dllmain.c I needed to include windows.h for cygwin but I don't want that being included by other units (like if they were combined in a unity build) so I added some logic to exempt it in cmake, but I'm not sure if anything else is needed for other build systems.

vszakats · 2023-11-28T10:03:00Z

@jay: #ifdef CURL_STATICLIB probably doesn't work for this because we have it enabled for both static and shared builds now. With CMake the approach we use with libcurl.rc might work here too. With autotools the DLL_EXPORT macro tells if it's compiling for DLL (I've found no way to make the list of objects different for static and shared).

bagder · 2023-11-28T10:10:45Z

CURL_STATICLIB is about libcurl being static, not OpenSSL. What if you build libcurl dynamically with a static OpenSSL?

jay · 2023-11-28T18:53:25Z

#ifdef CURL_STATICLIB probably doesn't work for this because we have it enabled for both static and shared builds now.

CURL_STATICLIB is meant to be defined for static libcurl only. I do not think it should be defined for libcurl DLL.

curl/lib/CMakeLists.txt

Lines 112 to 118 in d755a5f

 if(WIN32) 

 # Define CURL_STATICLIB always, to disable __declspec(dllexport) for exported 

 # libcurl symbols. We handle exports via libcurl.def instead. Except with 

 # symbol hiding disabled or debug mode enabled, when we export _all_ symbols 

 # from libcurl DLL, without using libcurl.def. 

 add_definitions("-DCURL_STATICLIB") 

 endif()

That seems like a hack. How about if we introduce another symbol to handle that situation, for example

diff --git a/include/curl/curl.h b/include/curl/curl.h
index 94d9053..7028890 100644
--- a/include/curl/curl.h
+++ b/include/curl/curl.h
@@ -117,7 +117,7 @@ typedef void CURLSH;
  * libcurl external API function linkage decorations.
  */
 
-#ifdef CURL_STATICLIB
+#if defined(CURL_STATICLIB) || defined(CURL_DISALLOW_EXTERN)
 #  define CURL_EXTERN
 #elif defined(_WIN32) || \
      (__has_declspec_attribute(dllexport) && \

vszakats · 2023-11-28T19:50:54Z

IMO CURL_STATICLIB itself is a hack, and I think it shouldn't be there in the first place when compiling the lib itself. If we depend on this for anything that changes generated code, it prevents building libcurl in a single pass, IOW it forces building libcurl twice. (It's on my longer term TODO list to delete it, though I'm not quite sure yet if it's possible without breaking build scripts.)

Using CURL_DISALLOW_EXTERN as a replacement doesn't help with this.

At this point I wouldn't touch this in CMakeLists.txt because it would undo months of work and will force going back to Makefile.mk for builds with acceptable performance.

I think a better approach for CMake would be to limit compiling dllmain.c to the ${LIB_SHARED} target:

--- a/lib/CMakeLists.txt
+++ b/lib/CMakeLists.txt
@@ -182,7 +182,7 @@ if(BUILD_SHARED_LIBS)
   add_library(${LIB_SHARED} SHARED ${LIB_SOURCE})
   add_library(${PROJECT_NAME}::${LIB_SHARED} ALIAS ${LIB_SHARED})
   if(WIN32)
-    set_property(TARGET ${LIB_SHARED} APPEND PROPERTY SOURCES libcurl.rc)
+    set_property(TARGET ${LIB_SHARED} APPEND PROPERTY SOURCES dllmain.c libcurl.rc)
     if(HIDES_CURL_PRIVATE_SYMBOLS)
       set_property(TARGET ${LIB_SHARED} APPEND PROPERTY SOURCES "${CURL_SOURCE_DIR}/libcurl.def")
     endif()

The next problem is how to enable it inside dllmain.c. For autotools we need
to enable it when DLL_EXPORT is defined. I don't know what is the equivalent
for MSVC builds (or if it compiles in a single pass, like CMake does now). For CMake
builds, we can append DLL_EXPORT to COMPILE_DEFINITIONS for the
${LIB_SHARED} target and have this solved. At the same time we should stop
relying on CURL_STATICLIB for this. (I haven't tested these.)

CMake patch for the above:

--- a/lib/CMakeLists.txt
+++ b/lib/CMakeLists.txt
@@ -181,8 +181,10 @@ if(BUILD_SHARED_LIBS)
   list(APPEND libcurl_export ${LIB_SHARED})
   add_library(${LIB_SHARED} SHARED ${LIB_SOURCE})
   add_library(${PROJECT_NAME}::${LIB_SHARED} ALIAS ${LIB_SHARED})
+  unset(_shared_compile_definitions)
   if(WIN32)
-    set_property(TARGET ${LIB_SHARED} APPEND PROPERTY SOURCES libcurl.rc)
+    set_property(TARGET ${LIB_SHARED} APPEND PROPERTY SOURCES libcurl.rc dllmain.c)
+    list(APPEND _shared_compile_definitions "DLL_EXPORT")
     if(HIDES_CURL_PRIVATE_SYMBOLS)
       set_property(TARGET ${LIB_SHARED} APPEND PROPERTY SOURCES "${CURL_SOURCE_DIR}/libcurl.def")
     endif()
@@ -195,7 +197,10 @@ if(BUILD_SHARED_LIBS)
     POSITION_INDEPENDENT_CODE ON)
   if(HIDES_CURL_PRIVATE_SYMBOLS)
     set_property(TARGET ${LIB_SHARED} APPEND PROPERTY COMPILE_FLAGS "${CURL_CFLAG_SYMBOLS_HIDE}")
-    set_property(TARGET ${LIB_SHARED} APPEND PROPERTY COMPILE_DEFINITIONS "CURL_HIDDEN_SYMBOLS")
+    list(APPEND _shared_compile_definitions "CURL_HIDDEN_SYMBOLS")
+  endif()
+  if(_shared_compile_definitions)
+    set_property(TARGET ${LIB_SHARED} APPEND PROPERTY COMPILE_DEFINITIONS ${_shared_compile_definitions})
   endif()
   if(CURL_HAS_LTO)
     set_target_properties(${LIB_SHARED} PROPERTIES

vszakats · 2023-11-28T20:48:03Z

Here's a rough patch building seemingly fine with CMake (while also keeping autotools working, but not tested).

Except this:

lib/dllmain.c:41:13: warning: no previous prototype for 'DllMain' [-Wmissing-prototypes]
   41 | BOOL WINAPI DllMain(HINSTANCE hinstDLL, DWORD fdwReason, LPVOID lpvReserved)
      |             ^~~~~~~

diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt
index 51d52578e..2e99c7861 100644
--- a/lib/CMakeLists.txt
+++ b/lib/CMakeLists.txt
@@ -181,10 +181,17 @@ if(BUILD_SHARED_LIBS)
   list(APPEND libcurl_export ${LIB_SHARED})
   add_library(${LIB_SHARED} SHARED ${LIB_SOURCE})
   add_library(${PROJECT_NAME}::${LIB_SHARED} ALIAS ${LIB_SHARED})
+  unset(_shared_compile_sources)
+  unset(_shared_compile_definitions)
+  if(WIN32 OR CYGWIN)
+    set_source_files_properties(${LIB_DLLMAIN} PROPERTIES SKIP_UNITY_BUILD_INCLUSION ON)
+    list(APPEND _shared_compile_sources ${LIB_DLLMAIN})
+    list(APPEND _shared_compile_definitions "DLL_EXPORT")
+  endif()
   if(WIN32)
-    set_property(TARGET ${LIB_SHARED} APPEND PROPERTY SOURCES libcurl.rc)
+    list(APPEND _shared_compile_sources ${LIB_RCFILES})
     if(HIDES_CURL_PRIVATE_SYMBOLS)
-      set_property(TARGET ${LIB_SHARED} APPEND PROPERTY SOURCES "${CURL_SOURCE_DIR}/libcurl.def")
+      list(APPEND _shared_compile_sources "${CURL_SOURCE_DIR}/libcurl.def")
     endif()
   endif()
   target_link_libraries(${LIB_SHARED} PRIVATE ${CURL_LIBS})
@@ -195,7 +202,13 @@ if(BUILD_SHARED_LIBS)
     POSITION_INDEPENDENT_CODE ON)
   if(HIDES_CURL_PRIVATE_SYMBOLS)
     set_property(TARGET ${LIB_SHARED} APPEND PROPERTY COMPILE_FLAGS "${CURL_CFLAG_SYMBOLS_HIDE}")
-    set_property(TARGET ${LIB_SHARED} APPEND PROPERTY COMPILE_DEFINITIONS "CURL_HIDDEN_SYMBOLS")
+    list(APPEND _shared_compile_definitions "CURL_HIDDEN_SYMBOLS")
+  endif()
+  if(_shared_compile_sources)
+    set_property(TARGET ${LIB_SHARED} APPEND PROPERTY SOURCES ${_shared_compile_sources})
+  endif()
+  if(_shared_compile_definitions)
+    set_property(TARGET ${LIB_SHARED} APPEND PROPERTY COMPILE_DEFINITIONS ${_shared_compile_definitions})
   endif()
   if(CURL_HAS_LTO)
     set_target_properties(${LIB_SHARED} PROPERTIES
diff --git a/lib/Makefile.am b/lib/Makefile.am
index 1237c8e99..5ce7c5650 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -70,6 +70,8 @@ include Makefile.inc
 libcurl_la_SOURCES = $(CSOURCES) $(HHEADERS)
 libcurlu_la_SOURCES = $(CSOURCES) $(HHEADERS)
 
+libcurl_la_SOURCES += $(LIB_DLLMAIN)
+
 libcurl_la_CPPFLAGS_EXTRA =
 libcurl_la_LDFLAGS_EXTRA =
 libcurl_la_CFLAGS_EXTRA =
diff --git a/lib/Makefile.inc b/lib/Makefile.inc
index e568ef953..46af66273 100644
--- a/lib/Makefile.inc
+++ b/lib/Makefile.inc
@@ -362,6 +362,8 @@ LIB_HFILES =         \
   warnless.h         \
   ws.h
 
+LIB_DLLMAIN = dllmain.c
+
 LIB_RCFILES = libcurl.rc
 
 CSOURCES = $(LIB_CFILES) $(LIB_VAUTH_CFILES) $(LIB_VTLS_CFILES) \
--- a/lib/dllmain.c
+++ b/lib/dllmain.c
@@ -24,17 +24,15 @@
 
 #include "curl_setup.h"
 
-#if (defined(_WIN32) || defined(__CYGWIN__)) && !defined(CURL_STATICLIB)
+#if (defined(_WIN32) || defined(__CYGWIN__)) && defined(DLL_EXPORT)
 
 /* WARNING: Including Cygwin windows.h may define _WIN32 in old versions or
    may have unexpected behavior in unity builds (where all source files are
    bundled into a single unit). For that reason, this source file must be
    compiled separately for Cygwin unity builds. */
 
-#ifdef __CYGWIN__
 #define WIN32_LEAN_AND_MEAN
 #include <windows.h>
-#endif
 
 #ifdef USE_OPENSSL
 #include <openssl/crypto.h>
@@ -64,4 +62,4 @@
   return TRUE;
 }
 
-#endif /* (_WIN32 || __CYGWIN__) && !CURL_STATICLIB */
+#endif /* (_WIN32 || __CYGWIN__) && DLL_EXPORT */

jay · 2023-11-28T21:03:22Z

IMO CURL_STATICLIB itself is a hack, and I think it shouldn't be there in the first place when compiling the lib itself. If we depend on this for anything that changes generated code, it prevents building libcurl in a single pass, IOW it forces building libcurl twice. (It's on my longer term TODO list to delete it, though I'm not quite sure yet if it's possible without breaking build scripts.)

It's great that it's possible to do a single pass with CMake for both shared and static but we have to account for other build systems. Also we tell the user to define CURL_STATICLIB if libcurl is linked statically.

At this point I wouldn't touch this in CMakeLists.txt because it would undo months of work and will force going back to Makefile.mk for builds with acceptable performance.

Alright let's say we leave it win32 CURL_STATICLIB always for cmake. I'm sure there is a way to still allow cmake to do a single pass while allowing the other build systems to do their thing.

Here's a rough patch building seemingly fine with CMake (while also keeping autotools working, but not tested).

Couldn't this be addressed by leaving dllmain check as I did it (ie check for !CURL_STATICLIB) but then also telling cmake not to ~~compile~~ include a compiled dllmain.c for static builds? That preserves the behavior you added without needing changes to other build systems.

vszakats · 2023-11-28T21:53:31Z

It's great that it's possible to do a single pass with CMake for both shared and static but we have to account for other build systems. Also we tell the user to define CURL_STATICLIB if libcurl is linked statically.

CURL_STATICLIB in user code is a different matter. There it's inevitable due to
how Windows designed DLL linking. I'm saying that it's an unnecessary hack when
building the lib itself.

Alright let's say we leave it win32 CURL_STATICLIB always for cmake. I'm sure there is a way to still allow cmake to do a single pass while allowing the other build systems to do their thing.

That's what my patch tries to do. Except for native MSVC builds which I'm
unfamiliar with. If it's doing multiple passes, DLL_EXPORT can be passed to
the shared pass, if there is one, dllmain.c can be limited to the shared lib, or if
this isn't possible, needs switching to two passes (a terrible waste for a single
line needed by some OpenSSL scenarios).

Couldn't this be addressed by leaving dllmain check as I did it (ie check for !CURL_STATICLIB) but then also telling cmake not to ~~compile~~ include a compiled dllmain.c for static builds? That preserves the behavior you added without needing changes to other build systems.

Maybe it can be done, but I find that more confusing than the proposal above.
I'd avoid using CURL_STATICLIB for more stuff, it's already head-spinning
to follow.

vszakats · 2023-11-28T21:56:37Z

CURL_STATICLIB is about libcurl being static, not OpenSSL. What if you build libcurl dynamically with a static OpenSSL?

Official libcurl.dll is doing that, might be useful to cover.

(Can't add much to this though, I'm lost when OpenSSL requires this deinit call.)

vszakats · 2023-11-28T22:29:44Z

Here's one take reusing CURL_STATICLIB. Works with the happy-path, buts need (much) more testing before merging:

Notably it drops CURL_STATICLIB from all shared compiles when using SHARE_LIB_OBJECT=OFF, which may or may not have unintended side-effects when used together with the .def file, for example.

(works with the unmodified dllmain.c)

diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt
index 51d52578e..e0ce3eacd 100644
--- a/lib/CMakeLists.txt
+++ b/lib/CMakeLists.txt
@@ -114,23 +114,26 @@ if(NOT DEFINED SHARE_LIB_OBJECT)
   endif()
 endif()
 
-if(WIN32)
-  # Define CURL_STATICLIB always, to disable __declspec(dllexport) for exported
-  # libcurl symbols. We handle exports via libcurl.def instead. Except with
-  # symbol hiding disabled or debug mode enabled, when we export _all_ symbols
-  # from libcurl DLL, without using libcurl.def.
-  add_definitions("-DCURL_STATICLIB")
-endif()
-
 if(SHARE_LIB_OBJECT)
   set(LIB_OBJECT "libcurl_object")
   add_library(${LIB_OBJECT} OBJECT ${HHEADERS} ${CSOURCES})
+  unset(_object_compile_definitions)
+  if(WIN32)
+    # Define CURL_STATICLIB always, to disable __declspec(dllexport) for exported
+    # libcurl symbols. We handle exports via libcurl.def instead. Except with
+    # symbol hiding disabled or debug mode enabled, when we export _all_ symbols
+    # from libcurl DLL, without using libcurl.def.
+    list(APPEND _object_compile_definitions "CURL_STATICLIB")
+  endif()
   target_link_libraries(${LIB_OBJECT} PRIVATE ${CURL_LIBS})
   set_target_properties(${LIB_OBJECT} PROPERTIES
     POSITION_INDEPENDENT_CODE ON)
   if(HIDES_CURL_PRIVATE_SYMBOLS)
     set_property(TARGET ${LIB_OBJECT} APPEND PROPERTY COMPILE_FLAGS "${CURL_CFLAG_SYMBOLS_HIDE}")
-    set_property(TARGET ${LIB_OBJECT} APPEND PROPERTY COMPILE_DEFINITIONS "CURL_HIDDEN_SYMBOLS")
+    list(APPEND _object_compile_definitions "CURL_HIDDEN_SYMBOLS")
+  endif()
+  if(_object_compile_definitions)
+    set_property(TARGET ${LIB_OBJECT} APPEND PROPERTY COMPILE_DEFINITIONS ${_object_compile_definitions})
   endif()
   if(CURL_HAS_LTO)
     set_target_properties(${LIB_OBJECT} PROPERTIES
@@ -152,6 +155,10 @@ if(BUILD_STATIC_LIBS)
   list(APPEND libcurl_export ${LIB_STATIC})
   add_library(${LIB_STATIC} STATIC ${LIB_SOURCE})
   add_library(${PROJECT_NAME}::${LIB_STATIC} ALIAS ${LIB_STATIC})
+  unset(_static_compile_definitions)
+  if(WIN32)
+    list(APPEND _static_compile_definitions "CURL_STATICLIB")
+  endif()
   target_link_libraries(${LIB_STATIC} PRIVATE ${CURL_LIBS})
   # Remove the "lib" prefix since the library is already named "libcurl".
   set_target_properties(${LIB_STATIC} PROPERTIES
@@ -160,7 +167,10 @@ if(BUILD_STATIC_LIBS)
     INTERFACE_COMPILE_DEFINITIONS "CURL_STATICLIB")
   if(HIDES_CURL_PRIVATE_SYMBOLS)
     set_property(TARGET ${LIB_STATIC} APPEND PROPERTY COMPILE_FLAGS "${CURL_CFLAG_SYMBOLS_HIDE}")
-    set_property(TARGET ${LIB_STATIC} APPEND PROPERTY COMPILE_DEFINITIONS "CURL_HIDDEN_SYMBOLS")
+    list(APPEND _static_compile_definitions "CURL_HIDDEN_SYMBOLS")
+  endif()
+  if(_static_compile_definitions)
+    set_property(TARGET ${LIB_STATIC} APPEND PROPERTY COMPILE_DEFINITIONS ${_static_compile_definitions})
   endif()
   if(CURL_HAS_LTO)
     set_target_properties(${LIB_STATIC} PROPERTIES
@@ -181,10 +191,15 @@ if(BUILD_SHARED_LIBS)
   list(APPEND libcurl_export ${LIB_SHARED})
   add_library(${LIB_SHARED} SHARED ${LIB_SOURCE})
   add_library(${PROJECT_NAME}::${LIB_SHARED} ALIAS ${LIB_SHARED})
+  unset(_shared_compile_sources)
+  if(WIN32 OR CYGWIN)
+    set_source_files_properties(${LIB_DLLMAIN} PROPERTIES SKIP_UNITY_BUILD_INCLUSION ON)
+    list(APPEND _shared_compile_sources ${LIB_DLLMAIN})
+  endif()
   if(WIN32)
-    set_property(TARGET ${LIB_SHARED} APPEND PROPERTY SOURCES libcurl.rc)
+    list(APPEND _shared_compile_sources ${LIB_RCFILES})
     if(HIDES_CURL_PRIVATE_SYMBOLS)
-      set_property(TARGET ${LIB_SHARED} APPEND PROPERTY SOURCES "${CURL_SOURCE_DIR}/libcurl.def")
+      list(APPEND _shared_compile_sources "${CURL_SOURCE_DIR}/libcurl.def")
     endif()
   endif()
   target_link_libraries(${LIB_SHARED} PRIVATE ${CURL_LIBS})
@@ -197,6 +212,9 @@ if(BUILD_SHARED_LIBS)
     set_property(TARGET ${LIB_SHARED} APPEND PROPERTY COMPILE_FLAGS "${CURL_CFLAG_SYMBOLS_HIDE}")
     set_property(TARGET ${LIB_SHARED} APPEND PROPERTY COMPILE_DEFINITIONS "CURL_HIDDEN_SYMBOLS")
   endif()
+  if(_shared_compile_sources)
+    set_property(TARGET ${LIB_SHARED} APPEND PROPERTY SOURCES ${_shared_compile_sources})
+  endif()
   if(CURL_HAS_LTO)
     set_target_properties(${LIB_SHARED} PROPERTIES
       INTERPROCEDURAL_OPTIMIZATION_RELEASE TRUE

vszakats · 2023-11-29T11:36:44Z

OPENSSL_thread_stop is not implemented by these forks (as of their current latest versions): LibreSSL, BoringSSL, AWS-LC. And also wolfSSL.

jay · 2023-12-03T08:26:53Z

@vszakats I made some changes:

dllmain.c is now added only to CMake Windows and Cygwin SHARED (DLL) builds and built with CURL_CMAKE_DLLMAIN. When both shared and static libs are built by CMake it will still do a single pass.

OPENSSL_thread_stop() is now not called for LibreSSL, BoringSSL, AWS-LC. Is USE_OPENSSL true ever for wolfSSL? Also, I notice some of openssl.c is built if USE_QUICHE even if not USE_OPENSSL so does that mean quiche has its own OpenSSL?

I temporarily added a MessageBox on THREAD_DETACH for debugging purposes.

vszakats · 2023-12-03T11:40:23Z

I tested it and it revealed two things:

CMake excluded dllmain.c also for MINGW due to the custom define.
set_property(TARGET ... APPEND PROPERTY is additive. Earlier tests left the impression that these overrided each other, but this doesn't seem to be the case.

This lead to this patch (over the current PR), which stops setting CURL_STATICLIB for dllmain.c, thus also making the new CURL_CMAKE_DLLMAIN unnecessary and simplifying things a bit and making things more aligned with other build systems, while allowing to build dllmain.c in unity mode with MINGW:

diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt
index 39b744d89..8e95f0477 100644
--- a/lib/CMakeLists.txt
+++ b/lib/CMakeLists.txt
@@ -33,8 +33,6 @@ include(${CMAKE_CURRENT_BINARY_DIR}/Makefile.inc.cmake)
 
 # DllMain is added later for DLL builds only.
 list(REMOVE_ITEM CSOURCES dllmain.c)
-set_source_files_properties(dllmain.c PROPERTIES
-  COMPILE_FLAGS -DCURL_CMAKE_DLLMAIN)
 
 list(APPEND HHEADERS ${CMAKE_CURRENT_BINARY_DIR}/curl_config.h)
 
@@ -66,13 +64,6 @@ if(ENABLE_CURLDEBUG)
   set_source_files_properties(memdebug.c curl_multibyte.c PROPERTIES SKIP_UNITY_BUILD_INCLUSION ON)
 endif()
 
-if(CYGWIN)
-  # For cygwin always compile dllmain.c as a separate unit since it includes
-  # windows.h, which shouldn't be included in other units.
-  set_source_files_properties(dllmain.c PROPERTIES
-    SKIP_UNITY_BUILD_INCLUSION ON)
-endif()
-
 if(BUILD_TESTING)
   target_link_libraries(curlu PRIVATE ${CURL_LIBS})
 endif()
@@ -124,17 +115,16 @@ if(NOT DEFINED SHARE_LIB_OBJECT)
   endif()
 endif()
 
-if(WIN32)
-  # Define CURL_STATICLIB always, to disable __declspec(dllexport) for exported
-  # libcurl symbols. We handle exports via libcurl.def instead. Except with
-  # symbol hiding disabled or debug mode enabled, when we export _all_ symbols
-  # from libcurl DLL, without using libcurl.def.
-  add_definitions("-DCURL_STATICLIB")
-endif()
-
 if(SHARE_LIB_OBJECT)
   set(LIB_OBJECT "libcurl_object")
   add_library(${LIB_OBJECT} OBJECT ${HHEADERS} ${CSOURCES})
+  if(WIN32)
+    # Define CURL_STATICLIB always, to disable __declspec(dllexport) for exported
+    # libcurl symbols. We handle exports via libcurl.def instead. Except with
+    # symbol hiding disabled or debug mode enabled, when we export _all_ symbols
+    # from libcurl DLL, without using libcurl.def.
+    set_property(TARGET ${LIB_OBJECT} APPEND PROPERTY COMPILE_DEFINITIONS "CURL_STATICLIB")
+  endif()
   target_link_libraries(${LIB_OBJECT} PRIVATE ${CURL_LIBS})
   set_target_properties(${LIB_OBJECT} PROPERTIES
     POSITION_INDEPENDENT_CODE ON)
@@ -162,6 +152,9 @@ if(BUILD_STATIC_LIBS)
   list(APPEND libcurl_export ${LIB_STATIC})
   add_library(${LIB_STATIC} STATIC ${LIB_SOURCE})
   add_library(${PROJECT_NAME}::${LIB_STATIC} ALIAS ${LIB_STATIC})
+  if(WIN32)
+    set_property(TARGET ${LIB_OBJECT} APPEND PROPERTY COMPILE_DEFINITIONS "CURL_STATICLIB")
+  endif()
   target_link_libraries(${LIB_STATIC} PRIVATE ${CURL_LIBS})
   # Remove the "lib" prefix since the library is already named "libcurl".
   set_target_properties(${LIB_STATIC} PROPERTIES
@@ -192,6 +185,11 @@ if(BUILD_SHARED_LIBS)
   add_library(${LIB_SHARED} SHARED ${LIB_SOURCE})
   add_library(${PROJECT_NAME}::${LIB_SHARED} ALIAS ${LIB_SHARED})
   if(WIN32 OR CYGWIN)
+    if(CYGWIN)
+      # For cygwin always compile dllmain.c as a separate unit since it includes
+      # windows.h, which shouldn't be included in other units.
+      set_source_files_properties(dllmain.c PROPERTIES SKIP_UNITY_BUILD_INCLUSION ON)
+    endif()
     set_property(TARGET ${LIB_SHARED} APPEND PROPERTY SOURCES dllmain.c)
   endif()
   if(WIN32)
diff --git a/lib/dllmain.c b/lib/dllmain.c
index 6d9753e43..5f5a241a5 100644
--- a/lib/dllmain.c
+++ b/lib/dllmain.c
@@ -44,14 +44,10 @@
 
 /*
  * DllMain() must only be defined for Windows and Cygwin DLL builds. For most
- * build systems that means CURL_STATICLIB is not defined. However, CMake
- * defines CURL_STATICLIB always for Windows builds (discussion in #12408).
- * CMake builds dllmain.c only for Windows and Cygwin DLL builds and defines
- * CURL_CMAKE_DLLMAIN.
+ * build systems that means CURL_STATICLIB is not defined.
  */
 
-#if (defined(_WIN32) || defined(__CYGWIN__)) && \
-    (!defined(CURL_STATICLIB) || defined(CURL_CMAKE_DLLMAIN))
+#if (defined(_WIN32) || defined(__CYGWIN__)) && !defined(CURL_STATICLIB)
 
 BOOL WINAPI DllMain(HINSTANCE hinstDLL, DWORD fdwReason, LPVOID lpvReserved);

jay · 2024-04-21T19:37:09Z

Some of these word checks are snobby. It doesn't like the word will. It doesn't like the word very. It doesn't even like It's. I mean who doesn't like It's.

lib/dllmain.c

- Call OPENSSL_thread_stop on thread termination (DLL_THREAD_DETACH) to prevent a memory leak in case OpenSSL is linked statically. - Warn in libcurl-thread.3 that if OpenSSL is linked statically then it may require thread cleanup. OpenSSL may need per-thread cleanup to stop a memory leak. For Windows and Cygwin if libcurl was built as a DLL then we can do that for the user by calling OPENSSL_thread_stop on thread termination. However, if libcurl was built statically then we do not have notification of thread termination and cannot do that for the user. Also, there are several other unusual cases where it may be necessary for the user to call OPENSSL_thread_stop, so in the libcurl-thread warning I added a link to the OpenSSL documentation. Co-authored-by: Viktor Szakats Reported-by: [email protected] Reported-by: [email protected] Ref: https://www.openssl.org/docs/man3.0/man3/OPENSSL_thread_stop.html#NOTES Fixes curl#12327 Closes #xxxx

jay added the documentation label Nov 27, 2023

github-actions bot added the libcurl API label Nov 27, 2023

bagder approved these changes Nov 27, 2023

View reviewed changes

jay added the on-hold label Nov 27, 2023

jay force-pushed the ossl_thread_warn branch from b79f675 to 3b2522f Compare November 28, 2023 08:39

jay removed the on-hold label Nov 28, 2023

jay changed the title ~~libcurl-thread.3: Warn that OpenSSL may need extra cleanup~~ dllmain: Call OpenSSL thread cleanup for Windows and Cygwin Nov 28, 2023

jay mentioned this pull request Nov 28, 2023

libcurl.dll memory leak #12327

Closed

vszakats added the Windows Windows-specific label Nov 28, 2023

jay force-pushed the ossl_thread_warn branch from f3eab30 to 31f5c76 Compare April 13, 2024 19:44

jay force-pushed the ossl_thread_warn branch from 31f5c76 to 57b4655 Compare April 21, 2024 18:38

vszakats reviewed Apr 22, 2024

View reviewed changes

lib/dllmain.c Outdated Show resolved Hide resolved

jay force-pushed the ossl_thread_warn branch from 91d4a4c to 9f573f4 Compare April 22, 2024 21:03

fixup cmake line wrap

d2a6930

vszakats approved these changes Apr 23, 2024

View reviewed changes

jay closed this in 7860f57 Apr 24, 2024

jay deleted the ossl_thread_warn branch April 24, 2024 08:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dllmain: Call OpenSSL thread cleanup for Windows and Cygwin #12408

dllmain: Call OpenSSL thread cleanup for Windows and Cygwin #12408

jay commented Nov 27, 2023 •

edited

jay commented Nov 27, 2023

jay commented Nov 28, 2023

vszakats commented Nov 28, 2023 •

edited

bagder commented Nov 28, 2023

jay commented Nov 28, 2023 •

edited

vszakats commented Nov 28, 2023 •

edited

vszakats commented Nov 28, 2023 •

edited

jay commented Nov 28, 2023 •

edited

vszakats commented Nov 28, 2023 •

edited

vszakats commented Nov 28, 2023

vszakats commented Nov 28, 2023 •

edited

vszakats commented Nov 29, 2023 •

edited

jay commented Dec 3, 2023

vszakats commented Dec 3, 2023

jay commented Apr 21, 2024

dllmain: Call OpenSSL thread cleanup for Windows and Cygwin #12408

dllmain: Call OpenSSL thread cleanup for Windows and Cygwin #12408

Conversation

jay commented Nov 27, 2023 • edited

jay commented Nov 27, 2023

jay commented Nov 28, 2023

vszakats commented Nov 28, 2023 • edited

bagder commented Nov 28, 2023

jay commented Nov 28, 2023 • edited

vszakats commented Nov 28, 2023 • edited

vszakats commented Nov 28, 2023 • edited

jay commented Nov 28, 2023 • edited

vszakats commented Nov 28, 2023 • edited

vszakats commented Nov 28, 2023

vszakats commented Nov 28, 2023 • edited

vszakats commented Nov 29, 2023 • edited

jay commented Dec 3, 2023

vszakats commented Dec 3, 2023

jay commented Apr 21, 2024

jay commented Nov 27, 2023 •

edited

vszakats commented Nov 28, 2023 •

edited

jay commented Nov 28, 2023 •

edited

vszakats commented Nov 28, 2023 •

edited

vszakats commented Nov 28, 2023 •

edited

jay commented Nov 28, 2023 •

edited

vszakats commented Nov 28, 2023 •

edited

vszakats commented Nov 28, 2023 •

edited

vszakats commented Nov 29, 2023 •

edited