[PATCH v2 00/22] ntdll: Relax the loader locking to match newer Windows behaviour.
Changes from the previous version accross the patches: - use ldr_data_srw_lock instead of introducing a new one when modifying WINE_MODREF data with a shared loader lock. - protect cached_modref with ldr_data_srw_lock instead of introducing interlocked access; - remove redundant 'volatile' qualifier from locked_exclusive; - fix and improve tests, split them across the patchset. I actually mean to send first 7 patches from this series for review and not 22 at once but it feels like the changes introduced by these first patches make sense in this exact form only if the general idea of locking rework is accepted. So I am sending the whole thing in its current state in a hope it would be possible to see what I am suggesting and make a decision on the general idea at least. I did some cleanup on all the functional patches, although would look once again at those below patch 7 at least before sending that for review. The test in the last patch obviously needs more work, it works well for me on the recent enough Windows 10 but not that well yet on earlier versions, also needs cleaning up and splitting. Some facts about the current Windows loader behaviour: - It seems that loader lock (the one that can be managed externally through LdrLockLoaderLock) is held only when calling the app's functions from the loader: DLL entry point, TLS callbacks, LDR notifications and LdrEnumerateLoadedModules callback. There is a quirk (at least on the newer Win10) with LDR notifications: the loader lock is held during the unload event but not during the load event, at least the first one. I am guessing that maybe it is unheld only until the first dependency DLL entry point is called (so they take it once there and then keep until process attach ends, leaving first load notification behind), but I didn't test that with the DLL with dependencies yet. In any case it looks to me this quirk may be left for later. - There is apparently other lock mechanics involved as Windows does not allow loading process of different DLLs to go in parallel; - It looks like any function which does not end up requiring actual loading or unloading of the DLL and does not query anything from the module currently being loaded or unloaded can be executed in parallel with the current load process. E. g., LoadLibrary("b.dll") won't ever block if the DLL is already loaded, LdrAddRefDll never blocks at all even if called for the module currently being loaded (hanging in dll entry point process attach) or unloaded (hanging in detach), LdrUnloadDll will only block if it decrements load count to zero. - The behaviour of the functions operating on the module being currently loaded or unloaded looks quirky. Here is what I recovered from testing on the 19043.1237 (that is mostly covered in the test in the last patch): - LdrUnloadDll blocks until the DLL load sequence is complete and succeeds without waiting if unload sequence is in progress; - LdrAddRefDll never blocks but behaviour depends on whether it is called from the app callback (tested with entry point and LDR notification) or outside of that (patch 0014). - LdrGetDllHandleEx blocks during module unload but not load; - LdrGetProcedureAddress waits until the module is being loaded and immediately returns an error if module is being unloaded; - Looks like calling functions from LDR notifications is the special case. Those that wouldn't block work, but those that would block if called from outside return error STATUS_NOT_FOUND. That is apparently not how that behaves from DLL entry points and TLS callbacks. I think the main patch in the series is patch 0008 which introduces exclusive locks downgrade / upgrade functions. The idea is that we can take shared or exclusive lock from loader function. The exclusive lock can be downgraded to shared. However, the thread still holds exlusiveness in this state in a sense that other exclusive lock cannot be taken until the initial exclusive lock is released. Shared lock waiters can joing when the exlusive lock is downgraded. This works recursively so loader functions can call each other and call outside callbacks with the lock held, with the exception that exclusive lock cannot be taken recursively if shared is currently held. So, the way of using it is: - Never call the callbacks with read lock held: that will assert once the application calls some loader function which takes the exclusive lock. - The way to call the callbacks is to have an exclusive lock and downgrade it to shared, then the recursive taking of exclusive lock is ok; - If it is unclear at function start whether exclusive lock is needed or not, the way to go is to start from shared lock, try to perform the operation (in case of LdrAddRefDll / LdrUnloadLibrary I used an extra short lived SRW lock), and if that is not possible release the shared lock and restart from scratch with exclusive. I think maybe it would be technically possible not to introduce the limitation and allow upgrading shared locks to exclusive but it seems it would make things much more complicated than they are: it would require some neater and finer grained structure access sync to avoid invalidating all the data the shared locker depends on at the moment it wants to upgrade to exclusive and yields to exclusive library load / unload sequence. Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> -- 2.31.1
Fixes racy access to LDR lists in lookup_function_info(). Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- 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 797d72aedb6..e363c060ab1 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 lists are modified with loader locked and exclusive ldr_data_srw_lock held. + * Taking shared ldr_data_srw_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 e363c060ab1..f7b96b45b6f 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 f7b96b45b6f..5ee4215f875 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__ ": ldr_notifications_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> --- dlls/kernel32/tests/loader.c | 177 +++++++++++++++++++++++++++++++++++ dlls/ntdll/loader.c | 58 ++++++++++-- include/winternl.h | 2 +- 3 files changed, 230 insertions(+), 7 deletions(-) diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 4f1b11338a6..045338c1289 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -70,6 +70,10 @@ static NTSTATUS (WINAPI *pLdrLockLoaderLock)(ULONG, ULONG *, ULONG_PTR *); static NTSTATUS (WINAPI *pLdrUnlockLoaderLock)(ULONG, ULONG_PTR); static NTSTATUS (WINAPI *pLdrLoadDll)(LPCWSTR,DWORD,const UNICODE_STRING *,HMODULE*); static NTSTATUS (WINAPI *pLdrUnloadDll)(HMODULE); + +typedef void (CALLBACK *LDRENUMPROC)(LDR_DATA_TABLE_ENTRY *, void *, BOOLEAN *); +static NTSTATUS (WINAPI *pLdrEnumerateLoadedModules)( void *unknown, LDRENUMPROC callback, void *context ); + static void (WINAPI *pRtlInitUnicodeString)(PUNICODE_STRING,LPCWSTR); static void (WINAPI *pRtlAcquirePebLock)(void); static void (WINAPI *pRtlReleasePebLock)(void); @@ -4036,6 +4040,177 @@ static void test_Wow64Transition(void) debugstr_wn(name->SectionFileName.Buffer, name->SectionFileName.Length / sizeof(WCHAR))); } +static BOOL test_loader_lock_repeat_lock; +static unsigned int test_loader_notification_count; +static const WCHAR *ldr_notify_track_dll; +static HANDLE lock_ready_event, next_test_event; +static BOOL test_loader_lock_abort_test; +static volatile LONG test_loader_lock_timeout_count; + +#define BLOCKING_TESTS_ENABLED 0 + +static DWORD WINAPI test_loader_lock_thread(void *param) +{ + ULONG_PTR magic; + NTSTATUS status; + DWORD result; + + WaitForSingleObject(next_test_event, INFINITE); + SetEvent(lock_ready_event); + WaitForSingleObject(next_test_event, INFINITE); + + /* 1. Test with loader lock held. */ + do + { + status = pLdrLockLoaderLock(0, NULL, &magic); + ok(!status, "Got unexpected status %#x.\n", status); + SetEvent(lock_ready_event); + + result = WaitForSingleObject(next_test_event, 3000); + if (result == WAIT_TIMEOUT) + ++test_loader_lock_timeout_count; + + status = pLdrUnlockLoaderLock(0, magic); + ok(!status, "Got unexpected status %#x.\n", status); + + if (result == WAIT_TIMEOUT) + { + WaitForSingleObject(next_test_event, INFINITE); + test_loader_lock_timeout_count = 0; + if (test_loader_lock_abort_test) + return 0; + } + } while (test_loader_lock_repeat_lock); + + SetEvent(lock_ready_event); + WaitForSingleObject(next_test_event, INFINITE); + + SetEvent(lock_ready_event); + return 0; +} + +static void CALLBACK test_loader_lock_ldr_notify(ULONG reason, LDR_DLL_NOTIFICATION_DATA *data, void *context) +{ + if (!test_loader_lock_timeout_count && !lstrcmpW(data->Loaded.BaseDllName->Buffer, ldr_notify_track_dll)) + ++test_loader_notification_count; +} + +static void CALLBACK test_loader_lock_enum_callback(LDR_DATA_TABLE_ENTRY *mod, void *context, BOOLEAN *stop) +{ + *stop = TRUE; +} + +#define check_timeout(expected_timeout) check_timeout_(__LINE__, expected_timeout) +static void check_timeout_(unsigned int line, BOOL expected_timeout) +{ + ok_(__FILE__, line)(!!test_loader_lock_timeout_count == !!expected_timeout, + "Got timeout count %u, expected_timeout %d.\n", + test_loader_lock_timeout_count, expected_timeout); + if (test_loader_lock_timeout_count) + { + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + } +} + +static void test_loader_lock_next_test(void) +{ + /* Exit previous test loop. */ + test_loader_lock_repeat_lock = FALSE; + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + test_loader_lock_repeat_lock = TRUE; + + /* Star new loop. */ + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); +} + +static void test_loader_lock(void) +{ + static const WCHAR not_loaded_dll_name[] = L"authz.dll"; + static const WCHAR preloaded_dll_name[] = L"winmm.dll"; + HMODULE hmodule_preloaded, hmodule; + NTSTATUS status; + HANDLE thread; + void *cookie; + BOOL bret; + + lock_ready_event = CreateEventA(NULL, FALSE, FALSE, "test_lock_ready_event"); + next_test_event = CreateEventA(NULL, FALSE, FALSE, "test_next_test_event"); + + thread = CreateThread(NULL, 0, test_loader_lock_thread, NULL, 0, NULL); + + hmodule_preloaded = LoadLibraryW(preloaded_dll_name); + ok(!!hmodule_preloaded, "LoadLibrary failed, err %u.\n", GetLastError()); + hmodule = GetModuleHandleW(not_loaded_dll_name); + ok(!hmodule, "%s is already loaded.\n", not_loaded_dll_name); + + test_loader_notification_count = 0; + + /* 1. Test with loader lock held. */ + trace("Test 1.\n"); + test_loader_lock_next_test(); + + status = LdrRegisterDllNotification(0, test_loader_lock_ldr_notify, NULL, &cookie); + ok(!status, "Got unexpected status %#x.\n", status); + if (test_loader_lock_timeout_count) + { + test_loader_lock_abort_test = TRUE; + SetEvent(next_test_event); + win_skip("Old loader, tests skipped.\n"); + goto done; + } + check_timeout(FALSE); + ldr_notify_track_dll = not_loaded_dll_name; + + bret = GetModuleHandleExW(0, preloaded_dll_name, &hmodule); + ok(bret, "GetModuleHandleEx failed, err %u.\n", GetLastError()); + ok(hmodule == hmodule_preloaded, "Got unexpected hmodule %p, expected %p.\n", hmodule, hmodule_preloaded); + check_timeout(FALSE); + + bret = FreeLibrary(hmodule); + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + check_timeout(FALSE); + + ok(!test_loader_notification_count, "Got unexpected test_loader_notification_count %u.\n", + test_loader_notification_count); + + if (BLOCKING_TESTS_ENABLED) + { + /* With loader lock held notification callback is called which should mean that: + * - LDR notifications themselves do not wait on loader lock; + * - The library load goes far enough to call the LDR notification until it blocks on the loader lock. + */ + test_loader_notification_count = 0; + hmodule = LoadLibraryW(not_loaded_dll_name); + todo_wine ok(test_loader_notification_count == 1 + || broken(!test_loader_notification_count) /* before Win10 1607. */, + "Got unexpected notification count %u.\n", test_loader_notification_count); + check_timeout(TRUE); + + bret = FreeLibrary(hmodule); + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + check_timeout(TRUE); + + pLdrEnumerateLoadedModules(NULL, test_loader_lock_enum_callback, NULL); + check_timeout(TRUE); + } + + LdrUnregisterDllNotification( cookie ); + check_timeout(FALSE); + + test_loader_lock_next_test(); + +done: + WaitForSingleObject(thread, INFINITE); + CloseHandle(thread); + + FreeLibrary(hmodule_preloaded); + CloseHandle(lock_ready_event); + CloseHandle(next_test_event); +} + START_TEST(loader) { int argc; @@ -4060,6 +4235,7 @@ START_TEST(loader) pLdrUnlockLoaderLock = (void *)GetProcAddress(ntdll, "LdrUnlockLoaderLock"); pLdrLoadDll = (void *)GetProcAddress(ntdll, "LdrLoadDll"); pLdrUnloadDll = (void *)GetProcAddress(ntdll, "LdrUnloadDll"); + pLdrEnumerateLoadedModules = (void *)GetProcAddress(ntdll, "LdrEnumerateLoadedModules"); pRtlInitUnicodeString = (void *)GetProcAddress(ntdll, "RtlInitUnicodeString"); pRtlAcquirePebLock = (void *)GetProcAddress(ntdll, "RtlAcquirePebLock"); pRtlReleasePebLock = (void *)GetProcAddress(ntdll, "RtlReleasePebLock"); @@ -4113,6 +4289,7 @@ START_TEST(loader) test_dll_file( "advapi32.dll" ); test_dll_file( "user32.dll" ); test_Wow64Transition(); + test_loader_lock(); /* loader test must be last, it can corrupt the internal loader state on Windows */ test_Loader(); } diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 5ee4215f875..d0d4e5448ed 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=99434 Your paranoid android. === w1064_tsign (32 bit report) === kernel32: loader.c:2818: Test failed: attached thread count should be 2
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- - remove redundant 'volatile' qualifier from locked_exclusive. dlls/kernel32/tests/loader.c | 46 ++++++++++++++++++++++++++++++++++++ dlls/ntdll/loader.c | 37 ++++++++++++++++++++++++++--- 2 files changed, 80 insertions(+), 3 deletions(-) diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 045338c1289..e97c114c5e9 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -4049,6 +4049,16 @@ static volatile LONG test_loader_lock_timeout_count; #define BLOCKING_TESTS_ENABLED 0 +static void CALLBACK test_loader_lock_module_enum_locking_callback(LDR_DATA_TABLE_ENTRY *mod, + void *context, BOOLEAN *stop) +{ + SetEvent(lock_ready_event); + if (WaitForSingleObject(next_test_event, 3000) == WAIT_TIMEOUT) + ++test_loader_lock_timeout_count; + + *stop = TRUE; +} + static DWORD WINAPI test_loader_lock_thread(void *param) { ULONG_PTR magic; @@ -4085,6 +4095,20 @@ static DWORD WINAPI test_loader_lock_thread(void *param) SetEvent(lock_ready_event); WaitForSingleObject(next_test_event, INFINITE); + /* 2. Test with the thread blocked in LdrEnumerateLoadedModules callback. */ + do + { + pLdrEnumerateLoadedModules(NULL, test_loader_lock_module_enum_locking_callback, NULL); + if (test_loader_lock_timeout_count) + { + WaitForSingleObject(next_test_event, INFINITE); + test_loader_lock_timeout_count = 0; + } + } while (test_loader_lock_repeat_lock); + + SetEvent(lock_ready_event); + WaitForSingleObject(next_test_event, INFINITE); + SetEvent(lock_ready_event); return 0; } @@ -4131,6 +4155,7 @@ static void test_loader_lock(void) static const WCHAR not_loaded_dll_name[] = L"authz.dll"; static const WCHAR preloaded_dll_name[] = L"winmm.dll"; HMODULE hmodule_preloaded, hmodule; + ULONG_PTR magic; NTSTATUS status; HANDLE thread; void *cookie; @@ -4200,6 +4225,27 @@ static void test_loader_lock(void) LdrUnregisterDllNotification( cookie ); check_timeout(FALSE); + /* 2. Test with the thread blocked in LdrEnumerateLoadedModules callback. */ + trace("Test 2.\n"); + test_loader_lock_next_test(); + + status = LdrRegisterDllNotification(0, test_loader_lock_ldr_notify, NULL, &cookie); + ok(!status, "Got unexpected status %#x.\n", status); + check_timeout(FALSE); + + if (BLOCKING_TESTS_ENABLED) + { + status = pLdrLockLoaderLock(0, NULL, &magic); + ok(!status, "Got unexpected status %#x.\n", status); + status = pLdrUnlockLoaderLock(0, magic); + ok(!status, "Got unexpected status %#x.\n", status); + + check_timeout(TRUE); + } + + LdrUnregisterDllNotification( cookie ); + check_timeout(FALSE); + test_loader_lock_next_test(); done: diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index d0d4e5448ed..092805cc945 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 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
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=99435 Your paranoid android. === w1064_tsign (32 bit report) === kernel32: loader.c:2818: Test failed: attached thread count should be 2
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 092805cc945..547c61aa7cb 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
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 192 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 180 insertions(+), 12 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 547c61aa7cb..c4fdfdf414b 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -160,8 +160,18 @@ static RTL_CRITICAL_SECTION_DEBUG critsect_debug = }; static RTL_CRITICAL_SECTION loader_section = { &critsect_debug, -1, 0, 0, 0, 0 }; + +/* Thread which holds exclusive lock or shared lock downgraded from exclusive which can be upgraded back. r*/ +static volatile DWORD exclusive_lock_thread_id; + static RTL_SRWLOCK loader_srw_lock = RTL_SRWLOCK_INIT; static BOOL locked_exclusive; +static RTL_CONDITION_VARIABLE exclusive_lock_released_cv = RTL_CONDITION_VARIABLE_INIT; + +#define MAX_DOWNGRADE_RECURSION_COUNT 256 + +static unsigned int lock_exclusive_recursion_count[MAX_DOWNGRADE_RECURSION_COUNT]; +static unsigned int lock_exclusive_recursion_stack; static CRITICAL_SECTION dlldir_section; static CRITICAL_SECTION_DEBUG dlldir_critsect_debug = @@ -240,27 +250,99 @@ static ULONG dec_recursion_count(void) return --NtCurrentTeb()->Spare2; } +/************************************************************************* + * wait_for_exclusive_lock_release + * + * Should be called with shared loader lock. + * Waits until exclusive lock is fully released through unlock_loader(), not + * just downgraded. + * The shared loader lock is released during wait and then acquired back, so + * any prior shared data is invalidated. + */ +static void wait_for_exclusive_lock_release(void) +{ + LARGE_INTEGER timeout; + NTSTATUS status; + + TRACE( ".\n" ); + + if (RtlDllShutdownInProgress()) return; + + assert( !locked_exclusive ); + /* Some thread must be in the loading sequence otherwise we may wait forever. */ + assert( exclusive_lock_thread_id && exclusive_lock_thread_id != GetCurrentThreadId() ); + + timeout.QuadPart = -10000 * 5000; /* 5 sec. */ + + while (!RtlDllShutdownInProgress() + && ((status = RtlSleepConditionVariableSRW( &exclusive_lock_released_cv, &loader_srw_lock, + &timeout, CONDITION_VARIABLE_LOCKMODE_SHARED )) + || exclusive_lock_thread_id)) + { + if (status) + { + ERR( "Error waiting for load event complete, status %#x.\n", status ); + assert( status == STATUS_TIMEOUT ); + } + } +} + /************************************************************************* * lock_loader_exclusive * * Take exclusive ownership of internal loader lock. * Recursive locking is allowed. + * If the current lock is shared taking exclusive mode is only possible if the current + * mode was previously downgraded from exclusive, i. e., if the initial lock the thread has + * taken was exclusive. */ static void lock_loader_exclusive(void) { ULONG recursion_count = inc_recursion_count(); - TRACE( "recursion_count %u.\n", recursion_count ); + TRACE( "recursion_count %u, locked_exclusive %d, exclusive_lock_thread_id %04x.\n", + recursion_count, locked_exclusive, exclusive_lock_thread_id ); + + if (RtlDllShutdownInProgress()) return; + + /* locked_exclusive implies recursion_count but still check recursion_count first + * to avoid potentially racy 'locked_exclusive' access without holding loader_srw_lock. */ + if (recursion_count && locked_exclusive) + { + ++lock_exclusive_recursion_count[lock_exclusive_recursion_stack]; + return; + } + if (!recursion_count) { - if (!RtlDllShutdownInProgress()) - RtlAcquireSRWLockExclusive( &loader_srw_lock ); - locked_exclusive = TRUE; + /* We can't call RtlAcquireSRWLockExclusive() right away. If there is a thread + * which claimed exclusive access through exclusive_lock_thread_id and downgraded + * the lock to shared, waiting for exclusive lock will block any new shared locks + * waiters which should be allowed to pass in that state. + * So acquire a shared lock, claim the exclusive access and then upgrade the SRW lock + * to exclusive. */ + RtlAcquireSRWLockShared( &loader_srw_lock ); + + while (InterlockedCompareExchange( (LONG volatile *)&exclusive_lock_thread_id, GetCurrentThreadId(), 0)) + { + wait_for_exclusive_lock_release(); + if (RtlDllShutdownInProgress()) return; + } + assert( !lock_exclusive_recursion_stack ); + assert( !lock_exclusive_recursion_count[lock_exclusive_recursion_stack] ); } else { - assert( locked_exclusive ); + /* It is not allowed to upgrade the lock to exclusive, only + * reacquire previously downgraded exclusive lock. */ + assert( GetCurrentThreadId() == exclusive_lock_thread_id ); } + RtlReleaseSRWLockShared( &loader_srw_lock ); + if (RtlDllShutdownInProgress()) return; + RtlAcquireSRWLockExclusive( &loader_srw_lock ); + locked_exclusive = TRUE; + + ++lock_exclusive_recursion_count[lock_exclusive_recursion_stack]; } /************************************************************************* @@ -273,35 +355,119 @@ static void lock_loader_shared(void) { ULONG recursion_count = inc_recursion_count(); - TRACE("recursion_count %u, locked_exclusive %d.\n", recursion_count, locked_exclusive); + TRACE( "recursion_count %u, locked_exclusive %d, exclusive_lock_thread_id %04x.\n", + recursion_count, locked_exclusive, exclusive_lock_thread_id ); - if (!recursion_count && !RtlDllShutdownInProgress()) + if (RtlDllShutdownInProgress()) return; + + if (!recursion_count) RtlAcquireSRWLockShared( &loader_srw_lock ); + + /* If we are in exclusive lock already the lock stays exclusive. + * The exclusive recursion count is needed to downgrade the exclusive + * lock back to shared once we out of the exclusive lock scope. + * The scopes are bounded by explicit exclusive locks downgrades + * which increment lock_exclusive_recursion_stack. + */ + if (locked_exclusive) + ++lock_exclusive_recursion_count[lock_exclusive_recursion_stack]; } /************************************************************************* * unlock_loader * * Release internal loader lock. + * The exclusive lock being released might have been recursively taken while the + * thread was holding shared lock (downgraded from exclusive). In this case unlock_loader() + * will denote the exclusive lock to shared. */ static void unlock_loader(void) { ULONG recursion_count = dec_recursion_count(); - TRACE( "recursion_count %u.\n", recursion_count ); + TRACE("recursion_count %u, locked_exclusive %d, lock_exclusive_recursion_stack %u.\n", + recursion_count, locked_exclusive, lock_exclusive_recursion_stack ); if (RtlDllShutdownInProgress()) return; assert( recursion_count != ~0u ); - if (recursion_count) return; + if (!locked_exclusive) + { + if (!recursion_count) RtlReleaseSRWLockShared( &loader_srw_lock ); + return; + } + + assert( exclusive_lock_thread_id == GetCurrentThreadId() ); + assert( lock_exclusive_recursion_count[lock_exclusive_recursion_stack] ); + assert( recursion_count || lock_exclusive_recursion_count[lock_exclusive_recursion_stack] == 1); - if (locked_exclusive) + if (--lock_exclusive_recursion_count[lock_exclusive_recursion_stack]) return; + + locked_exclusive = FALSE; + + if (!recursion_count) { - locked_exclusive = FALSE; + InterlockedExchange( (LONG volatile *)&exclusive_lock_thread_id, 0 ); RtlReleaseSRWLockExclusive( &loader_srw_lock ); + RtlWakeAllConditionVariable( &exclusive_lock_released_cv ); } - else RtlReleaseSRWLockShared( &loader_srw_lock ); + else + { + assert( lock_exclusive_recursion_stack ); + RtlReleaseSRWLockExclusive( &loader_srw_lock ); + if (RtlDllShutdownInProgress()) return; + RtlAcquireSRWLockShared( &loader_srw_lock ); + } +} + +/************************************************************************* + * lock_loader_downgrade_exclusive + * + * Denote exclusive internal loader lock ownership to shared. + * Denoted lock allows other threads to take shared lock but the threads + * waiting for exclusive lock will not overtake the lock until the + * initial (exclusive) lock taken by the current thread is released. + */ +static void lock_loader_downgrade_exclusive(void) +{ + TRACE( "locked_exclusive %d, exclusive thread %04x.\n", locked_exclusive, exclusive_lock_thread_id ); + + if (RtlDllShutdownInProgress()) return; + + assert( exclusive_lock_thread_id == GetCurrentThreadId() ); + assert( locked_exclusive ); + assert( lock_exclusive_recursion_stack < MAX_DOWNGRADE_RECURSION_COUNT ); + ++lock_exclusive_recursion_stack; + assert( !lock_exclusive_recursion_count[lock_exclusive_recursion_stack] ); + + locked_exclusive = FALSE; + RtlReleaseSRWLockExclusive( &loader_srw_lock ); + if (RtlDllShutdownInProgress()) return; + RtlAcquireSRWLockShared( &loader_srw_lock ); +} + +/************************************************************************* + * lock_loader_restore_exclusive + * + * Restore exclusive internal loader lock ownership which was previously denoted to shared. + */ +static void lock_loader_restore_exclusive(void) +{ + TRACE( "locked_exclusive %d, exclusive thread %04x.\n", locked_exclusive, exclusive_lock_thread_id ); + + if (RtlDllShutdownInProgress()) return; + + assert( !locked_exclusive ); + assert( GetCurrentThreadId() == exclusive_lock_thread_id ); + + assert( lock_exclusive_recursion_stack ); + --lock_exclusive_recursion_stack; + + RtlReleaseSRWLockShared( &loader_srw_lock ); + if (RtlDllShutdownInProgress()) return; + RtlAcquireSRWLockExclusive( &loader_srw_lock ); + locked_exclusive = TRUE; } #define RTL_UNLOAD_EVENT_TRACE_NUMBER 64 @@ -1478,6 +1644,7 @@ 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 ); + lock_loader_downgrade_exclusive(); RtlEnterCriticalSection( &loader_section ); __TRY @@ -1495,6 +1662,7 @@ static NTSTATUS MODULE_InitDLL( WINE_MODREF *wm, UINT reason, LPVOID lpReserved __ENDTRY RtlLeaveCriticalSection( &loader_section ); + lock_loader_restore_exclusive(); /* 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 -- 2.31.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index c4fdfdf414b..bac249bd3d9 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -1590,6 +1590,7 @@ static void call_tls_callbacks( HMODULE module, UINT reason ) dir = RtlImageDirectoryEntryToData( module, TRUE, IMAGE_DIRECTORY_ENTRY_TLS, &dirsize ); if (!dir || !dir->AddressOfCallBacks) return; + lock_loader_downgrade_exclusive(); RtlEnterCriticalSection( &loader_section ); for (callback = (const PIMAGE_TLS_CALLBACK *)dir->AddressOfCallBacks; *callback; callback++) @@ -1612,6 +1613,7 @@ static void call_tls_callbacks( HMODULE module, UINT reason ) } RtlLeaveCriticalSection( &loader_section ); + lock_loader_restore_exclusive(); } /************************************************************************* -- 2.31.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index bac249bd3d9..002710115cc 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -727,6 +727,7 @@ static void call_ldr_notifications( ULONG reason, LDR_DATA_TABLE_ENTRY *module ) data.Loaded.DllBase = module->DllBase; data.Loaded.SizeOfImage = module->SizeOfImage; + lock_loader_downgrade_exclusive(); RtlEnterCriticalSection( &loader_section ); RtlEnterCriticalSection( &ldr_notifications_section ); LIST_FOR_EACH_ENTRY_SAFE( notify, notify_next, &ldr_notifications, struct ldr_notification, entry ) @@ -741,6 +742,7 @@ static void call_ldr_notifications( ULONG reason, LDR_DATA_TABLE_ENTRY *module ) } RtlLeaveCriticalSection( &ldr_notifications_section ); RtlLeaveCriticalSection( &loader_section ); + lock_loader_restore_exclusive(); } /************************************************************************* -- 2.31.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 002710115cc..047e837238c 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -1907,6 +1907,7 @@ NTSTATUS WINAPI LdrEnumerateLoadedModules( void *unknown, LDRENUMPROC callback, return STATUS_INVALID_PARAMETER; lock_loader_exclusive(); + lock_loader_downgrade_exclusive(); RtlEnterCriticalSection( &loader_section ); mark = &NtCurrentTeb()->Peb->LdrData->InMemoryOrderModuleList; for (entry = mark->Flink; entry != mark; entry = entry->Flink) @@ -1916,6 +1917,7 @@ NTSTATUS WINAPI LdrEnumerateLoadedModules( void *unknown, LDRENUMPROC callback, if (stop) break; } RtlLeaveCriticalSection( &loader_section ); + lock_loader_restore_exclusive(); unlock_loader(); return STATUS_SUCCESS; } -- 2.31.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- v2: - protect cached_modref with ldr_data_srw_lock instead of introducing interlocked access. dlls/ntdll/loader.c | 65 +++++++++++++++++++++++++++++++-------------- 1 file changed, 45 insertions(+), 20 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 047e837238c..8e2ed03ad4c 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -206,7 +206,10 @@ static RTL_SRWLOCK ldr_data_srw_lock = RTL_SRWLOCK_INIT; static RTL_BITMAP tls_bitmap; static RTL_BITMAP tls_expansion_bitmap; +/* Guarded by ldr_data_srw_lock. */ static WINE_MODREF *cached_modref; + +/* Used with exclusive loader lock only. */ static WINE_MODREF *current_modref; static WINE_MODREF *last_failed_modref; @@ -470,6 +473,33 @@ static void lock_loader_restore_exclusive(void) locked_exclusive = TRUE; } +/************************************************************************* + * get_cached_modref + * + */ +static WINE_MODREF *get_cached_modref(void) +{ + WINE_MODREF *ret; + + RtlAcquireSRWLockShared( &ldr_data_srw_lock ); + ret = cached_modref; + RtlReleaseSRWLockShared( &ldr_data_srw_lock ); + return ret; +} + +/************************************************************************* + * set_cached_modref + * + * Returns the new cached modref. + */ +static WINE_MODREF *set_cached_modref( WINE_MODREF *new ) +{ + RtlAcquireSRWLockExclusive( &ldr_data_srw_lock ); + cached_modref = new; + RtlReleaseSRWLockExclusive( &ldr_data_srw_lock ); + return new; +} + #define RTL_UNLOAD_EVENT_TRACE_NUMBER 64 typedef struct _RTL_UNLOAD_EVENT_TRACE @@ -753,17 +783,18 @@ static void call_ldr_notifications( ULONG reason, LDR_DATA_TABLE_ENTRY *module ) */ static WINE_MODREF *get_modref( HMODULE hmod ) { + WINE_MODREF *cached = get_cached_modref(); PLIST_ENTRY mark, entry; PLDR_DATA_TABLE_ENTRY mod; - if (cached_modref && cached_modref->ldr.DllBase == hmod) return cached_modref; + if (cached && cached->ldr.DllBase == hmod) return cached; mark = &NtCurrentTeb()->Peb->LdrData->InMemoryOrderModuleList; for (entry = mark->Flink; entry != mark; entry = entry->Flink) { mod = CONTAINING_RECORD(entry, LDR_DATA_TABLE_ENTRY, InMemoryOrderLinks); if (mod->DllBase == hmod) - return cached_modref = CONTAINING_RECORD(mod, WINE_MODREF, ldr); + return set_cached_modref( CONTAINING_RECORD(mod, WINE_MODREF, ldr) ); } return NULL; } @@ -777,23 +808,21 @@ static WINE_MODREF *get_modref( HMODULE hmod ) */ static WINE_MODREF *find_basename_module( LPCWSTR name ) { + WINE_MODREF *cached = get_cached_modref(); PLIST_ENTRY mark, entry; UNICODE_STRING name_str; RtlInitUnicodeString( &name_str, name ); - if (cached_modref && RtlEqualUnicodeString( &name_str, &cached_modref->ldr.BaseDllName, TRUE )) - return cached_modref; + if (cached && RtlEqualUnicodeString( &name_str, &cached->ldr.BaseDllName, TRUE )) + return cached; mark = &NtCurrentTeb()->Peb->LdrData->InLoadOrderModuleList; for (entry = mark->Flink; entry != mark; entry = entry->Flink) { LDR_DATA_TABLE_ENTRY *mod = CONTAINING_RECORD(entry, LDR_DATA_TABLE_ENTRY, InLoadOrderLinks); if (RtlEqualUnicodeString( &name_str, &mod->BaseDllName, TRUE )) - { - cached_modref = CONTAINING_RECORD(mod, WINE_MODREF, ldr); - return cached_modref; - } + return set_cached_modref( CONTAINING_RECORD(mod, WINE_MODREF, ldr) ); } return NULL; } @@ -807,6 +836,7 @@ static WINE_MODREF *find_basename_module( LPCWSTR name ) */ static WINE_MODREF *find_fullname_module( const UNICODE_STRING *nt_name ) { + WINE_MODREF *cached = get_cached_modref(); PLIST_ENTRY mark, entry; UNICODE_STRING name = *nt_name; @@ -814,18 +844,15 @@ static WINE_MODREF *find_fullname_module( const UNICODE_STRING *nt_name ) name.Length -= 4 * sizeof(WCHAR); /* for \??\ prefix */ name.Buffer += 4; - if (cached_modref && RtlEqualUnicodeString( &name, &cached_modref->ldr.FullDllName, TRUE )) - return cached_modref; + if (cached && RtlEqualUnicodeString( &name, &cached->ldr.FullDllName, TRUE )) + return cached; mark = &NtCurrentTeb()->Peb->LdrData->InLoadOrderModuleList; for (entry = mark->Flink; entry != mark; entry = entry->Flink) { LDR_DATA_TABLE_ENTRY *mod = CONTAINING_RECORD(entry, LDR_DATA_TABLE_ENTRY, InLoadOrderLinks); if (RtlEqualUnicodeString( &name, &mod->FullDllName, TRUE )) - { - cached_modref = CONTAINING_RECORD(mod, WINE_MODREF, ldr); - return cached_modref; - } + return set_cached_modref( CONTAINING_RECORD(mod, WINE_MODREF, ldr) ); } return NULL; } @@ -839,9 +866,10 @@ static WINE_MODREF *find_fullname_module( const UNICODE_STRING *nt_name ) */ static WINE_MODREF *find_fileid_module( const struct file_id *id ) { + WINE_MODREF *cached = get_cached_modref(); LIST_ENTRY *mark, *entry; - if (cached_modref && !memcmp( &cached_modref->id, id, sizeof(*id) )) return cached_modref; + if (cached && !memcmp( &cached->id, id, sizeof(*id) )) return cached; mark = &NtCurrentTeb()->Peb->LdrData->InLoadOrderModuleList; for (entry = mark->Flink; entry != mark; entry = entry->Flink) @@ -850,10 +878,7 @@ static WINE_MODREF *find_fileid_module( const struct file_id *id ) WINE_MODREF *wm = CONTAINING_RECORD( mod, WINE_MODREF, ldr ); if (!memcmp( &wm->id, id, sizeof(*id) )) - { - cached_modref = wm; - return wm; - } + return set_cached_modref( wm ); } return NULL; } @@ -3810,6 +3835,7 @@ static void free_modref( WINE_MODREF *wm ) RemoveEntryList(&wm->ldr.InMemoryOrderLinks); if (wm->ldr.InInitializationOrderLinks.Flink) RemoveEntryList(&wm->ldr.InInitializationOrderLinks); + if (cached_modref == wm) cached_modref = NULL; RtlReleaseSRWLockExclusive( &ldr_data_srw_lock ); TRACE(" unloading %s\n", debugstr_w(wm->ldr.FullDllName.Buffer)); @@ -3821,7 +3847,6 @@ static void free_modref( WINE_MODREF *wm ) free_tls_slot( &wm->ldr ); RtlReleaseActivationContext( wm->ldr.ActivationContext ); NtUnmapViewOfSection( NtCurrentProcess(), wm->ldr.DllBase ); - if (cached_modref == wm) cached_modref = NULL; RtlFreeUnicodeString( &wm->ldr.FullDllName ); RtlFreeHeap( GetProcessHeap(), 0, wm->deps ); RtlFreeHeap( GetProcessHeap(), 0, wm ); -- 2.31.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- v2: - use ldr_data_srw_lock instead of introducing a new one. dlls/ntdll/loader.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 8e2ed03ad4c..5f4f4fea036 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -3858,7 +3858,7 @@ static void free_modref( WINE_MODREF *wm ) * Remove all unused modrefs and call the internal unloading routines * for the library type. * - * The loader must be locked while calling this function. + * The loader must be exclusively locked while calling this function. */ static void MODULE_FlushModrefs(void) { @@ -3889,7 +3889,7 @@ static void MODULE_FlushModrefs(void) /*********************************************************************** * MODULE_DecRefCount * - * The loader must be locked while calling this function. + * The loader must be exclusively locked while calling this function. */ static void MODULE_DecRefCount( WINE_MODREF *wm ) { @@ -3921,17 +3921,34 @@ static void MODULE_DecRefCount( WINE_MODREF *wm ) /****************************************************************** * LdrUnloadDll (NTDLL.@) * - * + * The loader must be unlocked or exclusively locked while calling this function. */ NTSTATUS WINAPI LdrUnloadDll( HMODULE hModule ) { WINE_MODREF *wm; NTSTATUS retv = STATUS_SUCCESS; + BOOL need_exclusive = FALSE; if (process_detaching) return retv; TRACE("(%p)\n", hModule); + lock_loader_shared(); + if ((wm = get_modref( hModule ))) + { + RtlAcquireSRWLockExclusive( &ldr_data_srw_lock ); + if (wm->ldr.LoadCount && wm->ldr.LoadCount != 1) + { + if (wm->ldr.LoadCount != -1) --wm->ldr.LoadCount; + } + else if (wm->ldr.LoadCount) need_exclusive = TRUE; + /* If the module is already being unloaded LdrUnloadDll() succeeds and doesn't wait. */ + RtlReleaseSRWLockExclusive( &ldr_data_srw_lock ); + } + else retv = STATUS_DLL_NOT_FOUND; + unlock_loader(); + if (!need_exclusive) return retv; + lock_loader_exclusive(); free_lib_count++; -- 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=99442 Your paranoid android. === debiant2 (32 bit Chinese:China report) === ntdll: change.c:277: Test failed: should be ready
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 5f4f4fea036..6b2e8d4686f 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -473,6 +473,16 @@ static void lock_loader_restore_exclusive(void) locked_exclusive = TRUE; } +/************************************************************************* + * is_thread_exclusive + * + * The loader must be locked while calling this function. + */ +static BOOL is_thread_exclusive(void) +{ + return RtlDllShutdownInProgress() || exclusive_lock_thread_id == GetCurrentThreadId(); +} + /************************************************************************* * get_cached_modref * @@ -3415,14 +3425,24 @@ NTSTATUS WINAPI LdrAddRefDll( ULONG flags, HMODULE module ) if (flags & ~LDR_ADDREF_DLL_PIN) FIXME( "%p flags %x not implemented\n", module, flags ); - lock_loader_exclusive(); + lock_loader_shared(); if ((wm = get_modref( module ))) { - if (flags & LDR_ADDREF_DLL_PIN) - wm->ldr.LoadCount = -1; - else - if (wm->ldr.LoadCount != -1) wm->ldr.LoadCount++; + RtlAcquireSRWLockExclusive( &ldr_data_srw_lock ); + /* If LdrAddRefDll is called during unloading from the application callback + * (DLL entry point or LDR notification) it fails with LDR_ADDREF_DLL_PIN flag + * but succeeds without, while module is still unloaded regardless. */ + if (!wm->ldr.LoadCount) + { + if (is_thread_exclusive()) + ret = flags & LDR_ADDREF_DLL_PIN ? STATUS_UNSUCCESSFUL : STATUS_SUCCESS; + else + ret = STATUS_DLL_NOT_FOUND; + } + else if (flags & LDR_ADDREF_DLL_PIN) wm->ldr.LoadCount = -1; + else if (wm->ldr.LoadCount != -1) ++wm->ldr.LoadCount; + RtlReleaseSRWLockExclusive( &ldr_data_srw_lock ); TRACE( "(%s) ldr.LoadCount: %d\n", debugstr_w(wm->ldr.BaseDllName.Buffer), wm->ldr.LoadCount ); } else ret = STATUS_INVALID_PARAMETER; -- 2.31.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 6b2e8d4686f..368ae2db42c 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -3380,11 +3380,23 @@ 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" ); - lock_loader_exclusive(); + lock_loader_shared(); +retry: status = find_dll_file( load_path, name->Buffer, L".dll", &nt_name, &wm, &mapping, &image_info, &id ); - if (wm) *base = wm->ldr.DllBase; + if (wm) + { + RtlAcquireSRWLockExclusive( &ldr_data_srw_lock ); + if (!is_thread_exclusive() && !wm->ldr.LoadCount) + { + RtlReleaseSRWLockExclusive( &ldr_data_srw_lock ); + wait_for_exclusive_lock_release(); + goto retry; + } + RtlReleaseSRWLockExclusive( &ldr_data_srw_lock ); + *base = wm->ldr.DllBase; + } else { if (status == STATUS_SUCCESS) NtClose( mapping ); @@ -3395,9 +3407,9 @@ NTSTATUS WINAPI LdrGetDllHandleEx( ULONG flags, LPCWSTR load_path, ULONG *dll_ch if (!status) { if (flags & LDR_GET_DLL_HANDLE_EX_FLAG_PIN) - LdrAddRefDll( LDR_ADDREF_DLL_PIN, *base ); + status = LdrAddRefDll( LDR_ADDREF_DLL_PIN, *base ); else if (!(flags & LDR_GET_DLL_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT)) - LdrAddRefDll( 0, *base ); + status = LdrAddRefDll( 0, *base ); } unlock_loader(); -- 2.31.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 368ae2db42c..e3610667e06 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -3305,6 +3305,8 @@ NTSTATUS WINAPI DECLSPEC_HOTPATCH LdrLoadDll(LPCWSTR path_name, DWORD flags, WINE_MODREF *wm; NTSTATUS nts; + if (!LdrGetDllHandleEx( 0, path_name, NULL, libname, hModule )) return STATUS_SUCCESS; + lock_loader_exclusive(); nts = load_dll( path_name, libname->Buffer, L".dll", flags, &wm ); -- 2.31.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/kernel32/tests/Makefile.in | 2 + dlls/kernel32/tests/actctx.c | 2 +- dlls/kernel32/tests/loader.c | 207 +++++++++++++++++++++++++++ dlls/kernel32/tests/locking_dll.c | 45 ++++++ dlls/kernel32/tests/locking_dll.spec | 1 + 5 files changed, 256 insertions(+), 1 deletion(-) create mode 100644 dlls/kernel32/tests/locking_dll.c create mode 100644 dlls/kernel32/tests/locking_dll.spec diff --git a/dlls/kernel32/tests/Makefile.in b/dlls/kernel32/tests/Makefile.in index e9516603ce9..be3a8fc8dfa 100644 --- a/dlls/kernel32/tests/Makefile.in +++ b/dlls/kernel32/tests/Makefile.in @@ -21,6 +21,8 @@ SOURCES = \ heap.c \ loader.c \ locale.c \ + locking_dll.c \ + locking_dll.spec \ mailslot.c \ module.c \ path.c \ diff --git a/dlls/kernel32/tests/actctx.c b/dlls/kernel32/tests/actctx.c index 163ea405222..f37796a6aba 100644 --- a/dlls/kernel32/tests/actctx.c +++ b/dlls/kernel32/tests/actctx.c @@ -2584,7 +2584,7 @@ static void delete_manifest_file(const char *filename) DeleteFileA(path); } -static void extract_resource(const char *name, const char *type, const char *path) +void extract_resource(const char *name, const char *type, const char *path) { DWORD written; HANDLE file; diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index e97c114c5e9..9f502aaf857 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -92,6 +92,8 @@ static BOOL (WINAPI *pWow64DisableWow64FsRedirection)(void **); static BOOL (WINAPI *pWow64RevertWow64FsRedirection)(void *); static HMODULE (WINAPI *pLoadPackagedLibrary)(LPCWSTR lpwLibFileName, DWORD Reserved); +void extract_resource(const char *name, const char *type, const char *path); + static PVOID RVAToAddr(DWORD_PTR rva, HMODULE module) { if (rva == 0) @@ -4043,6 +4045,7 @@ static void test_Wow64Transition(void) static BOOL test_loader_lock_repeat_lock; static unsigned int test_loader_notification_count; static const WCHAR *ldr_notify_track_dll; +static HMODULE lock_dll_handle; static HANDLE lock_ready_event, next_test_event; static BOOL test_loader_lock_abort_test; static volatile LONG test_loader_lock_timeout_count; @@ -4061,9 +4064,13 @@ static void CALLBACK test_loader_lock_module_enum_locking_callback(LDR_DATA_TABL static DWORD WINAPI test_loader_lock_thread(void *param) { + static const WCHAR env_var[] = L"test_wait_reason_value"; ULONG_PTR magic; NTSTATUS status; + HMODULE hmodule; + WCHAR str[32]; DWORD result; + BOOL bret; WaitForSingleObject(next_test_event, INFINITE); SetEvent(lock_ready_event); @@ -4109,6 +4116,67 @@ static DWORD WINAPI test_loader_lock_thread(void *param) SetEvent(lock_ready_event); WaitForSingleObject(next_test_event, INFINITE); + /* 3. Test with the thread blocked in DLL entry point during process attach. */ + swprintf(str, ARRAY_SIZE(str), L"%p", &test_loader_lock_timeout_count); + bret = SetEnvironmentVariableW(L"test_timeout_count_addr", str); + ok(bret, "SetEnvironmentVariableW failed.\n"); + + swprintf(str, ARRAY_SIZE(str), L"%u", DLL_PROCESS_ATTACH); + bret = SetEnvironmentVariableW(env_var, str); + ok(bret, "SetEnvironmentVariableW failed.\n"); + do + { + hmodule = LoadLibraryA("lock.dll"); + ok(!hmodule, "LoadLibrary succeeded.\n"); + ok(GetLastError() == ERROR_DLL_INIT_FAILED, "Got unexpected error %u.\n", GetLastError()); + if (test_loader_lock_timeout_count) + { + WaitForSingleObject(next_test_event, INFINITE); + test_loader_lock_timeout_count = 0; + } + } while (test_loader_lock_repeat_lock); + + /* A reference was added for the dll during process attach. */ + if (lock_dll_handle) + { + bret = FreeLibrary(lock_dll_handle); + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + bret = FreeLibrary(lock_dll_handle); + ok(!bret, "FreeLibrary succeeded.\n"); + ok(GetLastError() == ERROR_MOD_NOT_FOUND, "Got unexpected error %u.\n", GetLastError()); + } + + SetEvent(lock_ready_event); + WaitForSingleObject(next_test_event, INFINITE); + + /* 4. Test with the thread blocked in DLL entry point during process detach. */ + swprintf(str, ARRAY_SIZE(str), L"%u", DLL_PROCESS_DETACH); + bret = SetEnvironmentVariableW(env_var, str); + ok(bret, "SetEnvironmentVariableW failed.\n"); + do + { + lock_dll_handle = hmodule = LoadLibraryA("lock.dll"); + ok(!!hmodule, "LoadLibrary failed, err %u.\n", GetLastError()); + + bret = FreeLibrary(hmodule); + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + + lock_dll_handle = NULL; + + if (test_loader_lock_timeout_count) + { + WaitForSingleObject(next_test_event, INFINITE); + test_loader_lock_timeout_count = 0; + } + } while (test_loader_lock_repeat_lock); + + bret = FreeLibrary(hmodule); + ok(!bret, "FreeLibrary succeeded.\n"); + ok(GetLastError() == ERROR_MOD_NOT_FOUND, "Got unexpected error %u.\n", GetLastError()); + + SetEvent(lock_ready_event); + WaitForSingleObject(next_test_event, INFINITE); + SetEvent(lock_ready_event); return 0; } @@ -4154,13 +4222,18 @@ static void test_loader_lock(void) { static const WCHAR not_loaded_dll_name[] = L"authz.dll"; static const WCHAR preloaded_dll_name[] = L"winmm.dll"; + BOOL blocks_on_load_in_progress_module = FALSE; + BOOL blocks_on_decref_library = FALSE; HMODULE hmodule_preloaded, hmodule; ULONG_PTR magic; NTSTATUS status; HANDLE thread; void *cookie; + void *proc; BOOL bret; + extract_resource("locking_dll.dll", "TESTDLL", "lock.dll"); + lock_ready_event = CreateEventA(NULL, FALSE, FALSE, "test_lock_ready_event"); next_test_event = CreateEventA(NULL, FALSE, FALSE, "test_next_test_event"); @@ -4233,6 +4306,10 @@ static void test_loader_lock(void) ok(!status, "Got unexpected status %#x.\n", status); check_timeout(FALSE); + hmodule = GetModuleHandleW(preloaded_dll_name); + ok(!!hmodule, "LoadLibrary failed, err %u.\n", GetLastError()); + check_timeout(FALSE); + if (BLOCKING_TESTS_ENABLED) { status = pLdrLockLoaderLock(0, NULL, &magic); @@ -4246,9 +4323,139 @@ static void test_loader_lock(void) LdrUnregisterDllNotification( cookie ); check_timeout(FALSE); + /* 3. Test with the thread blocked in DLL entry point during process attach. + * DLL entry point returns failure. */ + trace("Test 3.\n"); test_loader_lock_next_test(); + status = LdrRegisterDllNotification(0, test_loader_lock_ldr_notify, NULL, &cookie); + ok(!status, "Got unexpected status %#x.\n", status); + check_timeout(FALSE); + + bret = GetModuleHandleExW(0, preloaded_dll_name, &hmodule); + check_timeout(FALSE); + ok(bret, "GetModuleHandleEx failed, err %u.\n", GetLastError()); + ok(hmodule == hmodule_preloaded, "Got unexpected hmodule %p, expected %p.\n", hmodule, hmodule_preloaded); + + status = pLdrUnloadDll(hmodule); + ok(!status, "Got unexpected status %#x.\n", status); + if (test_loader_lock_timeout_count) + { + /* Win10 1507. */ + check_timeout(TRUE); + blocks_on_decref_library = TRUE; + win_skip("LdrUnloadDll() timed out for unlrelated library without unload.\n"); + } + else + { + check_timeout(FALSE); + } + + hmodule = GetModuleHandleA("lock.dll"); + ok(!!hmodule || broken(!hmodule && GetLastError() == ERROR_MOD_NOT_FOUND) /* before Win10 1607 */, + "GetModuleHandleA hmodule %p, err %u.\n", hmodule, GetLastError()); + check_timeout(!hmodule); + + if (hmodule) + { + bret = GetModuleHandleExW(0, L"lock.dll", &hmodule); + check_timeout(FALSE); + ok(bret, "GetModuleHandleEx failed, err %u.\n", GetLastError()); + + lock_dll_handle = hmodule; + bret = FreeLibrary(hmodule); + check_timeout(blocks_on_decref_library); + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + + if (BLOCKING_TESTS_ENABLED) + { + ok(!!lock_dll_handle, "Got NULL lock_dll_handle.\n"); + proc = GetProcAddress(lock_dll_handle, "unknown"); + ok(!proc, "GetProcAddress succeeded.\n"); + check_timeout(TRUE); + } + } + else + { + /* Win8. */ + blocks_on_load_in_progress_module = TRUE; + win_skip("GetModuleHandleA failed for DLL being loaded.\n"); + } + + hmodule = LoadLibraryW(preloaded_dll_name); + check_timeout(FALSE); + ok(!!hmodule, "LoadLibrary failed, err %u.\n", GetLastError()); + + if (!blocks_on_decref_library) + { + bret = FreeLibrary(hmodule); + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + check_timeout(FALSE); + } + + if (!blocks_on_load_in_progress_module) + { + status = LdrAddRefDll(0, lock_dll_handle); + check_timeout(FALSE); + ok(!status, "Got unexpected status %#x.\n", status); + /* Leaving an extra reference to lock.dll which will fail in process attach. */ + } + + LdrUnregisterDllNotification( cookie ); + check_timeout(FALSE); + + /* 4. Test with the thread blocked in DLL entry point during process detach. */ + trace("Test 4.\n"); + test_loader_lock_next_test(); + + status = LdrRegisterDllNotification(0, test_loader_lock_ldr_notify, NULL, &cookie); + ok(!status, "Got unexpected status %#x.\n", status); + check_timeout(FALSE); + + bret = GetModuleHandleExW(0, preloaded_dll_name, &hmodule); + ok(bret, "GetModuleHandleEx failed, err %u.\n", GetLastError()); + ok(hmodule == hmodule_preloaded, "Got unexpected hmodule %p, expected %p.\n", hmodule, hmodule_preloaded); + check_timeout(FALSE); + + if (!blocks_on_decref_library) + { + bret = FreeLibrary(hmodule); + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + check_timeout(FALSE); + } + + ok(!!lock_dll_handle, "Got NULL lock_dll_handle.\n"); + + status = LdrAddRefDll(0, lock_dll_handle); + check_timeout(FALSE); + ok(status == STATUS_DLL_NOT_FOUND, "Got unexpected status %#x.\n", status); + + if (BLOCKING_TESTS_ENABLED) + { + hmodule = GetModuleHandleW(L"lock.dll"); + check_timeout(!blocks_on_load_in_progress_module); + ok(!hmodule, "GetModuleHandleW succeeded.\n", GetLastError()); + ok(GetLastError() == ERROR_MOD_NOT_FOUND, "Got unexpected error %u.\n", GetLastError()); + } + + LdrUnregisterDllNotification( cookie ); + check_timeout(FALSE); + + test_loader_lock_next_test(); + + if (blocks_on_decref_library) + { + /* Cleanup. */ + bret = FreeLibrary(hmodule_preloaded); + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + + bret = FreeLibrary(hmodule_preloaded); + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + } + done: + DeleteFileA("lock.dll"); + WaitForSingleObject(thread, INFINITE); CloseHandle(thread); diff --git a/dlls/kernel32/tests/locking_dll.c b/dlls/kernel32/tests/locking_dll.c new file mode 100644 index 00000000000..5384fae2ea3 --- /dev/null +++ b/dlls/kernel32/tests/locking_dll.c @@ -0,0 +1,45 @@ +#include <stdarg.h> +#include <stdlib.h> +#include <stdio.h> +#include "ntstatus.h" +#define WIN32_NO_STATUS +#include "windef.h" +#include "winbase.h" +#include "winternl.h" + +BOOL WINAPI DllMain( HINSTANCE instance, DWORD reason, LPVOID reserved ) +{ + LONG *test_loader_lock_timeout_count = NULL; + HANDLE lock_ready_event, next_test_event; + DWORD wait_reason = ~0u; + DWORD result; + WCHAR str[32]; + + if (GetEnvironmentVariableW(L"test_wait_reason_value", str, ARRAY_SIZE(str))) + wait_reason = _wtoi(str); + + if (reason != wait_reason) return TRUE; + + lock_ready_event = OpenEventA(EVENT_ALL_ACCESS, FALSE, "test_lock_ready_event"); + next_test_event = OpenEventA(EVENT_ALL_ACCESS, FALSE, "test_next_test_event"); + + if (GetEnvironmentVariableW(L"test_timeout_count_addr", str, ARRAY_SIZE(str))) + swscanf(str, L"%p", &test_loader_lock_timeout_count); + + SetEvent( lock_ready_event ); + result = WaitForSingleObject(next_test_event, 2000); + + if (result == WAIT_TIMEOUT) + { + if (test_loader_lock_timeout_count) + ++*test_loader_lock_timeout_count; + } + + CloseHandle(lock_ready_event); + CloseHandle(next_test_event); + return reason != DLL_PROCESS_ATTACH; +} + +void WINAPI test_proc(void) +{ +} diff --git a/dlls/kernel32/tests/locking_dll.spec b/dlls/kernel32/tests/locking_dll.spec new file mode 100644 index 00000000000..e3121f28aae --- /dev/null +++ b/dlls/kernel32/tests/locking_dll.spec @@ -0,0 +1 @@ +@ stdcall test_proc() -- 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=99446 Your paranoid android. === w10pro64 (testbot log) === WineRunTask.pl:error: The previous 1 run(s) terminated abnormally === w10pro64_ja (64 bit report) === kernel32: comm.c:886: Test failed: WaitCommEvent used 1516 ms for waiting === w7u_2qxl (32 bit report) === kernel32: debugger.c:1023: Test failed: ole32.dll was not reported === w7u_adm (32 bit report) === kernel32: debugger.c:1023: Test failed: ole32.dll was not reported === w7u_el (32 bit report) === kernel32: debugger.c:1023: Test failed: ole32.dll was not reported === w1064_tsign (32 bit report) === kernel32: debugger.c:681: Test failed: debugger reported 7 failures === w10pro64 (32 bit report) === kernel32: debugger.c:155: Test failed: unable to open 'C:\Users\winetest\AppData\Local\Temp\wt181C.tmp' debugger.c:155: Test failed: failed to open: C:\Users\winetest\AppData\Local\Temp\wt181C.tmp debugger.c:676: Test failed: the child and debugged pids don't match: 6312 != 1828 === w1064_tsign (64 bit report) === kernel32: debugger.c:155: Test failed: unable to open 'C:\Users\winetest\AppData\Local\Temp\wt5F85.tmp' debugger.c:155: Test failed: failed to open: C:\Users\winetest\AppData\Local\Temp\wt5F85.tmp debugger.c:676: Test failed: the child and debugged pids don't match: 6444 != 6068 === w10pro64 (64 bit report) === kernel32: debugger.c:155: Test failed: unable to open 'C:\Users\winetest\AppData\Local\Temp\wt1712.tmp' debugger.c:155: Test failed: failed to open: C:\Users\winetest\AppData\Local\Temp\wt1712.tmp debugger.c:676: Test failed: the child and debugged pids don't match: 296 != 8008 === w8adm (32 bit report) === kernel32: loader.c:726: Test failed: 1321: wrong status c000007b/c000035a loader.c:726: Test failed: 1330: wrong status c000007b/c000035a loader.c:726: Test failed: 1335: wrong status c000007b/c000035a loader.c:726: Test failed: 1342: wrong status c000007b/c000035a loader.c:726: Test failed: 1349: wrong status c000007b/c000035a loader.c:726: Test failed: 1357: wrong status c000007b/c000035a loader.c:726: Test failed: 1364: wrong status c000007b/c000035a loader.c:726: Test failed: 1374: wrong status c000007b/c000035a loader.c:726: Test failed: 1379: wrong status c000007b/c000035a loader.c:726: Test failed: 1384: wrong status c000007b/c000035a loader.c:726: Test failed: 1389: wrong status c000007b/c000035a loader.c:726: Test failed: 1394: wrong status c000007b/c000035a
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index e3610667e06..a503b577027 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -2053,22 +2053,21 @@ NTSTATUS WINAPI LdrUnlockLoaderLock( ULONG flags, ULONG_PTR magic ) return STATUS_SUCCESS; } - /****************************************************************** - * LdrGetProcedureAddress (NTDLL.@) + * get_procedure_address + * + * Helper for LdrGetProcedureAddress(). */ -NTSTATUS WINAPI LdrGetProcedureAddress(HMODULE module, const ANSI_STRING *name, - ULONG ord, PVOID *address) +NTSTATUS get_procedure_address( HMODULE module, const ANSI_STRING *name, + ULONG ord, void **address ) { IMAGE_EXPORT_DIRECTORY *exports; DWORD exp_size; - NTSTATUS ret = STATUS_PROCEDURE_NOT_FOUND; - - lock_loader_exclusive(); /* check if the module itself is invalid to return the proper error */ - if (!get_modref( module )) ret = STATUS_DLL_NOT_FOUND; - else if ((exports = RtlImageDirectoryEntryToData( module, TRUE, + if (!get_modref( module )) return STATUS_DLL_NOT_FOUND; + + if ((exports = RtlImageDirectoryEntryToData( module, TRUE, IMAGE_DIRECTORY_ENTRY_EXPORT, &exp_size ))) { void *proc = name ? find_named_export( module, exports, exp_size, name->Buffer, -1, NULL ) @@ -2076,10 +2075,22 @@ NTSTATUS WINAPI LdrGetProcedureAddress(HMODULE module, const ANSI_STRING *name, if (proc) { *address = proc; - ret = STATUS_SUCCESS; + return STATUS_SUCCESS; } } + return STATUS_PROCEDURE_NOT_FOUND; +} + +/****************************************************************** + * LdrGetProcedureAddress (NTDLL.@) + */ +NTSTATUS WINAPI LdrGetProcedureAddress( HMODULE module, const ANSI_STRING *name, + ULONG ord, PVOID *address ) +{ + NTSTATUS ret; + lock_loader_exclusive(); + ret = get_procedure_address( module, name, ord, address ); unlock_loader(); return ret; } -- 2.31.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/kernel32/tests/loader.c | 24 ++++++++++++++ dlls/ntdll/loader.c | 62 ++++++++++++++++++++++++++---------- 2 files changed, 70 insertions(+), 16 deletions(-) diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 9f502aaf857..ba979a26a32 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -4337,6 +4337,10 @@ static void test_loader_lock(void) ok(bret, "GetModuleHandleEx failed, err %u.\n", GetLastError()); ok(hmodule == hmodule_preloaded, "Got unexpected hmodule %p, expected %p.\n", hmodule, hmodule_preloaded); + proc = GetProcAddress(hmodule, "timeGetTime"); + check_timeout(FALSE); + ok(!!proc, "GetProcAddress failed.\n"); + status = pLdrUnloadDll(hmodule); ok(!status, "Got unexpected status %#x.\n", status); if (test_loader_lock_timeout_count) @@ -4417,6 +4421,10 @@ static void test_loader_lock(void) ok(hmodule == hmodule_preloaded, "Got unexpected hmodule %p, expected %p.\n", hmodule, hmodule_preloaded); check_timeout(FALSE); + proc = GetProcAddress(hmodule, "timeGetTime"); + ok(!!proc, "GetProcAddress failed.\n"); + check_timeout(FALSE); + if (!blocks_on_decref_library) { bret = FreeLibrary(hmodule); @@ -4426,6 +4434,22 @@ static void test_loader_lock(void) ok(!!lock_dll_handle, "Got NULL lock_dll_handle.\n"); + if (BLOCKING_TESTS_ENABLED) + { + proc = GetProcAddress(lock_dll_handle, "test_proc"); + if (GetLastError() == ERROR_MOD_NOT_FOUND && !blocks_on_load_in_progress_module) + { + /* Win 10 1507-1709. */ + check_timeout(TRUE); + } + else + { + check_timeout(FALSE); + } + ok(!proc, "GetProcAddress failed, err %u.\n", GetLastError()); + ok(GetLastError() == ERROR_INVALID_PARAMETER || broken(GetLastError() == ERROR_MOD_NOT_FOUND), + "Got unexpected error %u.\n", GetLastError()); + } status = LdrAddRefDll(0, lock_dll_handle); check_timeout(FALSE); ok(status == STATUS_DLL_NOT_FOUND, "Got unexpected status %#x.\n", status); diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index a503b577027..a044b9cce27 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -217,9 +217,9 @@ static NTSTATUS load_dll( const WCHAR *load_path, const WCHAR *libname, const WC DWORD flags, WINE_MODREF** pwm ); static NTSTATUS process_attach( WINE_MODREF *wm, LPVOID lpReserved ); static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, - DWORD exp_size, DWORD ordinal, LPCWSTR load_path ); + DWORD exp_size, DWORD ordinal, LPCWSTR load_path, NTSTATUS *status ); static FARPROC find_named_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, - DWORD exp_size, const char *name, int hint, LPCWSTR load_path ); + DWORD exp_size, const char *name, int hint, LPCWSTR load_path, NTSTATUS *status ); /* convert PE image VirtualAddress to Real Address */ static inline void *get_rva( HMODULE module, DWORD va ) @@ -921,7 +921,7 @@ static WINE_MODREF **grow_module_deps( WINE_MODREF *wm, int count ) * Find the final function pointer for a forwarded function. * The loader must be locked while calling this function. */ -static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWSTR load_path ) +static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWSTR load_path, NTSTATUS *status ) { const IMAGE_EXPORT_DIRECTORY *exports; DWORD exp_size; @@ -944,6 +944,14 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS if (!(wm = find_basename_module( mod_name ))) { + if (!locked_exclusive) + { + TRACE( "Need to load %s for '%s' while not in exclusive lock.\n", debugstr_w(mod_name), forward ); + assert( status ); + if (mod_name != buffer) RtlFreeHeap( GetProcessHeap(), 0, mod_name ); + if (status) *status = STATUS_NOT_FOUND; + return NULL; + } TRACE( "delay loading %s for '%s'\n", debugstr_w(mod_name), forward ); if (load_dll( load_path, mod_name, L".dll", 0, &wm ) == STATUS_SUCCESS && !(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) @@ -975,9 +983,9 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS if (*name == '#') { /* ordinal */ proc = find_ordinal_export( wm->ldr.DllBase, exports, exp_size, - atoi(name+1) - exports->Base, load_path ); + atoi(name+1) - exports->Base, load_path, status ); } else - proc = find_named_export( wm->ldr.DllBase, exports, exp_size, name, -1, load_path ); + proc = find_named_export( wm->ldr.DllBase, exports, exp_size, name, -1, load_path, status ); } if (!proc) @@ -1000,11 +1008,13 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS * 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 ) + DWORD exp_size, DWORD ordinal, LPCWSTR load_path, NTSTATUS *status ) { FARPROC proc; const DWORD *functions = get_rva( module, exports->AddressOfFunctions ); + if (status) *status = STATUS_PROCEDURE_NOT_FOUND; + if (ordinal >= exports->NumberOfFunctions) { TRACE(" ordinal %d out of range!\n", ordinal + exports->Base ); @@ -1017,7 +1027,7 @@ static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY /* if the address falls into the export dir, it's a forward */ if (((const char *)proc >= (const char *)exports) && ((const char *)proc < (const char *)exports + exp_size)) - return find_forwarded_export( module, (const char *)proc, load_path ); + return find_forwarded_export( module, (const char *)proc, load_path, status ); if (TRACE_ON(snoop)) { @@ -1063,7 +1073,7 @@ static int find_name_in_exports( HMODULE module, const IMAGE_EXPORT_DIRECTORY *e * 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 ) + DWORD exp_size, const char *name, int hint, LPCWSTR load_path, NTSTATUS *status ) { const WORD *ordinals = get_rva( module, exports->AddressOfNameOrdinals ); const DWORD *names = get_rva( module, exports->AddressOfNames ); @@ -1074,12 +1084,12 @@ static FARPROC find_named_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY * { char *ename = get_rva( module, names[hint] ); if (!strcmp( ename, name )) - return find_ordinal_export( module, exports, exp_size, ordinals[hint], load_path ); + return find_ordinal_export( module, exports, exp_size, ordinals[hint], load_path, status ); } /* then do a binary search */ if ((ordinal = find_name_in_exports( module, exports, name )) == -1) return NULL; - return find_ordinal_export( module, exports, exp_size, ordinal, load_path ); + return find_ordinal_export( module, exports, exp_size, ordinal, load_path, status ); } @@ -1213,7 +1223,7 @@ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LP int ordinal = IMAGE_ORDINAL(import_list->u1.Ordinal); thunk_list->u1.Function = (ULONG_PTR)find_ordinal_export( imp_mod, exports, exp_size, - ordinal - exports->Base, load_path ); + ordinal - exports->Base, load_path, NULL ); if (!thunk_list->u1.Function) { thunk_list->u1.Function = allocate_stub( name, IntToPtr(ordinal) ); @@ -1229,7 +1239,7 @@ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LP pe_name = get_rva( module, (DWORD)import_list->u1.AddressOfData ); thunk_list->u1.Function = (ULONG_PTR)find_named_export( imp_mod, exports, exp_size, (const char*)pe_name->Name, - pe_name->Hint, load_path ); + pe_name->Hint, load_path, NULL ); if (!thunk_list->u1.Function) { thunk_list->u1.Function = allocate_stub( name, (const char*)pe_name->Name ); @@ -2061,24 +2071,37 @@ NTSTATUS WINAPI LdrUnlockLoaderLock( ULONG flags, ULONG_PTR magic ) NTSTATUS get_procedure_address( HMODULE module, const ANSI_STRING *name, ULONG ord, void **address ) { + NTSTATUS status = STATUS_PROCEDURE_NOT_FOUND; IMAGE_EXPORT_DIRECTORY *exports; + WINE_MODREF *wm; DWORD exp_size; +retry: /* check if the module itself is invalid to return the proper error */ - if (!get_modref( module )) return STATUS_DLL_NOT_FOUND; + if (!(wm = get_modref( module ))) return STATUS_DLL_NOT_FOUND; + + RtlAcquireSRWLockExclusive( &ldr_data_srw_lock ); + if (!is_thread_exclusive() && (!wm->ldr.LoadCount || (wm->ldr.Flags & LDR_LOAD_IN_PROGRESS))) + { + RtlReleaseSRWLockExclusive( &ldr_data_srw_lock ); + if (!wm->ldr.LoadCount) return STATUS_INVALID_PARAMETER; + wait_for_exclusive_lock_release(); + goto retry; + } + RtlReleaseSRWLockExclusive( &ldr_data_srw_lock ); if ((exports = RtlImageDirectoryEntryToData( module, TRUE, IMAGE_DIRECTORY_ENTRY_EXPORT, &exp_size ))) { - void *proc = name ? find_named_export( module, exports, exp_size, name->Buffer, -1, NULL ) - : find_ordinal_export( module, exports, exp_size, ord - exports->Base, NULL ); + void *proc = name ? find_named_export( module, exports, exp_size, name->Buffer, -1, NULL, &status ) + : find_ordinal_export( module, exports, exp_size, ord - exports->Base, NULL, &status ); if (proc) { *address = proc; return STATUS_SUCCESS; } } - return STATUS_PROCEDURE_NOT_FOUND; + return status; } /****************************************************************** @@ -2089,9 +2112,16 @@ NTSTATUS WINAPI LdrGetProcedureAddress( HMODULE module, const ANSI_STRING *name, { NTSTATUS ret; + lock_loader_shared(); + ret = get_procedure_address( module, name, ord, address ); + unlock_loader(); + + if (ret != STATUS_NOT_FOUND) return ret; + lock_loader_exclusive(); ret = get_procedure_address( module, name, ord, address ); unlock_loader(); + 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=99448 Your paranoid android. === w8adm (32 bit report) === kernel32: loader.c:726: Test failed: 1321: wrong status c000007b/c000035a loader.c:726: Test failed: 1330: wrong status c000007b/c000035a loader.c:726: Test failed: 1335: wrong status c000007b/c000035a loader.c:726: Test failed: 1342: wrong status c000007b/c000035a loader.c:726: Test failed: 1349: wrong status c000007b/c000035a loader.c:726: Test failed: 1357: wrong status c000007b/c000035a loader.c:726: Test failed: 1364: wrong status c000007b/c000035a loader.c:726: Test failed: 1374: wrong status c000007b/c000035a loader.c:726: Test failed: 1379: wrong status c000007b/c000035a loader.c:726: Test failed: 1384: wrong status c000007b/c000035a loader.c:726: Test failed: 1389: wrong status c000007b/c000035a loader.c:726: Test failed: 1394: wrong status c000007b/c000035a === w1064_2qxl (64 bit report) === kernel32: loader.c:726: Test failed: 1202: wrong status c000011b/c0000131 loader.c:726: Test failed: 1202: wrong status c000011b/c0000130 loader.c:726: Test failed: 1219: wrong status c000011b/c0000130 loader.c:726: Test failed: 1224: wrong status c000011b/c0000130 loader.c:726: Test failed: 1229: wrong status c000011b/c000007b loader.c:726: Test failed: 1234: wrong status c000011b/c000007b loader.c:726: Test failed: 1239: wrong status c000011b/c000007b loader.c:726: Test failed: 1244: wrong status c000011b/c000007b loader.c:726: Test failed: 1249: wrong status c000011b/c000007b loader.c:726: Test failed: 1265: wrong status c000011b/0 loader.c:726: Test failed: 1269: wrong status c000011b/0 loader.c:726: Test failed: 1274: wrong status c000011b/0 loader.c:726: Test failed: 1278: wrong status c000011b/0 loader.c:726: Test failed: 1282: wrong status c000011b/0 loader.c:726: Test failed: 1436: wrong status c000011b/c000007b loader.c:726: Test failed: 1444: wrong status c000011b/c000007b loader.c:726: Test failed: 1449: wrong status c000011b/0 loader.c:729: Test failed: 1449: failed with c000011b expected fallback loader.c:726: Test failed: 1455: wrong status c000011b/0 loader.c:729: Test failed: 1455: failed with c000011b expected fallback loader.c:726: Test failed: 1461: wrong status c000011b/0 loader.c:729: Test failed: 1461: failed with c000011b expected fallback loader.c:726: Test failed: 1468: wrong status c000011b/0 loader.c:729: Test failed: 1468: failed with c000011b expected fallback loader.c:726: Test failed: 1474: wrong status c000011b/0 loader.c:726: Test failed: 1483: wrong status c000011b/0 loader.c:726: Test failed: 1487: wrong status c000011b/0 loader.c:726: Test failed: 1491: wrong status c000011b/0 loader.c:726: Test failed: 1495: wrong status c000011b/0 loader.c:726: Test failed: 1499: wrong status c000011b/0
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index a044b9cce27..eaca55acf65 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -1897,13 +1897,16 @@ NTSTATUS WINAPI LdrDisableThreadCalloutsForDll(HMODULE hModule) WINE_MODREF *wm; NTSTATUS ret = STATUS_SUCCESS; - lock_loader_exclusive(); + lock_loader_shared(); wm = get_modref( hModule ); + + RtlAcquireSRWLockExclusive( &ldr_data_srw_lock ); if (!wm || wm->ldr.TlsIndex != -1) ret = STATUS_DLL_NOT_FOUND; else wm->ldr.Flags |= LDR_NO_DLL_CALLS; + RtlReleaseSRWLockExclusive( &ldr_data_srw_lock ); unlock_loader(); -- 2.31.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/kernel32/tests/loader.c | 19 +++++++++++++++++++ dlls/ntdll/loader.c | 2 +- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index ba979a26a32..5a706fadc48 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -4225,11 +4225,13 @@ static void test_loader_lock(void) BOOL blocks_on_load_in_progress_module = FALSE; BOOL blocks_on_decref_library = FALSE; HMODULE hmodule_preloaded, hmodule; + WCHAR buffer[MAX_PATH]; ULONG_PTR magic; NTSTATUS status; HANDLE thread; void *cookie; void *proc; + DWORD ret; BOOL bret; extract_resource("locking_dll.dll", "TESTDLL", "lock.dll"); @@ -4337,6 +4339,10 @@ static void test_loader_lock(void) ok(bret, "GetModuleHandleEx failed, err %u.\n", GetLastError()); ok(hmodule == hmodule_preloaded, "Got unexpected hmodule %p, expected %p.\n", hmodule, hmodule_preloaded); + ret = GetModuleFileNameW(hmodule, buffer, ARRAY_SIZE(buffer)); + check_timeout(FALSE); + ok(ret, "Got zero ret.\n"); + proc = GetProcAddress(hmodule, "timeGetTime"); check_timeout(FALSE); ok(!!proc, "GetProcAddress failed.\n"); @@ -4371,6 +4377,10 @@ static void test_loader_lock(void) check_timeout(blocks_on_decref_library); ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + ret = GetModuleFileNameW(lock_dll_handle, buffer, ARRAY_SIZE(buffer)); + check_timeout(FALSE); + ok(ret, "Got zero ret.\n"); + if (BLOCKING_TESTS_ENABLED) { ok(!!lock_dll_handle, "Got NULL lock_dll_handle.\n"); @@ -4421,6 +4431,10 @@ static void test_loader_lock(void) ok(hmodule == hmodule_preloaded, "Got unexpected hmodule %p, expected %p.\n", hmodule, hmodule_preloaded); check_timeout(FALSE); + ret = GetModuleFileNameW(hmodule, buffer, ARRAY_SIZE(buffer)); + check_timeout(FALSE); + ok(ret, "Got zero ret.\n"); + proc = GetProcAddress(hmodule, "timeGetTime"); ok(!!proc, "GetProcAddress failed.\n"); check_timeout(FALSE); @@ -4454,6 +4468,11 @@ static void test_loader_lock(void) check_timeout(FALSE); ok(status == STATUS_DLL_NOT_FOUND, "Got unexpected status %#x.\n", status); + ret = GetModuleFileNameW(lock_dll_handle, buffer, ARRAY_SIZE(buffer)); + check_timeout(FALSE); + ok((!blocks_on_load_in_progress_module && ret) || (blocks_on_load_in_progress_module && !ret), + "Got unexpected ret %u.\n", ret); + if (BLOCKING_TESTS_ENABLED) { hmodule = GetModuleHandleW(L"lock.dll"); diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index eaca55acf65..65cf618626b 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -3383,7 +3383,7 @@ NTSTATUS WINAPI LdrGetDllFullName( HMODULE module, UNICODE_STRING *name ) if (!module) module = NtCurrentTeb()->Peb->ImageBaseAddress; - lock_loader_exclusive(); + lock_loader_shared(); wm = get_modref( module ); if (wm) { -- 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=99450 Your paranoid android. === w1064_tsign (32 bit report) === kernel32: loader.c:2820: Test failed: attached thread count should be 2 === debiant2 (32 bit Chinese:China report) === ntdll: change.c:277: Test failed: should be ready
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- - fixed RtlIsCriticalSectionLocked() -> RtlIsCriticalSectionLockedByThread(). dlls/kernel32/tests/loader.c | 97 +++++++++++++++++++++++++++++++++++- dlls/ntdll/loader.c | 17 +++++++ 2 files changed, 113 insertions(+), 1 deletion(-) diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 5a706fadc48..8532eca2633 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -4049,6 +4049,7 @@ static HMODULE lock_dll_handle; static HANDLE lock_ready_event, next_test_event; static BOOL test_loader_lock_abort_test; static volatile LONG test_loader_lock_timeout_count; +static BOOL blocks_on_load_in_progress_module; #define BLOCKING_TESTS_ENABLED 0 @@ -4062,6 +4063,53 @@ static void CALLBACK test_loader_lock_module_enum_locking_callback(LDR_DATA_TABL *stop = TRUE; } +static void CALLBACK test_loader_lock_dummy_callback(LDR_DATA_TABLE_ENTRY *mod, + void *context, BOOLEAN *stop) +{ + *stop = TRUE; +} + +static void CALLBACK test_loader_lock_ldr_notify_locking(ULONG reason, LDR_DLL_NOTIFICATION_DATA *data, void *context) +{ + NTSTATUS status; + HMODULE hmodule; + void *cookie; + BOOL bret; + + if (reason != LDR_DLL_NOTIFICATION_REASON_UNLOADED) return; + + status = LdrRegisterDllNotification(0, test_loader_lock_ldr_notify_locking, NULL, &cookie); + ok(!status, "Got unexpected status %#x.\n", status); + status = LdrUnregisterDllNotification( cookie ); + ok(!status, "Got unexpected status %#x.\n", status); + + hmodule = LoadLibraryW(L"winmm.dll"); + ok(!!hmodule, "GetModuleHandleW failed, err %u.\n", GetLastError()); + bret = FreeLibrary(hmodule); + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + + if (!blocks_on_load_in_progress_module) + { + bret = GetModuleHandleExW(0, L"lock.dll", &hmodule); + ok(bret, "GetModuleHandleExW failed, err %u.\n", GetLastError()); + ok(!!hmodule, "GetModuleHandleW failed, err %u.\n", GetLastError()); + bret = FreeLibrary(hmodule); + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + } + + hmodule = LoadLibraryW(L"authz.dll"); + ok(!hmodule, "LoadLibraryW succeeded.\n"); + ok(GetLastError() == ERROR_NOT_FOUND || broken(blocks_on_load_in_progress_module + && GetLastError() == ERROR_MOD_NOT_FOUND), "Got unexpected error %u.\n", GetLastError()); + + status = pLdrEnumerateLoadedModules(NULL, test_loader_lock_dummy_callback, NULL); + ok(!status, "Got unexpected status %#x.\n", status); + + SetEvent(lock_ready_event); + if (WaitForSingleObject(next_test_event, 3000) == WAIT_TIMEOUT) + ++test_loader_lock_timeout_count; +} + static DWORD WINAPI test_loader_lock_thread(void *param) { static const WCHAR env_var[] = L"test_wait_reason_value"; @@ -4069,6 +4117,7 @@ static DWORD WINAPI test_loader_lock_thread(void *param) NTSTATUS status; HMODULE hmodule; WCHAR str[32]; + void *cookie; DWORD result; BOOL bret; @@ -4177,6 +4226,28 @@ static DWORD WINAPI test_loader_lock_thread(void *param) SetEvent(lock_ready_event); WaitForSingleObject(next_test_event, INFINITE); + /* 5. Test with the thread blocked in LDR notification callback. */ + LdrRegisterDllNotification(0, test_loader_lock_ldr_notify_locking, NULL, &cookie); + /* lock.dll will return FALSE from the DLL entry point. */ + bret = SetEnvironmentVariableW(env_var, L"-1"); + ok(bret, "SetEnvironmentVariableW failed.\n"); + do + { + hmodule = LoadLibraryA("lock.dll"); + ok(!!hmodule, "LoadLibrary failed, err %u.\n", GetLastError()); + bret = FreeLibrary(hmodule); + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + if (test_loader_lock_timeout_count) + { + WaitForSingleObject(next_test_event, INFINITE); + test_loader_lock_timeout_count = 0; + } + } while (test_loader_lock_repeat_lock); + LdrUnregisterDllNotification( cookie ); + + SetEvent(lock_ready_event); + WaitForSingleObject(next_test_event, INFINITE); + SetEvent(lock_ready_event); return 0; } @@ -4222,7 +4293,6 @@ static void test_loader_lock(void) { static const WCHAR not_loaded_dll_name[] = L"authz.dll"; static const WCHAR preloaded_dll_name[] = L"winmm.dll"; - BOOL blocks_on_load_in_progress_module = FALSE; BOOL blocks_on_decref_library = FALSE; HMODULE hmodule_preloaded, hmodule; WCHAR buffer[MAX_PATH]; @@ -4484,6 +4554,31 @@ static void test_loader_lock(void) LdrUnregisterDllNotification( cookie ); check_timeout(FALSE); + /* 5. Test with the thread blocked in LDR notification callback. */ + trace("Test 5.\n"); + test_loader_lock_next_test(); + + hmodule = GetModuleHandleW(preloaded_dll_name); + ok(!!hmodule, "LoadLibrary failed, err %u.\n", GetLastError()); + check_timeout(FALSE); + + if (BLOCKING_TESTS_ENABLED) + { + status = LdrRegisterDllNotification(0, test_loader_lock_ldr_notify, NULL, &cookie); + check_timeout(TRUE); + ok(!status, "Got unexpected status %#x.\n", status); + status = LdrUnregisterDllNotification( cookie ); + check_timeout(TRUE); + ok(!status, "Got unexpected status %#x.\n", status); + + /* This doesn't block in load notifications on Windows, only unload. */ + status = pLdrLockLoaderLock(0, NULL, &magic); + ok(!status, "Got unexpected status %#x.\n", status); + status = pLdrUnlockLoaderLock(0, magic); + ok(!status, "Got unexpected status %#x.\n", status); + check_timeout(TRUE); + } + test_loader_lock_next_test(); if (blocks_on_decref_library) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 65cf618626b..c1a826914ea 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -2121,6 +2121,12 @@ NTSTATUS WINAPI LdrGetProcedureAddress( HMODULE module, const ANSI_STRING *name, if (ret != STATUS_NOT_FOUND) return ret; + if (RtlIsCriticalSectionLockedByThread( &ldr_notifications_section )) + { + WARN( "Attempt to unload a library from notification callback.\n" ); + return STATUS_NOT_FOUND; + } + lock_loader_exclusive(); ret = get_procedure_address( module, name, ord, address ); unlock_loader(); @@ -3351,6 +3357,11 @@ NTSTATUS WINAPI DECLSPEC_HOTPATCH LdrLoadDll(LPCWSTR path_name, DWORD flags, if (!LdrGetDllHandleEx( 0, path_name, NULL, libname, hModule )) return STATUS_SUCCESS; + if (RtlIsCriticalSectionLockedByThread( &ldr_notifications_section )) + { + WARN( "Attempt to load a new library from notification callback.\n" ); + return STATUS_NOT_FOUND; + } lock_loader_exclusive(); nts = load_dll( path_name, libname->Buffer, L".dll", flags, &wm ); @@ -4027,6 +4038,12 @@ NTSTATUS WINAPI LdrUnloadDll( HMODULE hModule ) unlock_loader(); if (!need_exclusive) return retv; + if (RtlIsCriticalSectionLockedByThread( &ldr_notifications_section )) + { + WARN( "Attempt to unload a library from notification callback.\n" ); + return STATUS_NOT_FOUND; + } + lock_loader_exclusive(); free_lib_count++; -- 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=99451 Your paranoid android. === w1064_tsign (32 bit report) === kernel32: loader.c:2820: Test failed: attached thread count should be 2
participants (2)
-
Marvin -
Paul Gofman