[PATCH v2 1/7] ntdll: Protect module list access in LdrFindEntryForAddress() with SRW lock.
Fixes racy access to LDR lists in lookup_function_info(). Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- Supersedes 215952-215974. dlls/ntdll/actctx.c | 6 ------ dlls/ntdll/loader.c | 21 ++++++++++++++++++--- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/dlls/ntdll/actctx.c b/dlls/ntdll/actctx.c index f9afb655885..bc222dadf27 100644 --- a/dlls/ntdll/actctx.c +++ b/dlls/ntdll/actctx.c @@ -705,10 +705,8 @@ static inline const char* debugstr_version(const struct assembly_version *ver) static NTSTATUS get_module_filename( HMODULE module, UNICODE_STRING *str, unsigned int extra_len ) { NTSTATUS status; - ULONG_PTR magic; LDR_DATA_TABLE_ENTRY *pldr; - LdrLockLoaderLock(0, NULL, &magic); status = LdrFindEntryForAddress( module, &pldr ); if (status == STATUS_SUCCESS) { @@ -721,7 +719,6 @@ static NTSTATUS get_module_filename( HMODULE module, UNICODE_STRING *str, unsign } else status = STATUS_NO_MEMORY; } - LdrUnlockLoaderLock(0, magic); return status; } @@ -3301,12 +3298,10 @@ static NTSTATUS find_query_actctx( HANDLE *handle, DWORD flags, ULONG class ) } else if (flags & (QUERY_ACTCTX_FLAG_ACTCTX_IS_ADDRESS|QUERY_ACTCTX_FLAG_ACTCTX_IS_HMODULE)) { - ULONG_PTR magic; LDR_DATA_TABLE_ENTRY *pldr; if (!*handle) return STATUS_INVALID_PARAMETER; - LdrLockLoaderLock( 0, NULL, &magic ); if (!LdrFindEntryForAddress( *handle, &pldr )) { if ((flags & QUERY_ACTCTX_FLAG_ACTCTX_IS_HMODULE) && *handle != pldr->DllBase) @@ -3315,7 +3310,6 @@ static NTSTATUS find_query_actctx( HANDLE *handle, DWORD flags, ULONG class ) *handle = pldr->ActivationContext; } else status = STATUS_DLL_NOT_FOUND; - LdrUnlockLoaderLock( 0, magic ); } else if (!*handle && (class != ActivationContextBasicInformation)) *handle = process_actctx; diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index e00c1e0c31d..55a6213d906 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -176,6 +176,10 @@ static PEB_LDR_DATA ldr = { &ldr.InMemoryOrderModuleList, &ldr.InMemoryOrderModuleList }, { &ldr.InInitializationOrderModuleList, &ldr.InInitializationOrderModuleList } }; +/* Ldr data is modified with loader locked and exclusive lock held. + * Taking shared lock to access the data is required outside of loader lock only. + */ +static RTL_SRWLOCK ldr_data_srw_lock = RTL_SRWLOCK_INIT; static RTL_BITMAP tls_bitmap; static RTL_BITMAP tls_expansion_bitmap; @@ -1245,10 +1249,12 @@ static WINE_MODREF *alloc_module( HMODULE hModule, const UNICODE_STRING *nt_name wm->ldr.EntryPoint = (char *)hModule + nt->OptionalHeader.AddressOfEntryPoint; } + RtlAcquireSRWLockExclusive( &ldr_data_srw_lock ); InsertTailList(&NtCurrentTeb()->Peb->LdrData->InLoadOrderModuleList, &wm->ldr.InLoadOrderLinks); InsertTailList(&NtCurrentTeb()->Peb->LdrData->InMemoryOrderModuleList, &wm->ldr.InMemoryOrderLinks); + RtlReleaseSRWLockExclusive( &ldr_data_srw_lock ); /* wait until init is called for inserting into InInitializationOrderModuleList */ if (!(nt->OptionalHeader.DllCharacteristics & IMAGE_DLLCHARACTERISTICS_NX_COMPAT)) @@ -1454,8 +1460,12 @@ static NTSTATUS process_attach( WINE_MODREF *wm, LPVOID lpReserved ) } if (!wm->ldr.InInitializationOrderLinks.Flink) + { + RtlAcquireSRWLockExclusive( &ldr_data_srw_lock ); InsertTailList(&NtCurrentTeb()->Peb->LdrData->InInitializationOrderModuleList, &wm->ldr.InInitializationOrderLinks); + RtlReleaseSRWLockExclusive( &ldr_data_srw_lock ); + } /* Call DLL entry point */ if (status == STATUS_SUCCESS) @@ -1578,13 +1588,14 @@ NTSTATUS WINAPI LdrDisableThreadCalloutsForDll(HMODULE hModule) /****************************************************************** * LdrFindEntryForAddress (NTDLL.@) * - * The loader_section must be locked while calling this function */ NTSTATUS WINAPI LdrFindEntryForAddress( const void *addr, PLDR_DATA_TABLE_ENTRY *pmod ) { + NTSTATUS ret = STATUS_NO_MORE_ENTRIES; PLIST_ENTRY mark, entry; PLDR_DATA_TABLE_ENTRY mod; + RtlAcquireSRWLockShared( &ldr_data_srw_lock ); mark = &NtCurrentTeb()->Peb->LdrData->InMemoryOrderModuleList; for (entry = mark->Flink; entry != mark; entry = entry->Flink) { @@ -1593,10 +1604,12 @@ NTSTATUS WINAPI LdrFindEntryForAddress( const void *addr, PLDR_DATA_TABLE_ENTRY (const char *)addr < (char*)mod->DllBase + mod->SizeOfImage) { *pmod = mod; - return STATUS_SUCCESS; + ret = STATUS_SUCCESS; + break; } } - return STATUS_NO_MORE_ENTRIES; + RtlReleaseSRWLockShared( &ldr_data_srw_lock ); + return ret; } /****************************************************************** @@ -3510,10 +3523,12 @@ void WINAPI LdrShutdownThread(void) */ static void free_modref( WINE_MODREF *wm ) { + RtlAcquireSRWLockExclusive( &ldr_data_srw_lock ); RemoveEntryList(&wm->ldr.InLoadOrderLinks); RemoveEntryList(&wm->ldr.InMemoryOrderLinks); if (wm->ldr.InInitializationOrderLinks.Flink) RemoveEntryList(&wm->ldr.InInitializationOrderLinks); + RtlReleaseSRWLockExclusive( &ldr_data_srw_lock ); TRACE(" unloading %s\n", debugstr_w(wm->ldr.FullDllName.Buffer)); if (!TRACE_ON(module)) -- 2.31.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntoskrnl.exe/ntoskrnl.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c index f2fb0a6d66e..73e8a7b5734 100644 --- a/dlls/ntoskrnl.exe/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/ntoskrnl.c @@ -3636,15 +3636,12 @@ error: static LDR_DATA_TABLE_ENTRY *find_ldr_module( HMODULE module ) { LDR_DATA_TABLE_ENTRY *ldr; - ULONG_PTR magic; - LdrLockLoaderLock( 0, NULL, &magic ); if (LdrFindEntryForAddress( module, &ldr )) { WARN( "module not found for %p\n", module ); ldr = NULL; } - LdrUnlockLoaderLock( 0, magic ); return ldr; } -- 2.31.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 120 ++++++++++++++++++++++++++------------------ 1 file changed, 70 insertions(+), 50 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 55a6213d906..776a88e0050 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -208,6 +208,26 @@ static inline BOOL contains_path( LPCWSTR name ) return ((*name && (name[1] == ':')) || wcschr(name, '/') || wcschr(name, '\\')); } +/************************************************************************* + * lock_loader_exclusive + * + * Take exclusive loader lock. + */ +static void lock_loader_exclusive(void) +{ + RtlEnterCriticalSection( &loader_section ); +} + +/************************************************************************* + * unlock_loader + * + * Release loader lock. + */ +static void unlock_loader(void) +{ + RtlLeaveCriticalSection( &loader_section ); +} + #define RTL_UNLOAD_EVENT_TRACE_NUMBER 64 typedef struct _RTL_UNLOAD_EVENT_TRACE @@ -481,7 +501,7 @@ static void call_ldr_notifications( ULONG reason, LDR_DATA_TABLE_ENTRY *module ) * get_modref * * Looks for the referenced HMODULE in the current process - * The loader_section must be locked while calling this function. + * The loader must be locked while calling this function. */ static WINE_MODREF *get_modref( HMODULE hmod ) { @@ -505,7 +525,7 @@ static WINE_MODREF *get_modref( HMODULE hmod ) * find_basename_module * * Find a module from its base name. - * The loader_section must be locked while calling this function + * The loader must be locked while calling this function */ static WINE_MODREF *find_basename_module( LPCWSTR name ) { @@ -535,7 +555,7 @@ static WINE_MODREF *find_basename_module( LPCWSTR name ) * find_fullname_module * * Find a module from its full path name. - * The loader_section must be locked while calling this function + * The loader must be locked while calling this function */ static WINE_MODREF *find_fullname_module( const UNICODE_STRING *nt_name ) { @@ -567,7 +587,7 @@ static WINE_MODREF *find_fullname_module( const UNICODE_STRING *nt_name ) * find_fileid_module * * Find a module from its file id. - * The loader_section must be locked while calling this function + * The loader must be locked while calling this function */ static WINE_MODREF *find_fileid_module( const struct file_id *id ) { @@ -616,7 +636,7 @@ static WINE_MODREF **grow_module_deps( WINE_MODREF *wm, int count ) * find_forwarded_export * * Find the final function pointer for a forwarded function. - * The loader_section must be locked while calling this function. + * The loader must be locked while calling this function. */ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWSTR load_path ) { @@ -694,7 +714,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS * * Find an exported function by ordinal. * The exports base must have been subtracted from the ordinal already. - * The loader_section must be locked while calling this function. + * The loader must be locked while calling this function. */ static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, DWORD exp_size, DWORD ordinal, LPCWSTR load_path ) @@ -757,7 +777,7 @@ static int find_name_in_exports( HMODULE module, const IMAGE_EXPORT_DIRECTORY *e * find_named_export * * Find an exported function by name. - * The loader_section must be locked while calling this function. + * The loader must be locked while calling this function. */ static FARPROC find_named_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, DWORD exp_size, const char *name, int hint, LPCWSTR load_path ) @@ -806,7 +826,7 @@ void * WINAPI RtlFindExportedRoutineByName( HMODULE module, const char *name ) * import_dll * * Import the dll specified by the given import descriptor. - * The loader_section must be locked while calling this function. + * The loader must be locked while calling this function. */ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LPCWSTR load_path, WINE_MODREF **pwm ) { @@ -1015,7 +1035,7 @@ static BOOL is_dll_native_subsystem( LDR_DATA_TABLE_ENTRY *mod, const IMAGE_NT_H * alloc_tls_slot * * Allocate a TLS slot for a newly-loaded module. - * The loader_section must be locked while calling this function. + * The loader must be locked while calling this function. */ static SHORT alloc_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) { @@ -1099,7 +1119,7 @@ static SHORT alloc_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) * free_tls_slot * * Free the module TLS slot on unload. - * The loader_section must be locked while calling this function. + * The loader must be locked while calling this function. */ static void free_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) { @@ -1115,7 +1135,7 @@ static void free_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) * fixup_imports_ilonly * * Fixup imports for an IL-only module. All we do is import mscoree. - * The loader_section must be locked while calling this function. + * The loader must be locked while calling this function. */ static NTSTATUS fixup_imports_ilonly( WINE_MODREF *wm, LPCWSTR load_path, void **entry ) { @@ -1154,7 +1174,7 @@ static NTSTATUS fixup_imports_ilonly( WINE_MODREF *wm, LPCWSTR load_path, void * * fixup_imports * * Fixup all imports of a given module. - * The loader_section must be locked while calling this function. + * The loader must be locked while calling this function. */ static NTSTATUS fixup_imports( WINE_MODREF *wm, LPCWSTR load_path ) { @@ -1210,7 +1230,7 @@ static NTSTATUS fixup_imports( WINE_MODREF *wm, LPCWSTR load_path ) * alloc_module * * Allocate a WINE_MODREF structure and add it to the process list - * The loader_section must be locked while calling this function. + * The loader must be locked while calling this function. */ static WINE_MODREF *alloc_module( HMODULE hModule, const UNICODE_STRING *nt_name, BOOL builtin ) { @@ -1430,7 +1450,7 @@ static NTSTATUS MODULE_InitDLL( WINE_MODREF *wm, UINT reason, LPVOID lpReserved * detach notifications are called in the reverse of the sequence the attach * notifications *returned*. * - * The loader_section must be locked while calling this function. + * The loader must be locked while calling this function. */ static NTSTATUS process_attach( WINE_MODREF *wm, LPVOID lpReserved ) { @@ -1542,7 +1562,7 @@ static void process_detach(void) * * Send DLL thread attach notifications. These are sent in the * reverse sequence of process detach notification. - * The loader_section must be locked while calling this function. + * The loader must be locked while calling this function. */ static void thread_attach(void) { @@ -1572,7 +1592,7 @@ NTSTATUS WINAPI LdrDisableThreadCalloutsForDll(HMODULE hModule) WINE_MODREF *wm; NTSTATUS ret = STATUS_SUCCESS; - RtlEnterCriticalSection( &loader_section ); + lock_loader_exclusive(); wm = get_modref( hModule ); if (!wm || wm->ldr.TlsIndex != -1) @@ -1580,7 +1600,7 @@ NTSTATUS WINAPI LdrDisableThreadCalloutsForDll(HMODULE hModule) else wm->ldr.Flags |= LDR_NO_DLL_CALLS; - RtlLeaveCriticalSection( &loader_section ); + unlock_loader(); return ret; } @@ -1626,7 +1646,7 @@ NTSTATUS WINAPI LdrEnumerateLoadedModules( void *unknown, LDRENUMPROC callback, if (unknown || !callback) return STATUS_INVALID_PARAMETER; - RtlEnterCriticalSection( &loader_section ); + lock_loader_exclusive(); mark = &NtCurrentTeb()->Peb->LdrData->InMemoryOrderModuleList; for (entry = mark->Flink; entry != mark; entry = entry->Flink) @@ -1636,7 +1656,7 @@ NTSTATUS WINAPI LdrEnumerateLoadedModules( void *unknown, LDRENUMPROC callback, if (stop) break; } - RtlLeaveCriticalSection( &loader_section ); + unlock_loader(); return STATUS_SUCCESS; } @@ -1661,9 +1681,9 @@ NTSTATUS WINAPI LdrRegisterDllNotification(ULONG flags, PLDR_DLL_NOTIFICATION_FU notify->callback = callback; notify->context = context; - RtlEnterCriticalSection( &loader_section ); + lock_loader_exclusive(); list_add_tail( &ldr_notifications, ¬ify->entry ); - RtlLeaveCriticalSection( &loader_section ); + unlock_loader(); *cookie = notify; return STATUS_SUCCESS; @@ -1680,9 +1700,9 @@ NTSTATUS WINAPI LdrUnregisterDllNotification( void *cookie ) if (!notify) return STATUS_INVALID_PARAMETER; - RtlEnterCriticalSection( &loader_section ); + lock_loader_exclusive(); list_remove( ¬ify->entry ); - RtlLeaveCriticalSection( &loader_section ); + unlock_loader(); RtlFreeHeap( GetProcessHeap(), 0, notify ); return STATUS_SUCCESS; @@ -1747,7 +1767,7 @@ NTSTATUS WINAPI LdrGetProcedureAddress(HMODULE module, const ANSI_STRING *name, DWORD exp_size; NTSTATUS ret = STATUS_PROCEDURE_NOT_FOUND; - RtlEnterCriticalSection( &loader_section ); + lock_loader_exclusive(); /* check if the module itself is invalid to return the proper error */ if (!get_modref( module )) ret = STATUS_DLL_NOT_FOUND; @@ -1763,7 +1783,7 @@ NTSTATUS WINAPI LdrGetProcedureAddress(HMODULE module, const ANSI_STRING *name, } } - RtlLeaveCriticalSection( &loader_section ); + unlock_loader(); return ret; } @@ -2882,7 +2902,7 @@ done: * load_dll (internal) * * Load a PE style module according to the load order. - * The loader_section must be locked while calling this function. + * The loader must be locked while calling this function. */ static NTSTATUS load_dll( const WCHAR *load_path, const WCHAR *libname, const WCHAR *default_ext, DWORD flags, WINE_MODREF** pwm ) @@ -2958,12 +2978,12 @@ NTSTATUS __cdecl __wine_init_unix_lib( HMODULE module, DWORD reason, const void WINE_MODREF *wm; NTSTATUS ret; - RtlEnterCriticalSection( &loader_section ); + lock_loader_exclusive(); if ((wm = get_modref( module ))) ret = unix_funcs->init_unix_lib( module, reason, ptr_in, ptr_out ); else ret = STATUS_INVALID_HANDLE; - RtlLeaveCriticalSection( &loader_section ); + unlock_loader(); return ret; } @@ -2988,7 +3008,7 @@ NTSTATUS WINAPI DECLSPEC_HOTPATCH LdrLoadDll(LPCWSTR path_name, DWORD flags, WINE_MODREF *wm; NTSTATUS nts; - RtlEnterCriticalSection( &loader_section ); + lock_loader_exclusive(); nts = load_dll( path_name, libname->Buffer, L".dll", flags, &wm ); @@ -3003,7 +3023,7 @@ NTSTATUS WINAPI DECLSPEC_HOTPATCH LdrLoadDll(LPCWSTR path_name, DWORD flags, } *hModule = (wm) ? wm->ldr.DllBase : NULL; - RtlLeaveCriticalSection( &loader_section ); + unlock_loader(); return nts; } @@ -3020,7 +3040,7 @@ NTSTATUS WINAPI LdrGetDllFullName( HMODULE module, UNICODE_STRING *name ) if (!module) module = NtCurrentTeb()->Peb->ImageBaseAddress; - RtlEnterCriticalSection( &loader_section ); + lock_loader_exclusive(); wm = get_modref( module ); if (wm) { @@ -3028,7 +3048,7 @@ NTSTATUS WINAPI LdrGetDllFullName( HMODULE module, UNICODE_STRING *name ) if (name->MaximumLength < wm->ldr.FullDllName.Length + sizeof(WCHAR)) status = STATUS_BUFFER_TOO_SMALL; else status = STATUS_SUCCESS; } else status = STATUS_DLL_NOT_FOUND; - RtlLeaveCriticalSection( &loader_section ); + unlock_loader(); return status; } @@ -3063,7 +3083,7 @@ NTSTATUS WINAPI LdrGetDllHandleEx( ULONG flags, LPCWSTR load_path, ULONG *dll_ch if (flags & ~supported_flags) FIXME( "Unsupported flags %#x.\n", flags ); if (dll_characteristics) FIXME( "dll_characteristics unsupported.\n" ); - RtlEnterCriticalSection( &loader_section ); + lock_loader_exclusive(); status = find_dll_file( load_path, name->Buffer, L".dll", &nt_name, &wm, &mapping, &image_info, &id ); @@ -3083,7 +3103,7 @@ NTSTATUS WINAPI LdrGetDllHandleEx( ULONG flags, LPCWSTR load_path, ULONG *dll_ch LdrAddRefDll( 0, *base ); } - RtlLeaveCriticalSection( &loader_section ); + unlock_loader(); TRACE( "%s -> %p (load path %s)\n", debugstr_us(name), status ? NULL : *base, debugstr_w(load_path) ); return status; } @@ -3108,7 +3128,7 @@ NTSTATUS WINAPI LdrAddRefDll( ULONG flags, HMODULE module ) if (flags & ~LDR_ADDREF_DLL_PIN) FIXME( "%p flags %x not implemented\n", module, flags ); - RtlEnterCriticalSection( &loader_section ); + lock_loader_exclusive(); if ((wm = get_modref( module ))) { @@ -3120,7 +3140,7 @@ NTSTATUS WINAPI LdrAddRefDll( ULONG flags, HMODULE module ) } else ret = STATUS_INVALID_PARAMETER; - RtlLeaveCriticalSection( &loader_section ); + unlock_loader(); return ret; } @@ -3206,7 +3226,7 @@ NTSTATUS WINAPI LdrQueryProcessModuleInformation(RTL_PROCESS_MODULES *smi, smi->ModulesCount = 0; - RtlEnterCriticalSection( &loader_section ); + lock_loader_exclusive(); mark = &NtCurrentTeb()->Peb->LdrData->InLoadOrderModuleList; for (entry = mark->Flink; entry != mark; entry = entry->Flink) { @@ -3234,7 +3254,7 @@ NTSTATUS WINAPI LdrQueryProcessModuleInformation(RTL_PROCESS_MODULES *smi, } else nts = STATUS_INFO_LENGTH_MISMATCH; } - RtlLeaveCriticalSection( &loader_section ); + unlock_loader(); if (req_size) *req_size = size; @@ -3452,7 +3472,7 @@ void WINAPI LdrShutdownProcess(void) */ void WINAPI RtlExitUserProcess( DWORD status ) { - RtlEnterCriticalSection( &loader_section ); + lock_loader_exclusive(); RtlAcquirePebLock(); NtTerminateProcess( 0, status ); LdrShutdownProcess(); @@ -3478,7 +3498,7 @@ void WINAPI LdrShutdownThread(void) RtlProcessFlsData( NtCurrentTeb()->FlsSlots, 1 ); - RtlEnterCriticalSection( &loader_section ); + lock_loader_exclusive(); wm = get_modref( NtCurrentTeb()->Peb->ImageBaseAddress ); mark = &NtCurrentTeb()->Peb->LdrData->InInitializationOrderModuleList; @@ -3510,7 +3530,7 @@ void WINAPI LdrShutdownThread(void) NtCurrentTeb()->TlsExpansionSlots = NULL; RtlReleasePebLock(); - RtlLeaveCriticalSection( &loader_section ); + unlock_loader(); /* don't call DbgUiGetThreadDebugObject as some apps hook it and terminate if called */ if (NtCurrentTeb()->DbgSsReserved[1]) NtClose( NtCurrentTeb()->DbgSsReserved[1] ); RtlFreeThreadActivationContextStack(); @@ -3551,7 +3571,7 @@ static void free_modref( WINE_MODREF *wm ) * Remove all unused modrefs and call the internal unloading routines * for the library type. * - * The loader_section must be locked while calling this function. + * The loader must be locked while calling this function. */ static void MODULE_FlushModrefs(void) { @@ -3582,7 +3602,7 @@ static void MODULE_FlushModrefs(void) /*********************************************************************** * MODULE_DecRefCount * - * The loader_section must be locked while calling this function. + * The loader must be locked while calling this function. */ static void MODULE_DecRefCount( WINE_MODREF *wm ) { @@ -3625,7 +3645,7 @@ NTSTATUS WINAPI LdrUnloadDll( HMODULE hModule ) TRACE("(%p)\n", hModule); - RtlEnterCriticalSection( &loader_section ); + lock_loader_exclusive(); free_lib_count++; if ((wm = get_modref( hModule )) != NULL) @@ -3649,7 +3669,7 @@ NTSTATUS WINAPI LdrUnloadDll( HMODULE hModule ) free_lib_count--; - RtlLeaveCriticalSection( &loader_section ); + unlock_loader(); return retv; } @@ -3762,7 +3782,7 @@ static void init_wow64( CONTEXT *context ) imports_fixup_done = TRUE; } - RtlLeaveCriticalSection( &loader_section ); + unlock_loader(); pWow64LdrpInitialize( context ); } @@ -3866,7 +3886,7 @@ void WINAPI LdrInitializeThunk( CONTEXT *context, ULONG_PTR unknown2, ULONG_PTR if (process_detaching) NtTerminateThread( GetCurrentThread(), 0 ); - RtlEnterCriticalSection( &loader_section ); + lock_loader_exclusive(); if (!imports_fixup_done) { @@ -3979,7 +3999,7 @@ void WINAPI LdrInitializeThunk( CONTEXT *context, ULONG_PTR unknown2, ULONG_PTR if (wm->ldr.TlsIndex != -1) call_tls_callbacks( wm->ldr.DllBase, DLL_THREAD_ATTACH ); } - RtlLeaveCriticalSection( &loader_section ); + unlock_loader(); signal_start_thread( context ); } @@ -4069,9 +4089,9 @@ PVOID WINAPI RtlPcToFileHeader( PVOID pc, PVOID *address ) LDR_DATA_TABLE_ENTRY *module; PVOID ret = NULL; - RtlEnterCriticalSection( &loader_section ); + lock_loader_exclusive(); if (!LdrFindEntryForAddress( pc, &module )) ret = module->DllBase; - RtlLeaveCriticalSection( &loader_section ); + unlock_loader(); *address = ret; return ret; } -- 2.31.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 776a88e0050..1bdc82478b1 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -115,6 +115,15 @@ struct ldr_notification static struct list ldr_notifications = LIST_INIT( ldr_notifications ); +static CRITICAL_SECTION ldr_notifications_section; +static CRITICAL_SECTION_DEBUG ldr_notifications_critsect_debug = +{ + 0, 0, &ldr_notifications_section, + { &ldr_notifications_critsect_debug.ProcessLocksList, &ldr_notifications_critsect_debug.ProcessLocksList }, + 0, 0, { (DWORD_PTR)(__FILE__ ": dlldir_section") } +}; +static CRITICAL_SECTION ldr_notifications_section = { &ldr_notifications_critsect_debug, -1, 0, 0, 0, 0 }; + static const char * const reason_names[] = { "PROCESS_DETACH", @@ -485,6 +494,7 @@ static void call_ldr_notifications( ULONG reason, LDR_DATA_TABLE_ENTRY *module ) data.Loaded.DllBase = module->DllBase; data.Loaded.SizeOfImage = module->SizeOfImage; + RtlEnterCriticalSection( &ldr_notifications_section ); LIST_FOR_EACH_ENTRY_SAFE( notify, notify_next, &ldr_notifications, struct ldr_notification, entry ) { TRACE_(relay)("\1Call LDR notification callback (proc=%p,reason=%u,data=%p,context=%p)\n", @@ -495,6 +505,7 @@ static void call_ldr_notifications( ULONG reason, LDR_DATA_TABLE_ENTRY *module ) TRACE_(relay)("\1Ret LDR notification callback (proc=%p,reason=%u,data=%p,context=%p)\n", notify->callback, reason, &data, notify->context ); } + RtlLeaveCriticalSection( &ldr_notifications_section ); } /************************************************************************* @@ -1681,9 +1692,9 @@ NTSTATUS WINAPI LdrRegisterDllNotification(ULONG flags, PLDR_DLL_NOTIFICATION_FU notify->callback = callback; notify->context = context; - lock_loader_exclusive(); + RtlEnterCriticalSection( &ldr_notifications_section ); list_add_tail( &ldr_notifications, ¬ify->entry ); - unlock_loader(); + RtlLeaveCriticalSection( &ldr_notifications_section ); *cookie = notify; return STATUS_SUCCESS; @@ -1700,9 +1711,9 @@ NTSTATUS WINAPI LdrUnregisterDllNotification( void *cookie ) if (!notify) return STATUS_INVALID_PARAMETER; - lock_loader_exclusive(); + RtlEnterCriticalSection( &ldr_notifications_section ); list_remove( ¬ify->entry ); - unlock_loader(); + RtlLeaveCriticalSection( &ldr_notifications_section ); RtlFreeHeap( GetProcessHeap(), 0, notify ); return STATUS_SUCCESS; -- 2.31.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- v2: - use static initializers for SRW lock. dlls/ntdll/loader.c | 58 ++++++++++++++++++++++++++++++++++++++++----- include/winternl.h | 2 +- 2 files changed, 53 insertions(+), 7 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 1bdc82478b1..9cadd32f1de 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -160,6 +160,8 @@ static RTL_CRITICAL_SECTION_DEBUG critsect_debug = }; static RTL_CRITICAL_SECTION loader_section = { &critsect_debug, -1, 0, 0, 0, 0 }; +static RTL_SRWLOCK loader_srw_lock = RTL_SRWLOCK_INIT; + static CRITICAL_SECTION dlldir_section; static CRITICAL_SECTION_DEBUG dlldir_critsect_debug = { @@ -217,24 +219,58 @@ static inline BOOL contains_path( LPCWSTR name ) return ((*name && (name[1] == ':')) || wcschr(name, '/') || wcschr(name, '\\')); } +/************************************************************************* + * inc_recursion_count + * + * Increment thread local internal loader lock recursion count and return the old value. + */ +static ULONG inc_recursion_count(void) +{ + return NtCurrentTeb()->Spare2++; +} + +/************************************************************************* + * dec_recursion_count + * + * Decrement thread local internal loader lock recursion count and return the new value. + */ +static ULONG dec_recursion_count(void) +{ + return --NtCurrentTeb()->Spare2; +} + /************************************************************************* * lock_loader_exclusive * - * Take exclusive loader lock. + * Take exclusive ownership of internal loader lock. + * Recursive locking is allowed. */ static void lock_loader_exclusive(void) { - RtlEnterCriticalSection( &loader_section ); + ULONG recursion_count = inc_recursion_count(); + + TRACE( "recursion_count %u.\n", recursion_count ); + if (!recursion_count && !RtlDllShutdownInProgress()) + RtlAcquireSRWLockExclusive( &loader_srw_lock ); } /************************************************************************* * unlock_loader * - * Release loader lock. + * Release internal loader lock. */ static void unlock_loader(void) { - RtlLeaveCriticalSection( &loader_section ); + ULONG recursion_count = dec_recursion_count(); + + TRACE( "recursion_count %u.\n", recursion_count ); + + if (RtlDllShutdownInProgress()) return; + + assert( recursion_count != ~0u ); + + if (!recursion_count) + RtlReleaseSRWLockExclusive( &loader_srw_lock ); } #define RTL_UNLOAD_EVENT_TRACE_NUMBER 64 @@ -494,6 +530,7 @@ static void call_ldr_notifications( ULONG reason, LDR_DATA_TABLE_ENTRY *module ) data.Loaded.DllBase = module->DllBase; data.Loaded.SizeOfImage = module->SizeOfImage; + RtlEnterCriticalSection( &loader_section ); RtlEnterCriticalSection( &ldr_notifications_section ); LIST_FOR_EACH_ENTRY_SAFE( notify, notify_next, &ldr_notifications, struct ldr_notification, entry ) { @@ -506,6 +543,7 @@ static void call_ldr_notifications( ULONG reason, LDR_DATA_TABLE_ENTRY *module ) notify->callback, reason, &data, notify->context ); } RtlLeaveCriticalSection( &ldr_notifications_section ); + RtlLeaveCriticalSection( &loader_section ); } /************************************************************************* @@ -1355,6 +1393,8 @@ static void call_tls_callbacks( HMODULE module, UINT reason ) dir = RtlImageDirectoryEntryToData( module, TRUE, IMAGE_DIRECTORY_ENTRY_TLS, &dirsize ); if (!dir || !dir->AddressOfCallBacks) return; + RtlEnterCriticalSection( &loader_section ); + for (callback = (const PIMAGE_TLS_CALLBACK *)dir->AddressOfCallBacks; *callback; callback++) { TRACE_(relay)("\1Call TLS callback (proc=%p,module=%p,reason=%s,reserved=0)\n", @@ -1373,6 +1413,8 @@ static void call_tls_callbacks( HMODULE module, UINT reason ) TRACE_(relay)("\1Ret TLS callback (proc=%p,module=%p,reason=%s,reserved=0)\n", *callback, module, reason_names[reason] ); } + + RtlLeaveCriticalSection( &loader_section ); } /************************************************************************* @@ -1405,6 +1447,8 @@ static NTSTATUS MODULE_InitDLL( WINE_MODREF *wm, UINT reason, LPVOID lpReserved else TRACE("(%p %s,%s,%p) - CALL\n", module, debugstr_w(wm->ldr.BaseDllName.Buffer), reason_names[reason], lpReserved ); + RtlEnterCriticalSection( &loader_section ); + __TRY { retv = call_dll_entry_point( entry, module, reason, lpReserved ); @@ -1419,6 +1463,8 @@ static NTSTATUS MODULE_InitDLL( WINE_MODREF *wm, UINT reason, LPVOID lpReserved } __ENDTRY + RtlLeaveCriticalSection( &loader_section ); + /* The state of the module list may have changed due to the call to the dll. We cannot assume that this module has not been deleted. */ @@ -1658,7 +1704,7 @@ NTSTATUS WINAPI LdrEnumerateLoadedModules( void *unknown, LDRENUMPROC callback, return STATUS_INVALID_PARAMETER; lock_loader_exclusive(); - + RtlEnterCriticalSection( &loader_section ); mark = &NtCurrentTeb()->Peb->LdrData->InMemoryOrderModuleList; for (entry = mark->Flink; entry != mark; entry = entry->Flink) { @@ -1666,7 +1712,7 @@ NTSTATUS WINAPI LdrEnumerateLoadedModules( void *unknown, LDRENUMPROC callback, callback( mod, context, &stop ); if (stop) break; } - + RtlLeaveCriticalSection( &loader_section ); unlock_loader(); return STATUS_SUCCESS; } diff --git a/include/winternl.h b/include/winternl.h index b6f93c116d8..521454396b4 100644 --- a/include/winternl.h +++ b/include/winternl.h @@ -466,7 +466,7 @@ typedef struct _TEB PVOID Instrumentation[16]; /* f2c/16b8 */ PVOID WinSockData; /* f6c/1738 */ ULONG GdiBatchCount; /* f70/1740 */ - ULONG Spare2; /* f74/1744 */ + ULONG Spare2; /* f74/1744 used for ntdll loader data in Wine */ ULONG GuaranteedStackBytes; /* f78/1748 */ PVOID ReservedForPerf; /* f7c/1750 */ PVOID ReservedForOle; /* f80/1758 */ -- 2.31.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=99244 Your paranoid android. === debiant2 (32 bit WoW report) === ntdll: change.c:277: Test failed: should be ready
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 9cadd32f1de..84b3592abb8 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -161,6 +161,7 @@ static RTL_CRITICAL_SECTION_DEBUG critsect_debug = static RTL_CRITICAL_SECTION loader_section = { &critsect_debug, -1, 0, 0, 0, 0 }; static RTL_SRWLOCK loader_srw_lock = RTL_SRWLOCK_INIT; +static volatile BOOL locked_exclusive; static CRITICAL_SECTION dlldir_section; static CRITICAL_SECTION_DEBUG dlldir_critsect_debug = @@ -250,8 +251,32 @@ static void lock_loader_exclusive(void) ULONG recursion_count = inc_recursion_count(); TRACE( "recursion_count %u.\n", recursion_count ); + if (!recursion_count) + { + if (!RtlDllShutdownInProgress()) + RtlAcquireSRWLockExclusive( &loader_srw_lock ); + locked_exclusive = TRUE; + } + else + { + assert( locked_exclusive ); + } +} + +/************************************************************************* + * lock_loader_shared + * + * Take shared ownership of internal loader lock. + * If the thread already has exclusive lock it will stay exclusive. + */ +static void lock_loader_shared(void) +{ + ULONG recursion_count = inc_recursion_count(); + + TRACE("recursion_count %u, locked_exclusive %d.\n", recursion_count, locked_exclusive); + if (!recursion_count && !RtlDllShutdownInProgress()) - RtlAcquireSRWLockExclusive( &loader_srw_lock ); + RtlAcquireSRWLockShared( &loader_srw_lock ); } /************************************************************************* @@ -269,8 +294,14 @@ static void unlock_loader(void) assert( recursion_count != ~0u ); - if (!recursion_count) + if (recursion_count) return; + + if (locked_exclusive) + { + locked_exclusive = FALSE; RtlReleaseSRWLockExclusive( &loader_srw_lock ); + } + else RtlReleaseSRWLockShared( &loader_srw_lock ); } #define RTL_UNLOAD_EVENT_TRACE_NUMBER 64 @@ -3283,7 +3314,7 @@ NTSTATUS WINAPI LdrQueryProcessModuleInformation(RTL_PROCESS_MODULES *smi, smi->ModulesCount = 0; - lock_loader_exclusive(); + lock_loader_shared(); mark = &NtCurrentTeb()->Peb->LdrData->InLoadOrderModuleList; for (entry = mark->Flink; entry != mark; entry = entry->Flink) { -- 2.31.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 84b3592abb8..1deae752fc9 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -4177,9 +4177,7 @@ PVOID WINAPI RtlPcToFileHeader( PVOID pc, PVOID *address ) LDR_DATA_TABLE_ENTRY *module; PVOID ret = NULL; - lock_loader_exclusive(); if (!LdrFindEntryForAddress( pc, &module )) ret = module->DllBase; - unlock_loader(); *address = ret; return ret; } -- 2.31.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=99246 Your paranoid android. === debiant2 (32 bit Chinese:China report) === ntdll: virtual: Timeout
participants (2)
-
Marvin -
Paul Gofman