[PATCH 00/22] ntdll: Relax the loader locking to match newer Windows behaviour.
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 | 27 ++++++++++++++++++++++++--- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/dlls/ntdll/actctx.c b/dlls/ntdll/actctx.c index f9afb655885..bc222dadf27 100644 --- a/dlls/ntdll/actctx.c +++ b/dlls/ntdll/actctx.c @@ -705,10 +705,8 @@ static inline const char* debugstr_version(const struct assembly_version *ver) static NTSTATUS get_module_filename( HMODULE module, UNICODE_STRING *str, unsigned int extra_len ) { NTSTATUS status; - ULONG_PTR magic; LDR_DATA_TABLE_ENTRY *pldr; - LdrLockLoaderLock(0, NULL, &magic); status = LdrFindEntryForAddress( module, &pldr ); if (status == STATUS_SUCCESS) { @@ -721,7 +719,6 @@ static NTSTATUS get_module_filename( HMODULE module, UNICODE_STRING *str, unsign } else status = STATUS_NO_MEMORY; } - LdrUnlockLoaderLock(0, magic); return status; } @@ -3301,12 +3298,10 @@ static NTSTATUS find_query_actctx( HANDLE *handle, DWORD flags, ULONG class ) } else if (flags & (QUERY_ACTCTX_FLAG_ACTCTX_IS_ADDRESS|QUERY_ACTCTX_FLAG_ACTCTX_IS_HMODULE)) { - ULONG_PTR magic; LDR_DATA_TABLE_ENTRY *pldr; if (!*handle) return STATUS_INVALID_PARAMETER; - LdrLockLoaderLock( 0, NULL, &magic ); if (!LdrFindEntryForAddress( *handle, &pldr )) { if ((flags & QUERY_ACTCTX_FLAG_ACTCTX_IS_HMODULE) && *handle != pldr->DllBase) @@ -3315,7 +3310,6 @@ static NTSTATUS find_query_actctx( HANDLE *handle, DWORD flags, ULONG class ) *handle = pldr->ActivationContext; } else status = STATUS_DLL_NOT_FOUND; - LdrUnlockLoaderLock( 0, magic ); } else if (!*handle && (class != ActivationContextBasicInformation)) *handle = process_actctx; diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index e00c1e0c31d..f3b9837194b 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -176,6 +176,10 @@ static PEB_LDR_DATA ldr = { &ldr.InMemoryOrderModuleList, &ldr.InMemoryOrderModuleList }, { &ldr.InInitializationOrderModuleList, &ldr.InInitializationOrderModuleList } }; +/* Ldr data is modified with loader locked and exclusive lock held. + * Taking shared lock to access the data is required outside of loader lock only. + */ +static RTL_SRWLOCK ldr_data_srw_lock; 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)) @@ -3851,6 +3866,12 @@ void WINAPI LdrInitializeThunk( CONTEXT *context, ULONG_PTR unknown2, ULONG_PTR if (process_detaching) NtTerminateThread( GetCurrentThread(), 0 ); + if (!imports_fixup_done) + { + TRACE( "Initializing loader locks.\n" ); + RtlInitializeSRWLock( &ldr_data_srw_lock ); + } + RtlEnterCriticalSection( &loader_section ); if (!imports_fixup_done) -- 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 f3b9837194b..1c4faf2fba2 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 ); } @@ -3872,7 +3892,7 @@ void WINAPI LdrInitializeThunk( CONTEXT *context, ULONG_PTR unknown2, ULONG_PTR RtlInitializeSRWLock( &ldr_data_srw_lock ); } - RtlEnterCriticalSection( &loader_section ); + lock_loader_exclusive(); if (!imports_fixup_done) { @@ -3985,7 +4005,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 ); } @@ -4075,9 +4095,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 1c4faf2fba2..3081bb31434 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -115,6 +115,15 @@ struct ldr_notification static struct list ldr_notifications = LIST_INIT( ldr_notifications ); +static CRITICAL_SECTION ldr_notifications_section; +static CRITICAL_SECTION_DEBUG ldr_notifications_critsect_debug = +{ + 0, 0, &ldr_notifications_section, + { &ldr_notifications_critsect_debug.ProcessLocksList, &ldr_notifications_critsect_debug.ProcessLocksList }, + 0, 0, { (DWORD_PTR)(__FILE__ ": dlldir_section") } +}; +static CRITICAL_SECTION ldr_notifications_section = { &ldr_notifications_critsect_debug, -1, 0, 0, 0, 0 }; + static const char * const reason_names[] = { "PROCESS_DETACH", @@ -485,6 +494,7 @@ static void call_ldr_notifications( ULONG reason, LDR_DATA_TABLE_ENTRY *module ) data.Loaded.DllBase = module->DllBase; data.Loaded.SizeOfImage = module->SizeOfImage; + RtlEnterCriticalSection( &ldr_notifications_section ); LIST_FOR_EACH_ENTRY_SAFE( notify, notify_next, &ldr_notifications, struct ldr_notification, entry ) { TRACE_(relay)("\1Call LDR notification callback (proc=%p,reason=%u,data=%p,context=%p)\n", @@ -495,6 +505,7 @@ static void call_ldr_notifications( ULONG reason, LDR_DATA_TABLE_ENTRY *module ) TRACE_(relay)("\1Ret LDR notification callback (proc=%p,reason=%u,data=%p,context=%p)\n", notify->callback, reason, &data, notify->context ); } + RtlLeaveCriticalSection( &ldr_notifications_section ); } /************************************************************************* @@ -1681,9 +1692,9 @@ NTSTATUS WINAPI LdrRegisterDllNotification(ULONG flags, PLDR_DLL_NOTIFICATION_FU notify->callback = callback; notify->context = context; - lock_loader_exclusive(); + RtlEnterCriticalSection( &ldr_notifications_section ); list_add_tail( &ldr_notifications, ¬ify->entry ); - unlock_loader(); + RtlLeaveCriticalSection( &ldr_notifications_section ); *cookie = notify; return STATUS_SUCCESS; @@ -1700,9 +1711,9 @@ NTSTATUS WINAPI LdrUnregisterDllNotification( void *cookie ) if (!notify) return STATUS_INVALID_PARAMETER; - lock_loader_exclusive(); + RtlEnterCriticalSection( &ldr_notifications_section ); list_remove( ¬ify->entry ); - unlock_loader(); + RtlLeaveCriticalSection( &ldr_notifications_section ); RtlFreeHeap( GetProcessHeap(), 0, notify ); return STATUS_SUCCESS; -- 2.31.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 59 ++++++++++++++++++++++++++++++++++++++++----- include/winternl.h | 2 +- 2 files changed, 54 insertions(+), 7 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 3081bb31434..4eb1eac09ed 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; + 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; } @@ -3901,6 +3947,7 @@ void WINAPI LdrInitializeThunk( CONTEXT *context, ULONG_PTR unknown2, ULONG_PTR { TRACE( "Initializing loader locks.\n" ); RtlInitializeSRWLock( &ldr_data_srw_lock ); + RtlInitializeSRWLock( &loader_srw_lock ); } lock_loader_exclusive(); 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=99222 Your paranoid android. === debiant2 (32 bit report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit Chinese:China report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (64 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events.
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 4eb1eac09ed..76459c21679 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; +static volatile BOOL locked_exclusive; static CRITICAL_SECTION dlldir_section; static CRITICAL_SECTION_DEBUG dlldir_critsect_debug = @@ -250,8 +251,32 @@ static void lock_loader_exclusive(void) ULONG recursion_count = inc_recursion_count(); TRACE( "recursion_count %u.\n", recursion_count ); + if (!recursion_count) + { + if (!RtlDllShutdownInProgress()) + RtlAcquireSRWLockExclusive( &loader_srw_lock ); + locked_exclusive = TRUE; + } + else + { + assert( locked_exclusive ); + } +} + +/************************************************************************* + * lock_loader_shared + * + * Take shared ownership of internal loader lock. + * If the thread already has exclusive lock it will stay exclusive. + */ +static void lock_loader_shared(void) +{ + ULONG recursion_count = inc_recursion_count(); + + TRACE("recursion_count %u, locked_exclusive %d.\n", recursion_count, locked_exclusive); + if (!recursion_count && !RtlDllShutdownInProgress()) - RtlAcquireSRWLockExclusive( &loader_srw_lock ); + RtlAcquireSRWLockShared( &loader_srw_lock ); } /************************************************************************* @@ -269,8 +294,14 @@ static void unlock_loader(void) assert( recursion_count != ~0u ); - if (!recursion_count) + if (recursion_count) return; + + if (locked_exclusive) + { + locked_exclusive = FALSE; RtlReleaseSRWLockExclusive( &loader_srw_lock ); + } + else RtlReleaseSRWLockShared( &loader_srw_lock ); } #define RTL_UNLOAD_EVENT_TRACE_NUMBER 64 @@ -3283,7 +3314,7 @@ NTSTATUS WINAPI LdrQueryProcessModuleInformation(RTL_PROCESS_MODULES *smi, smi->ModulesCount = 0; - lock_loader_exclusive(); + lock_loader_shared(); mark = &NtCurrentTeb()->Peb->LdrData->InLoadOrderModuleList; for (entry = mark->Flink; entry != mark; entry = entry->Flink) { -- 2.31.1
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=99223 Your paranoid android. === debiant2 (32 bit report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit Chinese:China report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (64 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events.
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 76459c21679..df83f4f579d 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -4184,9 +4184,7 @@ PVOID WINAPI RtlPcToFileHeader( PVOID pc, PVOID *address ) LDR_DATA_TABLE_ENTRY *module; PVOID ret = NULL; - lock_loader_exclusive(); if (!LdrFindEntryForAddress( pc, &module )) ret = module->DllBase; - unlock_loader(); *address = ret; return ret; } -- 2.31.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=99224 Your paranoid android. === debiant2 (32 bit report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit Chinese:China report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (64 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events.
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 193 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 181 insertions(+), 12 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index df83f4f579d..8bcd4e6a237 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; static volatile BOOL locked_exclusive; +static RTL_CONDITION_VARIABLE exclusive_lock_released_cv; + +#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 @@ -3979,6 +4147,7 @@ void WINAPI LdrInitializeThunk( CONTEXT *context, ULONG_PTR unknown2, ULONG_PTR TRACE( "Initializing loader locks.\n" ); RtlInitializeSRWLock( &ldr_data_srw_lock ); RtlInitializeSRWLock( &loader_srw_lock ); + RtlInitializeConditionVariable( &exclusive_lock_released_cv ); } lock_loader_exclusive(); -- 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=99225 Your paranoid android. === debiant2 (32 bit report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit Chinese:China report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. virtual: Timeout === debiant2 (32 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (64 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events.
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 8bcd4e6a237..a7a7089ca66 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
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=99226 Your paranoid android. === debiant2 (32 bit report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit Chinese:China report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (64 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events.
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 a7a7089ca66..dae4ce13b44 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
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=99227 Your paranoid android. === debiant2 (32 bit report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit Chinese:China report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (64 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events.
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 dae4ce13b44..9397c927c3e 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
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=99228 Your paranoid android. === debiant2 (32 bit report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit Chinese:China report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (64 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events.
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 59 +++++++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 21 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 9397c927c3e..a8222ec30df 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -206,7 +206,9 @@ static RTL_SRWLOCK ldr_data_srw_lock; static RTL_BITMAP tls_bitmap; static RTL_BITMAP tls_expansion_bitmap; -static WINE_MODREF *cached_modref; +static WINE_MODREF * volatile cached_modref; + +/* Used with exclusive loader lock only. */ static WINE_MODREF *current_modref; static WINE_MODREF *last_failed_modref; @@ -470,6 +472,26 @@ static void lock_loader_restore_exclusive(void) locked_exclusive = TRUE; } +/************************************************************************* + * get_cached_modref + * + */ +static WINE_MODREF *get_cached_modref(void) +{ + return cached_modref; +} + +/************************************************************************* + * put_cached_modref + * + * Returns the new argument regardless of whether it was actually cached or not. + */ +static WINE_MODREF *put_cached_modref( WINE_MODREF *old, WINE_MODREF *new ) +{ + InterlockedCompareExchangePointer( (void **)&cached_modref, new, old ); + return new; +} + #define RTL_UNLOAD_EVENT_TRACE_NUMBER 64 typedef struct _RTL_UNLOAD_EVENT_TRACE @@ -753,17 +775,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 put_cached_modref( cached, CONTAINING_RECORD(mod, WINE_MODREF, ldr) ); } return NULL; } @@ -777,23 +800,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 put_cached_modref( cached, CONTAINING_RECORD(mod, WINE_MODREF, ldr) ); } return NULL; } @@ -807,6 +828,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 +836,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 put_cached_modref( cached, CONTAINING_RECORD(mod, WINE_MODREF, ldr) ); } return NULL; } @@ -839,9 +858,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 +870,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 put_cached_modref( cached, wm ); } return NULL; } @@ -3821,7 +3838,7 @@ 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; + if (get_cached_modref() == wm) put_cached_modref( wm, NULL ); RtlFreeUnicodeString( &wm->ldr.FullDllName ); RtlFreeHeap( GetProcessHeap(), 0, wm->deps ); RtlFreeHeap( GetProcessHeap(), 0, 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=99229 Your paranoid android. === debiant2 (32 bit report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit Chinese:China report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (64 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events.
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 48 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index a8222ec30df..766a3108425 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -172,6 +172,7 @@ static RTL_CONDITION_VARIABLE exclusive_lock_released_cv; static unsigned int lock_exclusive_recursion_count[MAX_DOWNGRADE_RECURSION_COUNT]; static unsigned int lock_exclusive_recursion_stack; +static RTL_SRWLOCK loader_module_info_lock; static CRITICAL_SECTION dlldir_section; static CRITICAL_SECTION_DEBUG dlldir_critsect_debug = @@ -472,6 +473,29 @@ static void lock_loader_restore_exclusive(void) locked_exclusive = TRUE; } +/************************************************************************* + * lock_module_info + * + * Lock module info data for performing modifications not resulting in module + * unload with only shared loader internal lock held. + * The lock is supposed to be very short lived. Only calls to ntdll loader + * functions which do not lock anything themselves are allowed with this lock held. + */ +static void lock_module_info(void) +{ + RtlAcquireSRWLockExclusive( &loader_module_info_lock ); +} + +/************************************************************************* + * unlock_module_info + * + * Unlock module info data. + */ +static void unlock_module_info(void) +{ + RtlReleaseSRWLockExclusive( &loader_module_info_lock ); +} + /************************************************************************* * get_cached_modref * @@ -3850,7 +3874,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) { @@ -3881,7 +3905,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 ) { @@ -3913,17 +3937,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 ))) + { + lock_module_info(); + 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. */ + unlock_module_info(); + } + else retv = STATUS_DLL_NOT_FOUND; + unlock_loader(); + if (!need_exclusive) return retv; + lock_loader_exclusive(); free_lib_count++; @@ -4170,6 +4211,7 @@ void WINAPI LdrInitializeThunk( CONTEXT *context, ULONG_PTR unknown2, ULONG_PTR TRACE( "Initializing loader locks.\n" ); RtlInitializeSRWLock( &ldr_data_srw_lock ); RtlInitializeSRWLock( &loader_srw_lock ); + RtlInitializeSRWLock( &loader_module_info_lock ); RtlInitializeConditionVariable( &exclusive_lock_released_cv ); } -- 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=99230 Your paranoid android. === debiant2 (32 bit report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit Chinese:China report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (64 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events.
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 766a3108425..927de8e25f5 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -496,6 +496,16 @@ static void unlock_module_info(void) RtlReleaseSRWLockExclusive( &loader_module_info_lock ); } +/************************************************************************* + * 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 * @@ -3431,14 +3441,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++; + lock_module_info(); + /* 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; + unlock_module_info(); TRACE( "(%s) ldr.LoadCount: %d\n", debugstr_w(wm->ldr.BaseDllName.Buffer), wm->ldr.LoadCount ); } else ret = STATUS_INVALID_PARAMETER; -- 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=99231 Your paranoid android. === debiant2 (32 bit report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit Chinese:China report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (64 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events.
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 927de8e25f5..99d5c08b002 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -3396,11 +3396,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) + { + lock_module_info(); + if (!is_thread_exclusive() && !wm->ldr.LoadCount) + { + unlock_module_info(); + wait_for_exclusive_lock_release(); + goto retry; + } + unlock_module_info(); + *base = wm->ldr.DllBase; + } else { if (status == STATUS_SUCCESS) NtClose( mapping ); @@ -3411,9 +3423,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
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=99232 Your paranoid android. === debiant2 (32 bit report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit Chinese:China report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (64 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events.
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 99d5c08b002..37323608c3a 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -3321,6 +3321,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
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=99233 Your paranoid android. === debiant2 (32 bit report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit Chinese:China report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (64 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events.
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 37323608c3a..436491921c6 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -2069,22 +2069,22 @@ 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 ) @@ -2092,10 +2092,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
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=99234 Your paranoid android. === debiant2 (32 bit report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit Chinese:China report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (64 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events.
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 62 +++++++++++++++++++++++++++++++++------------ 1 file changed, 46 insertions(+), 16 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 436491921c6..77d7a292569 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 ) @@ -937,7 +937,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; @@ -960,6 +960,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)) @@ -991,9 +999,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) @@ -1016,11 +1024,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 ); @@ -1033,7 +1043,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)) { @@ -1079,7 +1089,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 ); @@ -1090,12 +1100,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 ); } @@ -1229,7 +1239,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) ); @@ -1245,7 +1255,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 ); @@ -2078,24 +2088,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; + + lock_module_info(); + if (!is_thread_exclusive() && (!wm->ldr.LoadCount || (wm->ldr.Flags & LDR_LOAD_IN_PROGRESS))) + { + unlock_module_info(); + if (!wm->ldr.LoadCount) return STATUS_INVALID_PARAMETER; + wait_for_exclusive_lock_release(); + goto retry; + } + unlock_module_info(); 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; } /****************************************************************** @@ -2106,9 +2129,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=99235 Your paranoid android. === debiant2 (32 bit report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit Chinese:China report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (64 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events.
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 77d7a292569..2ef49691f46 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -1913,13 +1913,16 @@ NTSTATUS WINAPI LdrDisableThreadCalloutsForDll(HMODULE hModule) WINE_MODREF *wm; NTSTATUS ret = STATUS_SUCCESS; - lock_loader_exclusive(); + lock_loader_shared(); wm = get_modref( hModule ); + + lock_module_info(); if (!wm || wm->ldr.TlsIndex != -1) ret = STATUS_DLL_NOT_FOUND; else wm->ldr.Flags |= LDR_NO_DLL_CALLS; + unlock_module_info(); unlock_loader(); -- 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=99236 Your paranoid android. === debiant2 (32 bit report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit Chinese:China report) === ntdll: change.c:241: Test failed: should be ready info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (64 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events.
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 2ef49691f46..e4c090aa70e 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -3400,7 +3400,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=99237 Your paranoid android. === debiant2 (32 bit report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit Chinese:China report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (64 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events.
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/loader.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index e4c090aa70e..116df9b1453 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -2138,6 +2138,12 @@ NTSTATUS WINAPI LdrGetProcedureAddress( HMODULE module, const ANSI_STRING *name, if (ret != STATUS_NOT_FOUND) return ret; + if (RtlIsCriticalSectionLocked( &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(); @@ -3368,6 +3374,11 @@ NTSTATUS WINAPI DECLSPEC_HOTPATCH LdrLoadDll(LPCWSTR path_name, DWORD flags, if (!LdrGetDllHandleEx( 0, path_name, NULL, libname, hModule )) return STATUS_SUCCESS; + if (RtlIsCriticalSectionLocked( &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 ); @@ -4044,6 +4055,12 @@ NTSTATUS WINAPI LdrUnloadDll( HMODULE hModule ) unlock_loader(); if (!need_exclusive) return retv; + if (RtlIsCriticalSectionLocked( &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=99238 Your paranoid android. === debiant2 (32 bit report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit Chinese:China report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (32 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events. === debiant2 (64 bit WoW report) === ntdll: info.c:2333: Test failed: Expected less than 100 debug events. info.c:2333: Test failed: Expected less than 100 debug events.
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 | 579 +++++++++++++++++++++++++++ dlls/kernel32/tests/locking_dll.c | 50 +++ dlls/kernel32/tests/locking_dll.spec | 1 + 5 files changed, 633 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 4f1b11338a6..df530055db4 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); @@ -88,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) @@ -4036,6 +4042,574 @@ static void test_Wow64Transition(void) debugstr_wn(name->SectionFileName.Buffer, name->SectionFileName.Length / sizeof(WCHAR))); } +static BOOL test_loader_lock_repeat_lock; +static DWORD test_loader_lock_expected_wait_result; +static unsigned int test_loader_notification_count; +static unsigned int test_loader_expected_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_test_failed; +static volatile LONG test_loader_lock_timeout_count; + +#define BLOCKING_TESTS_ENABLED 1 + +static void CALLBACK test_loader_lock_module_enum_locking_callback(LDR_DATA_TABLE_ENTRY *mod, + void *context, BOOLEAN *stop) +{ + DWORD *result = context; + + SetEvent(lock_ready_event); + *result = WaitForSingleObject(next_test_event, 3000); + *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) +{ + DWORD *result = 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()); + + 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, "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); + *result = WaitForSingleObject(next_test_event, 3000); +} + +static DWORD WINAPI test_loader_lock_thread(void *param) +{ + static const WCHAR env_var[] = L"test_wait_reason_value"; + void (WINAPI *p_set_lock_result_addr)( DWORD *addr ); + unsigned int test, iter; + ULONG_PTR magic; + NTSTATUS status; + HMODULE hmodule; + void *cookie; + DWORD result; + WCHAR str[8]; + BOOL bret; + + WaitForSingleObject(next_test_event, INFINITE); + + test = 1; + /* 1. Test with loader lock held. */ + iter = 0; + 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 (!iter && result == WAIT_TIMEOUT) + { + /* First test timed out, likely the old loader. */ + test_loader_lock_test_failed = TRUE; + status = pLdrUnlockLoaderLock(0, magic); + ok(!status, "Got unexpected status %#x.\n", status); + SetEvent(lock_ready_event); + return 0; + } + + ok(result == test_loader_lock_expected_wait_result, + "Got unexpected wait result %#x, expected %#x, test %u.%u.\n", + result, test_loader_lock_expected_wait_result, test, iter); + + todo_wine_if(test && iter == 1) + ok(test_loader_notification_count == test_loader_expected_notification_count, + "Got unexpected notification count %u, expected %u, test %u.%u.\n", + test_loader_notification_count, test_loader_expected_notification_count, test, iter); + + status = pLdrUnlockLoaderLock(0, magic); + ok(!status, "Got unexpected status %#x.\n", status); + + if (result == WAIT_TIMEOUT) + WaitForSingleObject(next_test_event, INFINITE); + ++iter; + } while (test_loader_lock_repeat_lock); + + SetEvent(lock_ready_event); + WaitForSingleObject(next_test_event, INFINITE); + /* 2. Test with the thread blocked in DLL entry point during process attach. */ + swprintf(str, ARRAY_SIZE(str), L"%u", DLL_PROCESS_ATTACH); + bret = SetEnvironmentVariableW(env_var, str); + ok(bret, "SetEnvironmentVariableW failed.\n"); + ++test; + iter = 0; + do + { + hmodule = LoadLibraryA("lock.dll"); + ok(!!hmodule, "LoadLibrary failed, err %u.\n", GetLastError()); + p_set_lock_result_addr = (void *)GetProcAddress(hmodule, "set_lock_result_addr"); + p_set_lock_result_addr(&result); + + if (result == WAIT_TIMEOUT) + InterlockedIncrement(&test_loader_lock_timeout_count); + + bret = FreeLibrary(hmodule); + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + + ok(result == test_loader_lock_expected_wait_result, + "Got unexpected wait result %#x, expected %#x, test %u.%u.\n", + result, test_loader_lock_expected_wait_result, test, iter); + + if (result == WAIT_TIMEOUT) + WaitForSingleObject(next_test_event, INFINITE); + ++iter; + } while (test_loader_lock_repeat_lock); + + SetEvent(lock_ready_event); + WaitForSingleObject(next_test_event, INFINITE); + + /* 3. 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"); + ++test; + iter = 0; + do + { + lock_dll_handle = hmodule = LoadLibraryA("lock.dll"); + ok(!!hmodule, "LoadLibrary failed, err %u.\n", GetLastError()); + p_set_lock_result_addr = (void *)GetProcAddress(hmodule, "set_lock_result_addr"); + ok(!!p_set_lock_result_addr, "Got NULL p_set_lock_result_addr.\n"); + p_set_lock_result_addr(&result); + bret = FreeLibrary(hmodule); + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + + lock_dll_handle = NULL; + + if (result == WAIT_TIMEOUT) + InterlockedIncrement(&test_loader_lock_timeout_count); + + ok(result == test_loader_lock_expected_wait_result, + "Got unexpected wait result %#x, expected %#x, test %u.%u.\n", + result, test_loader_lock_expected_wait_result, test, iter); + + if (result == WAIT_TIMEOUT) + WaitForSingleObject(next_test_event, INFINITE); + ++iter; + } while (test_loader_lock_repeat_lock); + + SetEvent(lock_ready_event); + WaitForSingleObject(next_test_event, INFINITE); + + /* 4. Test with the thread blocked in LdrEnumerateLoadedModules callback. */ + ++test; + iter = 0; + do + { + pLdrEnumerateLoadedModules(NULL, test_loader_lock_module_enum_locking_callback, &result); + + if (result == WAIT_TIMEOUT) + InterlockedIncrement(&test_loader_lock_timeout_count); + + ok(result == test_loader_lock_expected_wait_result, + "Got unexpected wait result %#x, expected %#x, test %u.%u.\n", + result, test_loader_lock_expected_wait_result, test, iter); + ok(test_loader_notification_count == test_loader_expected_notification_count, + "Got unexpected notification count %u, expected %u, test %u.%u.\n", + test_loader_notification_count, test_loader_expected_notification_count, test, iter); + + if (result == WAIT_TIMEOUT) + WaitForSingleObject(next_test_event, INFINITE); + ++iter; + } while (test_loader_lock_repeat_lock); + + 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, &result, &cookie); + /* lock.dll will return FALSE from the DLL entry point. */ + bret = SetEnvironmentVariableW(env_var, L"-1"); + ok(bret, "SetEnvironmentVariableW failed.\n"); + + ++test; + iter = 0; + 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 (result == WAIT_TIMEOUT) + InterlockedIncrement(&test_loader_lock_timeout_count); + + ok(result == test_loader_lock_expected_wait_result, + "Got unexpected wait result %#x, expected %#x, test %u.%u.\n", + result, test_loader_lock_expected_wait_result, test, iter); + ok(test_loader_notification_count == test_loader_expected_notification_count, + "Got unexpected notification count %u, expected %u, test %u.%u.\n", + test_loader_notification_count, test_loader_expected_notification_count, test, iter); + + if (result == WAIT_TIMEOUT) + WaitForSingleObject(next_test_event, INFINITE); + ++iter; + } while (test_loader_lock_repeat_lock); + LdrUnregisterDllNotification( cookie ); + SetEvent(lock_ready_event); + return 0; +} + +static void CALLBACK test_loader_lock_ldr_notify(ULONG reason, LDR_DLL_NOTIFICATION_DATA *data, void *context) +{ + if (!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; +} + +static void test_loader_lock(void) +{ +#define CHECK ok(!test_loader_lock_timeout_count, "Got timeout count %d.\n", test_loader_lock_timeout_count) + 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; + 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"); + + 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"); + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + test_loader_lock_expected_wait_result = WAIT_OBJECT_0; + + status = LdrRegisterDllNotification(0, test_loader_lock_ldr_notify, NULL, &cookie); + ok(!status, "Got unexpected status %#x.\n", status); + CHECK; + 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; + + bret = FreeLibrary(hmodule); + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + CHECK; + + ok(!test_loader_notification_count, "Got unexpected test_loader_notification_count %u.\n", test_loader_notification_count); + + if (BLOCKING_TESTS_ENABLED) + { + CHECK; + test_loader_lock_repeat_lock = TRUE; + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + + if (test_loader_lock_test_failed) + { + win_skip("Old loader, tests skipped.\n"); + goto done; + } + + /* 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_expected_notification_count = 1; + test_loader_lock_expected_wait_result = WAIT_TIMEOUT; + hmodule = LoadLibraryW(not_loaded_dll_name); + ok(!!hmodule, "LoadLibrary failed, err %u.\n", GetLastError()); + + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + + test_loader_notification_count = 0; + test_loader_expected_notification_count = 0; + bret = FreeLibrary(hmodule); + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + + ok(test_loader_notification_count == 1, "Got unexpected notification count %u, expected %u.\n", + test_loader_notification_count, 1); + test_loader_notification_count = 0; + test_loader_expected_notification_count = 0; + + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + pLdrEnumerateLoadedModules(NULL, test_loader_lock_enum_callback, NULL); + + test_loader_lock_repeat_lock = FALSE; + } + + LdrUnregisterDllNotification( cookie ); + + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + + if (test_loader_lock_test_failed) + { + win_skip("Old loader, tests skipped.\n"); + goto done; + } + + /* 2. Test with the thread blocked in DLL entry point during process attach. */ + trace("Test 2.\n"); + SetEvent(next_test_event); + + WaitForSingleObject(lock_ready_event, INFINITE); + test_loader_lock_expected_wait_result = WAIT_OBJECT_0; + + status = LdrRegisterDllNotification(0, test_loader_lock_ldr_notify, NULL, &cookie); + ok(!status, "Got unexpected status %#x.\n", status); + CHECK; + + bret = GetModuleHandleExW(0, preloaded_dll_name, &hmodule); + CHECK; + 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; + ok(!!proc, "GetProcAddress failed.\n"); + + bret = FreeLibrary(hmodule); + CHECK; + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + + hmodule = GetModuleHandleA("lock.dll"); + CHECK; + ok(!!hmodule, "GetModuleHandleA failed, err %u.\n", GetLastError()); + + bret = GetModuleHandleExW(0, L"lock.dll", &hmodule); + CHECK; + ok(bret, "GetModuleHandleEx failed, err %u.\n", GetLastError()); + + lock_dll_handle = hmodule; + + status = LdrAddRefDll(0, hmodule); + CHECK; + ok(!status, "Got unexpected status %#x.\n", status); + + bret = FreeLibrary(hmodule); + CHECK; + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + + bret = FreeLibrary(hmodule); + CHECK; + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + + hmodule = LoadLibraryW(preloaded_dll_name); + CHECK; + ok(!!hmodule, "LoadLibrary failed, err %u.\n", GetLastError()); + + bret = FreeLibrary(hmodule); + CHECK; + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + + if (BLOCKING_TESTS_ENABLED) + { + test_loader_lock_repeat_lock = TRUE; + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + + test_loader_lock_expected_wait_result = WAIT_TIMEOUT; + + ok(!!lock_dll_handle, "Got NULL lock_dll_handle.\n"); + proc = GetProcAddress(lock_dll_handle, "unknown"); + ok(!proc, "GetProcAddress succeeded.\n"); + + test_loader_lock_repeat_lock = FALSE; + } + + LdrUnregisterDllNotification( cookie ); + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + /* 3. Test with the thread blocked in DLL entry point during process detach. */ + trace("Test 3.\n"); + + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + test_loader_lock_timeout_count = 0; + test_loader_lock_expected_wait_result = WAIT_OBJECT_0; + + status = LdrRegisterDllNotification(0, test_loader_lock_ldr_notify, NULL, &cookie); + ok(!status, "Got unexpected status %#x.\n", status); + CHECK; + + 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; + + proc = GetProcAddress(hmodule, "timeGetTime"); + ok(!!proc, "GetProcAddress failed.\n"); + CHECK; + + bret = FreeLibrary(hmodule); + ok(bret, "FreeLibrary failed, err %u.\n", GetLastError()); + CHECK; + + ok(!!lock_dll_handle, "Got NULL lock_dll_handle.\n"); + + proc = GetProcAddress(lock_dll_handle, "set_lock_result_addr"); + CHECK; + + ok(!proc, "GetProcAddress failed, err %u.\n", GetLastError()); + ok(GetLastError() == ERROR_INVALID_PARAMETER, "Got unexpected error %u.\n", GetLastError()); + + status = LdrAddRefDll(0, lock_dll_handle); + CHECK; + ok(status == STATUS_DLL_NOT_FOUND, "Got unexpected status %#x.\n", status); + + if (BLOCKING_TESTS_ENABLED) + { + test_loader_lock_repeat_lock = TRUE; + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + + test_loader_lock_expected_wait_result = WAIT_TIMEOUT; + hmodule = GetModuleHandleW(L"lock.dll"); + ok(!hmodule, "GetModuleHandleW succeeded.\n", GetLastError()); + ok(GetLastError() == ERROR_MOD_NOT_FOUND, "Got unexpected error %u.\n", GetLastError()); + test_loader_lock_repeat_lock = FALSE; + } + + LdrUnregisterDllNotification( cookie ); + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + + /* 4. Test with the thread blocked in LdrEnumerateLoadedModules callback. */ + trace("Test 4.\n"); + + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + test_loader_lock_timeout_count = 0; + test_loader_lock_expected_wait_result = WAIT_OBJECT_0; + + status = LdrRegisterDllNotification(0, test_loader_lock_ldr_notify, NULL, &cookie); + ok(!status, "Got unexpected status %#x.\n", status); + CHECK; + + hmodule = GetModuleHandleW(preloaded_dll_name); + ok(!!hmodule, "LoadLibrary failed, err %u.\n", GetLastError()); + CHECK; + + if (BLOCKING_TESTS_ENABLED) + { + test_loader_lock_repeat_lock = TRUE; + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + + test_loader_lock_expected_wait_result = WAIT_TIMEOUT; + + 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); + + test_loader_lock_repeat_lock = FALSE; + } + + LdrUnregisterDllNotification( cookie ); + + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + + /* 5. Test with the thread blocked in LDR notification callback. */ + trace("Test 5.\n"); + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + + test_loader_lock_timeout_count = 0; + test_loader_lock_expected_wait_result = WAIT_OBJECT_0; + + hmodule = GetModuleHandleW(preloaded_dll_name); + ok(!!hmodule, "LoadLibrary failed, err %u.\n", GetLastError()); + CHECK; + + if (BLOCKING_TESTS_ENABLED) + { + test_loader_lock_repeat_lock = TRUE; + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + test_loader_lock_expected_wait_result = WAIT_TIMEOUT; + + status = LdrRegisterDllNotification(0, test_loader_lock_ldr_notify, NULL, &cookie); + ok(!status, "Got unexpected status %#x.\n", status); + LdrUnregisterDllNotification( cookie ); + + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + + /* 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); + + test_loader_lock_repeat_lock = FALSE; + } + + SetEvent(next_test_event); + WaitForSingleObject(lock_ready_event, INFINITE); + + + WaitForSingleObject(thread, INFINITE); + CloseHandle(thread); + +done: + DeleteFileA("lock.dll"); + FreeLibrary(hmodule_preloaded); + CloseHandle(lock_ready_event); + CloseHandle(next_test_event); +} + START_TEST(loader) { int argc; @@ -4060,6 +4634,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"); @@ -4099,6 +4674,7 @@ START_TEST(loader) return; } +if(0){ test_filenames(); test_ResolveDelayLoadedAPI(); test_ImportDescriptors(); @@ -4113,6 +4689,9 @@ START_TEST(loader) test_dll_file( "advapi32.dll" ); test_dll_file( "user32.dll" ); test_Wow64Transition(); +} + test_loader_lock(); +return; /* loader test must be last, it can corrupt the internal loader state on Windows */ test_Loader(); } diff --git a/dlls/kernel32/tests/locking_dll.c b/dlls/kernel32/tests/locking_dll.c new file mode 100644 index 00000000000..31aff05fe84 --- /dev/null +++ b/dlls/kernel32/tests/locking_dll.c @@ -0,0 +1,50 @@ +#include <stdarg.h> +#include <stdlib.h> +#include "ntstatus.h" +#define WIN32_NO_STATUS +#include "windef.h" +#include "winbase.h" +#include "winternl.h" + +static DWORD last_lock_result, *lock_result_addr; + +BOOL WINAPI DllMain( HINSTANCE instance, DWORD reason, LPVOID reserved ) +{ + HANDLE lock_ready_event, next_test_event; + DWORD wait_reason; + NTSTATUS status; + char str[8]; + + GetEnvironmentVariableA("test_wait_reason_value", str, sizeof(str)); + wait_reason = atoi(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"); + + SetEvent( lock_ready_event ); + last_lock_result = WaitForSingleObject(next_test_event, 2000); + + if (0) + { + if (reason == DLL_PROCESS_DETACH) + { + status = LdrAddRefDll(0, instance); + if (status) last_lock_result = ~0u; + status = LdrAddRefDll(LDR_ADDREF_DLL_PIN, instance); + if (status != STATUS_UNSUCCESSFUL) last_lock_result = ~0u - 1; + } + } + if (lock_result_addr) *lock_result_addr = last_lock_result; + + CloseHandle(lock_ready_event); + CloseHandle(next_test_event); + return TRUE; +} + +void WINAPI set_lock_result_addr( DWORD *addr ) +{ + lock_result_addr = addr; + *addr = last_lock_result; +} diff --git a/dlls/kernel32/tests/locking_dll.spec b/dlls/kernel32/tests/locking_dll.spec new file mode 100644 index 00000000000..327515590c3 --- /dev/null +++ b/dlls/kernel32/tests/locking_dll.spec @@ -0,0 +1 @@ +@ stdcall set_lock_result_addr(ptr) -- 2.31.1
participants (2)
-
Marvin -
Paul Gofman