[PATCH v3 0/4] MR6792: ntdll: Implement HashLinks field in LDR module data.
-- v3: ntdll: Use HashLinks when searching for a dll using the basename. kernel32/tests: Add tests for module hash links. kernel32/tests: Factor out is_old_loader_struct(). https://gitlab.winehq.org/wine/wine/-/merge_requests/6792
From: Michael Müller <michael(a)fds-team.de> Changed by Paul Gofman in Wine-Staging patch: - remove older hash version support and use RtlHashUnicodeString(); - split off the test in a separate patch; - remove unrelated InInitializationOrderLinks nullification; - remove a comment. --- dlls/ntdll/loader.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 2f2a7fe5427..9e2a32f69d3 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -128,6 +128,9 @@ struct file_id BYTE ObjectId[16]; }; +#define HASH_MAP_SIZE 32 +static LIST_ENTRY hash_table[HASH_MAP_SIZE]; + /* internal representation of loaded modules */ typedef struct _wine_modref { @@ -600,6 +603,15 @@ static int base_address_compare( const void *key, const RTL_BALANCED_NODE *entry return 0; } +/* compute basename hash */ +static ULONG hash_basename( const UNICODE_STRING *basename ) +{ + ULONG hash = 0; + + RtlHashUnicodeString( basename, TRUE, HASH_STRING_ALGORITHM_DEFAULT, &hash ); + return hash & (HASH_MAP_SIZE - 1); +} + /************************************************************************* * get_modref * @@ -1596,6 +1608,7 @@ static WINE_MODREF *alloc_module( HMODULE hModule, const UNICODE_STRING *nt_name &wm->ldr.InLoadOrderLinks); InsertTailList(&NtCurrentTeb()->Peb->LdrData->InMemoryOrderModuleList, &wm->ldr.InMemoryOrderLinks); + InsertTailList(&hash_table[hash_basename( &wm->ldr.BaseDllName )], &wm->ldr.HashLinks); if (rtl_rb_tree_put( &base_address_index_tree, wm->ldr.DllBase, &wm->ldr.BaseAddressIndexNode, base_address_compare )) ERR( "rtl_rb_tree_put failed.\n" ); /* wait until init is called for inserting into InInitializationOrderModuleList */ @@ -2296,6 +2309,7 @@ static NTSTATUS build_module( LPCWSTR load_path, const UNICODE_STRING *nt_name, /* the module has only be inserted in the load & memory order lists */ RemoveEntryList(&wm->ldr.InLoadOrderLinks); RemoveEntryList(&wm->ldr.InMemoryOrderLinks); + RemoveEntryList(&wm->ldr.HashLinks); RtlRbRemoveNode( &base_address_index_tree, &wm->ldr.BaseAddressIndexNode ); /* FIXME: there are several more dangling references @@ -3966,6 +3980,7 @@ static void free_modref( WINE_MODREF *wm ) RemoveEntryList(&wm->ldr.InLoadOrderLinks); RemoveEntryList(&wm->ldr.InMemoryOrderLinks); + RemoveEntryList(&wm->ldr.HashLinks); RtlRbRemoveNode( &base_address_index_tree, &wm->ldr.BaseAddressIndexNode ); if (wm->ldr.InInitializationOrderLinks.Flink) RemoveEntryList(&wm->ldr.InInitializationOrderLinks); @@ -4387,6 +4402,7 @@ void loader_init( CONTEXT *context, void **entry ) ANSI_STRING ctrl_routine = RTL_CONSTANT_STRING( "CtrlRoutine" ); WINE_MODREF *kernel32; PEB *peb = NtCurrentTeb()->Peb; + unsigned int i; peb->LdrData = &ldr; peb->FastPebLock = &peb_lock; @@ -4405,6 +4421,9 @@ void loader_init( CONTEXT *context, void **entry ) if (!(tls_dirs = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, tls_module_count * sizeof(*tls_dirs) ))) NtTerminateProcess( GetCurrentProcess(), STATUS_NO_MEMORY ); + for (i = 0; i < HASH_MAP_SIZE; i++) + InitializeListHead( &hash_table[i] ); + init_user_process_params(); load_global_options(); version_init(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6792
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/kernel32/tests/module.c | 50 +++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/dlls/kernel32/tests/module.c b/dlls/kernel32/tests/module.c index 8f3ed5872e4..72c2ce97127 100644 --- a/dlls/kernel32/tests/module.c +++ b/dlls/kernel32/tests/module.c @@ -141,6 +141,34 @@ static void create_test_dll( const char *name ) CloseHandle( handle ); } +static BOOL is_old_loader_struct(void) +{ + LDR_DATA_TABLE_ENTRY *mod, *mod2; + LDR_DDAG_NODE *ddag_node; + NTSTATUS status; + HMODULE hexe; + + /* Check for old LDR data strcuture. */ + hexe = GetModuleHandleW( NULL ); + ok( !!hexe, "Got NULL exe handle.\n" ); + status = LdrFindEntryForAddress( hexe, &mod ); + ok( !status, "got %#lx.\n", status ); + if (!(ddag_node = mod->DdagNode)) + { + win_skip( "DdagNode is NULL, skipping tests.\n" ); + return TRUE; + } + ok( !!ddag_node->Modules.Flink, "Got NULL module link.\n" ); + mod2 = CONTAINING_RECORD(ddag_node->Modules.Flink, LDR_DATA_TABLE_ENTRY, NodeModuleLink); + ok( mod2 == mod || broken( (void **)mod2 == (void **)mod - 1 ), "got %p, expected %p.\n", mod2, mod ); + if (mod2 != mod) + { + win_skip( "Old LDR_DATA_TABLE_ENTRY structure, skipping tests.\n" ); + return TRUE; + } + return FALSE; +} + static void testGetModuleFileName(const char* name) { HMODULE hMod; @@ -1750,29 +1778,9 @@ static void test_base_address_index_tree(void) unsigned int tree_count, list_count = 0; LDR_DATA_TABLE_ENTRY *mod, *mod2; RTL_BALANCED_NODE *root, *node; - LDR_DDAG_NODE *ddag_node; - NTSTATUS status; - HMODULE hexe; char *base; - /* Check for old LDR data strcuture. */ - hexe = GetModuleHandleW( NULL ); - ok( !!hexe, "Got NULL exe handle.\n" ); - status = LdrFindEntryForAddress( hexe, &mod ); - ok( !status, "got %#lx.\n", status ); - if (!(ddag_node = mod->DdagNode)) - { - win_skip( "DdagNode is NULL, skipping tests.\n" ); - return; - } - ok( !!ddag_node->Modules.Flink, "Got NULL module link.\n" ); - mod2 = CONTAINING_RECORD(ddag_node->Modules.Flink, LDR_DATA_TABLE_ENTRY, NodeModuleLink); - ok( mod2 == mod || broken( (void **)mod2 == (void **)mod - 1 ), "got %p, expected %p.\n", mod2, mod ); - if (mod2 != mod) - { - win_skip( "Old LDR_DATA_TABLE_ENTRY structure, skipping tests.\n" ); - return; - } + if (is_old_loader_struct()) return; mod = CONTAINING_RECORD(first->Flink, LDR_DATA_TABLE_ENTRY, InLoadOrderLinks); ok( mod->BaseAddressIndexNode.ParentValue || mod->BaseAddressIndexNode.Left || mod->BaseAddressIndexNode.Right, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6792
From: Paul Gofman <pgofman(a)codeweavers.com> Based on a patch by Michael Müller <michael(a)fds-team.de>. --- dlls/kernel32/tests/module.c | 43 ++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/dlls/kernel32/tests/module.c b/dlls/kernel32/tests/module.c index 72c2ce97127..0619ce5f747 100644 --- a/dlls/kernel32/tests/module.c +++ b/dlls/kernel32/tests/module.c @@ -48,6 +48,8 @@ static NTSTATUS (WINAPI *pLdrGetDllFullName)( HMODULE module, UNICODE_STRING *na static BOOL (WINAPI *pIsApiSetImplemented)(LPCSTR); +static NTSTATUS (WINAPI *pRtlHashUnicodeString)( const UNICODE_STRING *, BOOLEAN, ULONG, ULONG * ); + static BOOL is_unicode_enabled = TRUE; static BOOL cmpStrAW(const char* a, const WCHAR* b, DWORD lenA, DWORD lenB) @@ -967,6 +969,7 @@ static void init_pointers(void) MAKEFUNC(LdrGetDllHandle); MAKEFUNC(LdrGetDllHandleEx); MAKEFUNC(LdrGetDllFullName); + MAKEFUNC(RtlHashUnicodeString); mod = GetModuleHandleA( "kernelbase.dll" ); MAKEFUNC(IsApiSetImplemented); #undef MAKEFUNC @@ -1810,6 +1813,45 @@ static void test_base_address_index_tree(void) ok( tree_count == list_count, "count mismatch %u, %u.\n", tree_count, list_count ); } +static ULONG hash_basename( const UNICODE_STRING *basename ) +{ + NTSTATUS status; + ULONG hash; + + status = pRtlHashUnicodeString( basename, TRUE, HASH_STRING_ALGORITHM_DEFAULT, &hash ); + ok( !status, "got %#lx.\n", status ); + return hash & 31; +} + +static void test_hash_links(void) +{ + LIST_ENTRY *hash_map, *entry, *entry2, *mark, *root; + LDR_DATA_TABLE_ENTRY *module; + const WCHAR *modname; + BOOL found; + + /* Hash links structure is the same on older Windows loader but hashing algorithm is different. */ + if (is_old_loader_struct()) return; + + root = &NtCurrentTeb()->Peb->LdrData->InLoadOrderModuleList; + module = CONTAINING_RECORD(root->Flink, LDR_DATA_TABLE_ENTRY, InLoadOrderLinks); + hash_map = module->HashLinks.Blink - hash_basename( &module->BaseDllName ); + + for (entry = root->Flink; entry != root; entry = entry->Flink) + { + module = CONTAINING_RECORD(entry, LDR_DATA_TABLE_ENTRY, InLoadOrderLinks); + modname = module->BaseDllName.Buffer; + mark = &hash_map[hash_basename( &module->BaseDllName )]; + found = FALSE; + for (entry2 = mark->Flink; entry2 != mark; entry2 = entry2->Flink) + { + module = CONTAINING_RECORD(entry2, LDR_DATA_TABLE_ENTRY, HashLinks); + if ((found = !lstrcmpiW( module->BaseDllName.Buffer, modname ))) break; + } + ok( found, "Could not find %s.\n", debugstr_w(modname) ); + } +} + START_TEST(module) { WCHAR filenameW[MAX_PATH]; @@ -1848,4 +1890,5 @@ START_TEST(module) test_ddag_node(); test_tls_links(); test_base_address_index_tree(); + test_hash_links(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6792
From: Michael Müller <michael(a)fds-team.de> --- dlls/ntdll/loader.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 9e2a32f69d3..c3d3896c8ea 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -647,10 +647,10 @@ static WINE_MODREF *find_basename_module( LPCWSTR name ) if (cached_modref && RtlEqualUnicodeString( &name_str, &cached_modref->ldr.BaseDllName, TRUE )) return cached_modref; - mark = &NtCurrentTeb()->Peb->LdrData->InLoadOrderModuleList; + mark = &hash_table[hash_basename( &name_str )]; for (entry = mark->Flink; entry != mark; entry = entry->Flink) { - WINE_MODREF *mod = CONTAINING_RECORD(entry, WINE_MODREF, ldr.InLoadOrderLinks); + WINE_MODREF *mod = CONTAINING_RECORD(entry, WINE_MODREF, ldr.HashLinks); if (RtlEqualUnicodeString( &name_str, &mod->ldr.BaseDllName, TRUE ) && !mod->system) { cached_modref = CONTAINING_RECORD(mod, WINE_MODREF, ldr); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6792
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=149601 Your paranoid android. === build (build log) === error: patch failed: dlls/ntdll/loader.c:128 error: patch failed: dlls/kernel32/tests/module.c:48 error: patch failed: dlls/ntdll/loader.c:647 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/ntdll/loader.c:128 error: patch failed: dlls/kernel32/tests/module.c:48 error: patch failed: dlls/ntdll/loader.c:647 Task: Patch failed to apply === debian11b (build log) === error: patch failed: dlls/ntdll/loader.c:128 error: patch failed: dlls/kernel32/tests/module.c:48 error: patch failed: dlls/ntdll/loader.c:647 Task: Patch failed to apply
v3: - also remove version checks in test (and skip thus crashing tests on Win7 based on loader structure layout). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6792#note_87066
participants (4)
-
Marvin -
Michael Müller -
Paul Gofman
-
Paul Gofman (＠gofman)