diff --git a/src/nscurl/curl.c b/src/nscurl/curl.c index de5ff85..4343cd2 100644 --- a/src/nscurl/curl.c +++ b/src/nscurl/curl.c @@ -9,8 +9,6 @@ typedef struct { - volatile HMODULE hPinnedModule; - CRITICAL_SECTION csLock; CHAR szVersion[64]; CHAR szUserAgent[128]; } CURL_GLOBALS; @@ -190,7 +188,6 @@ ULONG CurlInitialize(void) { TRACE( _T( "%hs()\n" ), __FUNCTION__ ); - InitializeCriticalSection( &g_Curl.csLock ); { // Plugin version // Default user agent @@ -201,51 +198,32 @@ ULONG CurlInitialize(void) _snprintf( g_Curl.szUserAgent, ARRAYSIZE( g_Curl.szUserAgent ), "nscurl/%s", g_Curl.szVersion ); } + curl_global_init(CURL_GLOBAL_DEFAULT); return ERROR_SUCCESS; } - //++ CurlDestroy void CurlDestroy(void) { TRACE( _T( "%hs()\n" ), __FUNCTION__ ); - DeleteCriticalSection( &g_Curl.csLock ); -} - - -//++ CurlInitializeLibcurl -ULONG CurlInitializeLibcurl(void) -{ - if (InterlockedCompareExchangePointer( (void*)&g_Curl.hPinnedModule, NULL, NULL ) == NULL) { - - TCHAR szPath[MAX_PATH]; - TRACE( _T( "%hs()\n" ), __FUNCTION__ ); - - // Initialize libcurl - curl_global_init( CURL_GLOBAL_ALL ); - - //! CAUTION: - //? https://curl.haxx.se/libcurl/c/curl_global_cleanup.html - //? curl_global_cleanup does not block waiting for any libcurl-created threads - //? to terminate (such as threads used for name resolving). If a module containing libcurl - //? is dynamically unloaded while libcurl-created threads are still running then your program - //? may crash or other corruption may occur. We recommend you do not run libcurl from any module - //? that may be unloaded dynamically. This behavior may be addressed in the future. - - // It's confirmed that NScurl.dll crashes when unloaded after curl_global_cleanup() gets called - // Easily reproducible in XP and Vista - //! We'll pin it in memory forever and never call curl_global_cleanup() - if (GetModuleFileName( g_hInst, szPath, ARRAYSIZE( szPath ) ) > 0) { - g_Curl.hPinnedModule = LoadLibrary( szPath ); - assert( g_Curl.hPinnedModule ); - } - - // kernel32!GetModuleHandleEx is available in XP+ - //x GetModuleHandleEx( GET_MODULE_HANDLE_EX_FLAG_PIN | GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS, (LPCTSTR)__FUNCTION__, (HMODULE*)&g_Curl.hPinnedModule ); - } - - return ERROR_SUCCESS; + // Before curl/7.84.0 (June 2022) it's unsafe to call curl_global_cleanup() + // from modules that are about to unload. + // It's confirmed that NScurl.dll used to crash in XP and Vista when unloaded after curl_global_cleanup(). + assert(curl_version_info(CURLVERSION_NOW)->features & CURL_VERSION_THREADSAFE); + curl_global_cleanup(); + + // [GH-15] Mitigate unload crash on older platforms (XP..8) + // The issue: + // - openssl/crypto/init.c calls `atexit(OPENSSL_cleanup)` to register its `atexit` callback + // - `OPENSSL_cleanup` routine sits in our openssl-hosted module (NScurl.dll) + // - at runtime NScurl.dll gets unloaded by the NSIS plugin framework, yet the `atexit` callback remains registered + // - at process exit, the `atexit` callback gets called, but by now it points inside an unloaded dll + // - on older platforms (XP..8) the process crashes. Newer platforms (10, 11) somehow mitigate this issue and the crash doesn't happen + // The fix: + // - we build openssl with `-DOPENSSL_NO_ATEXIT` to prevent openssl/crypto/init.c from calling `atexit(OPENSSL_cleanup)` + // - with automatic cleanup disabled, we manually call `OPENSSL_cleanup` while NScurl.dll is still loaded + OPENSSL_cleanup(); } diff --git a/src/nscurl/curl.h b/src/nscurl/curl.h index 0284990..73e615a 100644 --- a/src/nscurl/curl.h +++ b/src/nscurl/curl.h @@ -170,8 +170,6 @@ ULONG CurlRequestErrorCode( _In_ PCURL_REQUEST pReq ); //? e.g. 404 ULONG CurlInitialize(void); void CurlDestroy(void); -ULONG CurlInitializeLibcurl(void); //! Must be called only when starting transfers. This will lock NScurl.dll in memory until the process exists - //+ CurlParseRequestParam // Returns Win32 error code (ERROR_NOT_SUPPORTED for unknown parameters) ULONG CurlParseRequestParam( diff --git a/src/nscurl/main.c b/src/nscurl/main.c index 94a78b4..ae0215b 100644 --- a/src/nscurl/main.c +++ b/src/nscurl/main.c @@ -96,6 +96,8 @@ void __cdecl md5( HWND parent, int string_size, TCHAR *variables, stack_t **stac TRACE( _T( "%s!%hs\n" ), PLUGINNAME, __FUNCTION__ ); + // extra->RegisterPluginCallback(g_hInst, UnloadCallback); + psz = (LPTSTR)MyAlloc( string_size * sizeof( TCHAR ) ); assert( psz ); psz[0] = 0; @@ -132,6 +134,8 @@ void __cdecl sha1( HWND parent, int string_size, TCHAR *variables, stack_t **sta TRACE( _T( "%s!%hs\n" ), PLUGINNAME, __FUNCTION__ ); + // extra->RegisterPluginCallback(g_hInst, UnloadCallback); + psz = (LPTSTR)MyAlloc( string_size * sizeof( TCHAR ) ); assert( psz ); psz[0] = 0; @@ -168,6 +172,8 @@ void __cdecl sha256( HWND parent, int string_size, TCHAR *variables, stack_t **s TRACE( _T( "%s!%hs\n" ), PLUGINNAME, __FUNCTION__ ); + // extra->RegisterPluginCallback(g_hInst, UnloadCallback); + psz = (LPTSTR)MyAlloc( string_size * sizeof( TCHAR ) ); assert( psz ); psz[0] = 0; @@ -203,6 +209,8 @@ void __cdecl echo( HWND parent, int string_size, TCHAR *variables, stack_t **sta TRACE( _T( "%s!%hs\n" ), PLUGINNAME, __FUNCTION__ ); + // extra->RegisterPluginCallback(g_hInst, UnloadCallback); + psz = (LPTSTR)MyAlloc( string_size * sizeof( TCHAR ) ); psz2 = (LPTSTR)MyAlloc( string_size * sizeof( TCHAR ) ); assert( psz && psz2 ); @@ -242,13 +250,10 @@ void __cdecl http( HWND parent, int string_size, TCHAR *variables, stack_t **sta TRACE( _T( "%s!%hs\n" ), PLUGINNAME, __FUNCTION__ ); - // Lock the plugin in memory (against NSIS framework trying to unload it) + // Lock the plugin in memory + // NScurl.dll won't unload until the process ends extra->RegisterPluginCallback( g_hInst, UnloadCallback ); - // Lock the plugin in memory (against Windows itself trying to FreeLibrary it) - // Once curl_global_init gets called, the current module must never unload - CurlInitializeLibcurl(); - // Working structures psz = (LPTSTR)MyAlloc( string_size * sizeof(TCHAR) ); pReq = (PCURL_REQUEST)MyAlloc( sizeof( *pReq ) ); @@ -338,6 +343,8 @@ void __cdecl wait( HWND parent, int string_size, TCHAR *variables, stack_t **sta TRACE( _T( "%s!%hs\n" ), PLUGINNAME, __FUNCTION__ ); + // extra->RegisterPluginCallback(g_hInst, UnloadCallback); + // Working structures psz = (LPTSTR)MyAlloc( string_size * sizeof( TCHAR ) ); pGui = (PGUI_REQUEST)MyAlloc( sizeof( *pGui ) ); @@ -391,6 +398,8 @@ void __cdecl query( HWND parent, int string_size, TCHAR *variables, stack_t **st TRACE( _T( "%s!%hs\n" ), PLUGINNAME, __FUNCTION__ ); + // extra->RegisterPluginCallback(g_hInst, UnloadCallback); + // Working buffer psz = (LPTSTR)MyAlloc( string_size * sizeof( TCHAR ) ); assert( psz ); @@ -439,6 +448,8 @@ void __cdecl cancel( HWND parent, int string_size, TCHAR *variables, stack_t **s TRACE( _T( "%s!%hs\n" ), PLUGINNAME, __FUNCTION__ ); + // extra->RegisterPluginCallback(g_hInst, UnloadCallback); + // Working buffer psz = (LPTSTR)MyAlloc( string_size * sizeof( TCHAR ) ); assert( psz ); @@ -488,6 +499,8 @@ void __cdecl enumerate( HWND parent, int string_size, TCHAR *variables, stack_t TRACE( _T( "%s!%hs\n" ), PLUGINNAME, __FUNCTION__ ); + // extra->RegisterPluginCallback(g_hInst, UnloadCallback); + if ((psz = (LPTSTR)MyAlloc( string_size * sizeof( TCHAR ) )) != NULL) { BOOLEAN bWaiting = FALSE, bRunning = FALSE, bComplete = FALSE; @@ -557,6 +570,8 @@ void __cdecl escape( HWND parent, int string_size, TCHAR *variables, stack_t **s TRACE( _T( "%s!%hs\n" ), PLUGINNAME, __FUNCTION__ ); + // extra->RegisterPluginCallback(g_hInst, UnloadCallback); + psz = (LPTSTR)MyAlloc( string_size * sizeof(TCHAR) ); assert( psz ); @@ -588,6 +603,8 @@ void __cdecl unescape( HWND parent, int string_size, TCHAR *variables, stack_t * TRACE( _T( "%s!%hs\n" ), PLUGINNAME, __FUNCTION__ ); + // extra->RegisterPluginCallback(g_hInst, UnloadCallback); + psz = (LPTSTR)MyAlloc( string_size * sizeof(TCHAR) ); assert( psz ); @@ -612,8 +629,10 @@ EXTERN_C BOOL WINAPI DllMain( HMODULE hInst, UINT iReason, LPVOID lpReserved ) { if ( iReason == DLL_PROCESS_ATTACH ) { + TRACE(_T("DllMain( DLL_PROCESS_ATTACH )\n")); PluginInit( hInst ); } else if ( iReason == DLL_PROCESS_DETACH ) { + TRACE(_T("DllMain( DLL_PROCESS_DETACH )\n")); PluginUninit(); } UNREFERENCED_PARAMETER( lpReserved );