[PATCH 1/4] kernelbase: Don't use loader lock for loading libraries as data files.
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/kernelbase/loader.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/dlls/kernelbase/loader.c b/dlls/kernelbase/loader.c index da139e91176..53c6750e668 100644 --- a/dlls/kernelbase/loader.c +++ b/dlls/kernelbase/loader.c @@ -48,6 +48,14 @@ struct exclusive_datafile }; static struct list exclusive_datafile_list = LIST_INIT( exclusive_datafile_list ); +static CRITICAL_SECTION exclusive_datafile_list_section; +static CRITICAL_SECTION_DEBUG critsect_debug = +{ + 0, 0, &exclusive_datafile_list_section, + { &critsect_debug.ProcessLocksList, &critsect_debug.ProcessLocksList }, + 0, 0, { (DWORD_PTR)(__FILE__ ": exclusive_datafile_list_section") } +}; +static CRITICAL_SECTION exclusive_datafile_list_section = { &critsect_debug, -1, 0, 0, 0, 0 }; /*********************************************************************** * Modules @@ -120,7 +128,9 @@ static BOOL load_library_as_datafile( LPCWSTR load_path, DWORD flags, LPCWSTR na if (!datafile) goto failed; datafile->module = *mod_ret; datafile->file = file; + RtlEnterCriticalSection( &exclusive_datafile_list_section ); list_add_head( &exclusive_datafile_list, &datafile->entry ); + RtlLeaveCriticalSection( &exclusive_datafile_list_section ); TRACE( "delaying close %p for module %p\n", datafile->file, datafile->module ); return TRUE; } @@ -154,14 +164,8 @@ static HMODULE load_library( const UNICODE_STRING *libname, DWORD flags ) if (flags & (LOAD_LIBRARY_AS_DATAFILE | LOAD_LIBRARY_AS_DATAFILE_EXCLUSIVE | LOAD_LIBRARY_AS_IMAGE_RESOURCE)) { - ULONG_PTR magic; - - LdrLockLoaderLock( 0, NULL, &magic ); - if (!LdrGetDllHandle( load_path, flags, libname, &module )) - LdrAddRefDll( 0, module ); - else + if (LdrGetDllHandleEx( 0, load_path, NULL, libname, &module )) load_library_as_datafile( load_path, flags, libname->Buffer, &module ); - LdrUnlockLoaderLock( 0, magic ); } else { @@ -242,9 +246,8 @@ BOOL WINAPI DECLSPEC_HOTPATCH FreeLibrary( HINSTANCE module ) if ((ULONG_PTR)module & 1) { struct exclusive_datafile *file; - ULONG_PTR magic; - LdrLockLoaderLock( 0, NULL, &magic ); + RtlEnterCriticalSection( &exclusive_datafile_list_section ); LIST_FOR_EACH_ENTRY( file, &exclusive_datafile_list, struct exclusive_datafile, entry ) { if (file->module != module) continue; @@ -254,7 +257,7 @@ BOOL WINAPI DECLSPEC_HOTPATCH FreeLibrary( HINSTANCE module ) HeapFree( GetProcessHeap(), 0, file ); break; } - LdrUnlockLoaderLock( 0, magic ); + RtlLeaveCriticalSection( &exclusive_datafile_list_section ); } return UnmapViewOfFile( ptr ); } -- 2.31.1
From: Alex Henrie <alexhenrie24(a)gmail.com> Changes from the original patch: - compare full exe path instead of extension; - don't use shlwapi; - rename ntdll_path to expected_path; - move test to kernel32/tests/module.c; - change formatting. Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/kernel32/tests/module.c | 59 ++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/dlls/kernel32/tests/module.c b/dlls/kernel32/tests/module.c index bfa389ac9ab..eac4f478293 100644 --- a/dlls/kernel32/tests/module.c +++ b/dlls/kernel32/tests/module.c @@ -42,6 +42,7 @@ static NTSTATUS (WINAPI *pLdrSetDllDirectory)(UNICODE_STRING*); static NTSTATUS (WINAPI *pLdrGetDllHandle)( LPCWSTR load_path, ULONG flags, const UNICODE_STRING *name, HMODULE *base ); static NTSTATUS (WINAPI *pLdrGetDllHandleEx)( ULONG flags, LPCWSTR load_path, ULONG *dll_characteristics, const UNICODE_STRING *name, HMODULE *base ); +static NTSTATUS (WINAPI *pLdrGetDllFullName)( HMODULE module, UNICODE_STRING *name ); static BOOL is_unicode_enabled = TRUE; @@ -831,6 +832,7 @@ static void init_pointers(void) MAKEFUNC(LdrSetDllDirectory); MAKEFUNC(LdrGetDllHandle); MAKEFUNC(LdrGetDllHandleEx); + MAKEFUNC(LdrGetDllFullName); #undef MAKEFUNC /* before Windows 7 this was not exported in kernel32 */ @@ -1266,6 +1268,62 @@ static void test_LdrGetDllHandleEx(void) winetest_pop_context(); } +static void test_LdrGetDllFullName(void) +{ + WCHAR expected_path[MAX_PATH], path_buffer[MAX_PATH]; + UNICODE_STRING path = {0, 0, path_buffer}; + WCHAR expected_terminator; + NTSTATUS status; + HMODULE ntdll; + + if (!pLdrGetDllFullName) + { + skip( "LdrGetDllFullName not available.\n" ); + return; + } + + if (0) /* crashes on Windows */ + pLdrGetDllFullName( ntdll, NULL ); + + ntdll = GetModuleHandleW( L"ntdll.dll" ); + + memset( path_buffer, 0x23, sizeof(path_buffer) ); + + status = pLdrGetDllFullName( ntdll, &path ); + ok( status == STATUS_BUFFER_TOO_SMALL, "Got unexpected status %#x.\n", status ); + ok( path.Length == 0, "Expected length 0, got %d.\n", path.Length ); + ok( path_buffer[0] == 0x2323, "Expected 0x2323, got 0x%x.\n", path_buffer[0] ); + + GetSystemDirectoryW( expected_path, ARRAY_SIZE(expected_path) ); + path.MaximumLength = 5; /* odd numbers produce partially copied characters */ + + status = pLdrGetDllFullName( ntdll, &path ); + ok( status == STATUS_BUFFER_TOO_SMALL, "Got unexpected status %#x.\n", status ); + ok( path.Length == path.MaximumLength, "Expected length %u, got %u.\n", path.MaximumLength, path.Length ); + expected_terminator = 0x2300 | (expected_path[path.MaximumLength / sizeof(WCHAR)] & 0xFF); + ok( path_buffer[path.MaximumLength / sizeof(WCHAR)] == expected_terminator, + "Expected 0x%x, got 0x%x.\n", expected_terminator, path_buffer[path.MaximumLength / 2] ); + path_buffer[path.MaximumLength / sizeof(WCHAR)] = 0; + expected_path[path.MaximumLength / sizeof(WCHAR)] = 0; + ok( lstrcmpW(path_buffer, expected_path) == 0, "Expected %s, got %s.\n", + wine_dbgstr_w(expected_path), wine_dbgstr_w(path_buffer) ); + + GetSystemDirectoryW( expected_path, ARRAY_SIZE(expected_path) ); + lstrcatW( expected_path, L"\\ntdll.dll" ); + path.MaximumLength = sizeof(path_buffer); + + status = pLdrGetDllFullName( ntdll, &path ); + ok( status == STATUS_SUCCESS, "Got unexpected status %#x.\n", status ); + ok( !lstrcmpiW(path_buffer, expected_path), "Expected %s, got %s\n", + wine_dbgstr_w(expected_path), wine_dbgstr_w(path_buffer) ); + + status = pLdrGetDllFullName( NULL, &path ); + ok( status == STATUS_SUCCESS, "Got unexpected status %#x.\n", status ); + GetModuleFileNameW( NULL, expected_path, ARRAY_SIZE(expected_path) ); + ok( !lstrcmpiW(path_buffer, expected_path), "Expected %s, got %s.\n", + wine_dbgstr_w(expected_path), wine_dbgstr_w(path_buffer) ); +} + START_TEST(module) { WCHAR filenameW[MAX_PATH]; @@ -1299,4 +1357,5 @@ START_TEST(module) test_AddDllDirectory(); test_SetDefaultDllDirectories(); test_LdrGetDllHandleEx(); + test_LdrGetDllFullName(); } -- 2.31.1
On Mon, Sep 27, 2021 at 5:59 PM Paul Gofman <pgofman(a)codeweavers.com> wrote:
From: Alex Henrie <alexhenrie24(a)gmail.com>
Changes from the original patch: - compare full exe path instead of extension; - don't use shlwapi; - rename ntdll_path to expected_path; - move test to kernel32/tests/module.c; - change formatting.
"Based on a patch by Alex Henrie." would be more appropriate than "From: Alex Henrie" in this patch and the next. I abandoned my work on LdrGetDllFullName once I realized that I didn't really need it. Thanks for picking it up again! -Alex
From: Alex Henrie <alexhenrie24(a)gmail.com> Changes from the original patch: - Access module data inside loader lock. - Add function prototype to winternl.h. Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/kernel32/tests/module.c | 2 +- dlls/ntdll/loader.c | 26 ++++++++++++++++++++++++++ dlls/ntdll/ntdll.spec | 1 + include/winternl.h | 1 + 4 files changed, 29 insertions(+), 1 deletion(-) diff --git a/dlls/kernel32/tests/module.c b/dlls/kernel32/tests/module.c index eac4f478293..2487b9d65ab 100644 --- a/dlls/kernel32/tests/module.c +++ b/dlls/kernel32/tests/module.c @@ -1278,7 +1278,7 @@ static void test_LdrGetDllFullName(void) if (!pLdrGetDllFullName) { - skip( "LdrGetDllFullName not available.\n" ); + win_skip( "LdrGetDllFullName not available.\n" ); return; } diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 3e3f991eba2..66886ad349a 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -2995,6 +2995,32 @@ NTSTATUS WINAPI DECLSPEC_HOTPATCH LdrLoadDll(LPCWSTR path_name, DWORD flags, } +/****************************************************************** + * LdrGetDllFullName (NTDLL.@) + */ +NTSTATUS WINAPI LdrGetDllFullName( HMODULE module, UNICODE_STRING *name ) +{ + WINE_MODREF *wm; + NTSTATUS status; + + TRACE( "module %p, name %p.\n", module, name ); + + if (!module) module = NtCurrentTeb()->Peb->ImageBaseAddress; + + RtlEnterCriticalSection( &loader_section ); + wm = get_modref( module ); + if (wm) + { + RtlCopyUnicodeString( name, &wm->ldr.FullDllName ); + 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 ); + + return status; +} + + /****************************************************************** * LdrGetDllHandleEx (NTDLL.@) */ diff --git a/dlls/ntdll/ntdll.spec b/dlls/ntdll/ntdll.spec index e7ac08e94ba..13e65f65139 100644 --- a/dlls/ntdll/ntdll.spec +++ b/dlls/ntdll/ntdll.spec @@ -87,6 +87,7 @@ @ stdcall LdrFindResource_U(long ptr long ptr) @ stub LdrFlushAlternateResourceModules @ stdcall LdrGetDllDirectory(ptr) +@ stdcall LdrGetDllFullName(long ptr) @ stdcall LdrGetDllHandle(wstr long ptr ptr) @ stdcall LdrGetDllHandleEx(long ptr ptr ptr ptr) # @ stub LdrGetDllHandleEx diff --git a/include/winternl.h b/include/winternl.h index 94d781ea412..b6f93c116d8 100644 --- a/include/winternl.h +++ b/include/winternl.h @@ -3791,6 +3791,7 @@ NTSYSAPI NTSTATUS WINAPI LdrGetDllDirectory(UNICODE_STRING*); NTSYSAPI NTSTATUS WINAPI LdrGetDllHandle(LPCWSTR, ULONG, const UNICODE_STRING*, HMODULE*); NTSYSAPI NTSTATUS WINAPI LdrGetDllHandleEx(ULONG, LPCWSTR, ULONG *, const UNICODE_STRING*, HMODULE*); NTSYSAPI NTSTATUS WINAPI LdrGetDllPath(PCWSTR,ULONG,PWSTR*,PWSTR*); +NTSYSAPI NTSTATUS WINAPI LdrGetDllFullName(HMODULE, UNICODE_STRING*); NTSYSAPI NTSTATUS WINAPI LdrGetProcedureAddress(HMODULE, const ANSI_STRING*, ULONG, void**); NTSYSAPI NTSTATUS WINAPI LdrLoadDll(LPCWSTR, DWORD, const UNICODE_STRING*, HMODULE*); NTSYSAPI NTSTATUS WINAPI LdrLockLoaderLock(ULONG,ULONG*,ULONG_PTR*); -- 2.31.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/kernelbase/loader.c | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/dlls/kernelbase/loader.c b/dlls/kernelbase/loader.c index 53c6750e668..145d721bc26 100644 --- a/dlls/kernelbase/loader.c +++ b/dlls/kernelbase/loader.c @@ -298,9 +298,9 @@ DWORD WINAPI DECLSPEC_HOTPATCH GetModuleFileNameA( HMODULE module, LPSTR filenam DWORD WINAPI DECLSPEC_HOTPATCH GetModuleFileNameW( HMODULE module, LPWSTR filename, DWORD size ) { ULONG len = 0; - ULONG_PTR magic; - LDR_DATA_TABLE_ENTRY *pldr; WIN16_SUBSYSTEM_TIB *win16_tib; + UNICODE_STRING name; + NTSTATUS status; if (!module && ((win16_tib = NtCurrentTeb()->Tib.SubSystemTib)) && win16_tib->exe_name) { @@ -310,22 +310,11 @@ DWORD WINAPI DECLSPEC_HOTPATCH GetModuleFileNameW( HMODULE module, LPWSTR filena goto done; } - LdrLockLoaderLock( 0, NULL, &magic ); - - if (!module) module = NtCurrentTeb()->Peb->ImageBaseAddress; - if (set_ntstatus( LdrFindEntryForAddress( module, &pldr ))) - { - len = min( size, pldr->FullDllName.Length / sizeof(WCHAR) ); - memcpy( filename, pldr->FullDllName.Buffer, len * sizeof(WCHAR) ); - if (len < size) - { - filename[len] = 0; - SetLastError( 0 ); - } - else SetLastError( ERROR_INSUFFICIENT_BUFFER ); - } - - LdrUnlockLoaderLock( 0, magic ); + name.Buffer = filename; + name.MaximumLength = size * sizeof(WCHAR); + status = LdrGetDllFullName( module, &name ); + if (!status || status == STATUS_BUFFER_TOO_SMALL) len = name.Length / sizeof(WCHAR); + SetLastError( RtlNtStatusToDosError( status )); done: TRACE( "%s\n", debugstr_wn(filename, len) ); return len; -- 2.31.1
On 9/28/21 04:09, Alex Henrie wrote:
Thanks, good to know. While I also think it would be good to have something to print failed LdrGetProcedureAddress results, I am currently not pursuing diagnostics improvements. I am trying to approach (once again) the loader locking redesign to match newer Windows behaviour (as we are starting to hit the deadlocks in some games), and the preparation step is to get rid of some bits of loader locking management outside of ntdll.
participants (2)
-
Alex Henrie -
Paul Gofman