Skip to content

Commit

Permalink
GH-15: Remove DLL pinning and let it unload gracefully
Browse files Browse the repository at this point in the history 
* call `curl_global_cleanup` to clean up curl
* call `OPENSSL_cleanup` to clean up openssl (manual cleanup since we've disabled automatic cleanup with `-DOPENSSL_NO_ATEXIT`)
  • Loading branch information
@negrutiu
negrutiu committed Aug 11, 2024
1 parent 10ae35d commit fc8a12e
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 47 deletions.
58 changes: 18 additions & 40 deletions src/nscurl/curl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@


typedef struct {
volatile HMODULE hPinnedModule;
CRITICAL_SECTION csLock;
CHAR szVersion[64];
CHAR szUserAgent[128];
} CURL_GLOBALS;
Expand Down Expand Up @@ -190,7 +188,6 @@ ULONG CurlInitialize(void)
{
TRACE( _T( "%hs()\n" ), __FUNCTION__ );

InitializeCriticalSection( &g_Curl.csLock );
{
// Plugin version
// Default user agent
Expand All @@ -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();
}


Expand Down
2 changes: 0 additions & 2 deletions src/nscurl/curl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
29 changes: 24 additions & 5 deletions src/nscurl/main.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 );
Expand Down Expand Up @@ -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 ) );
Expand Down Expand Up @@ -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 ) );
Expand Down Expand Up @@ -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 );
Expand Down Expand Up @@ -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 );
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 );

Expand Down Expand Up @@ -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 );

Expand All @@ -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 );
Expand Down

0 comments on commit fc8a12e

Please sign in to comment.