[PATCH v2 0/5] MR7347: ntdll: Avoid attach even as dependency if loaded with `DONT_RESOLVE_DLL_REFERENCES`
This cleans up `LDR_DONT_RESOLVE_REFS` checks in preparation for !7.
-- v2: ntdlll: Remove redundant LDR_DONT_RESOLVE_REFS checks before calling process_attach(). ntdll: Skip DLL initialization and ldr notification entirely if DONT_RESOLVE_DLL_REFERENCES is set. kernel32/tests: Test for unexpected loader notification for import dependency loaded with DONT_RESOLVE_DLL_REFERENCES. kernel32/tests: Test for unexpected LDR_PROCESS_ATTACHED flag in import dependency loaded with DONT_RESOLVE_DLL_REFERENCES. kernel32/tests: Add basic tests for internal flags of modules loaded with DONT_RESOLVE_DLL_REFERENCES.
From: Jinoh Kang jinoh.kang.kr@gmail.com
--- dlls/kernel32/tests/module.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
diff --git a/dlls/kernel32/tests/module.c b/dlls/kernel32/tests/module.c index 0619ce5f747..6d6fb236a64 100644 --- a/dlls/kernel32/tests/module.c +++ b/dlls/kernel32/tests/module.c @@ -1852,6 +1852,40 @@ static void test_hash_links(void) } }
+static void test_dont_resolve_dll_references(void) +{ + char tmp_path[MAX_PATH], tmp_file[MAX_PATH]; + LDR_DATA_TABLE_ENTRY *mod; + NTSTATUS status; + HMODULE modbase; + DWORD ret; + int ires; + + ret = GetTempPathA( sizeof(tmp_path), tmp_path ); + ok( !!ret, "GetTempPathA returned %lu (err %lu)\n", ret, GetLastError() ); + + ires = sprintf( tmp_file, "%swtstdrdr.dll", tmp_path ); + ok( ires >= 0 && ires < sizeof(tmp_file), "sprintf returned %d\n", ires ); + + create_test_dll( tmp_file ); + + modbase = LoadLibraryExA( tmp_file, 0, DONT_RESOLVE_DLL_REFERENCES ); + ok( modbase != NULL, "LoadLibrary returned %p (err %lu)\n", modbase, GetLastError() ); + + status = LdrFindEntryForAddress( modbase, &mod ); + ok( !status, "LdrFindEntryForAddress returned %lx\n", status ); + + ok( !(mod->Flags & LDR_LOAD_IN_PROGRESS), "expected LDR_LOAD_IN_PROGRESS to be unset (Flags: %lx)\n", mod->Flags ); + ok( !(mod->Flags & LDR_UNLOAD_IN_PROGRESS), "expected LDR_UNLOAD_IN_PROGRESS to be unset (Flags: %lx)\n", mod->Flags ); + ok( !(mod->Flags & LDR_PROCESS_ATTACHED), "expected LDR_PROCESS_ATTACHED to be unset (Flags: %lx)\n", mod->Flags ); + + ret = FreeLibrary( modbase ); + ok( !!ret, "FreeLibrary returned %lu\n", ret ); + + ret = DeleteFileA( tmp_file ); + ok( !!ret, "DeleteFileA returned %lu\n", ret ); +} + START_TEST(module) { WCHAR filenameW[MAX_PATH]; @@ -1891,4 +1925,5 @@ START_TEST(module) test_tls_links(); test_base_address_index_tree(); test_hash_links(); + test_dont_resolve_dll_references(); }
From: Jinoh Kang jinoh.kang.kr@gmail.com
--- dlls/kernel32/tests/loader.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-)
diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 2c7cc784be4..c7872bd3522 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -2669,7 +2669,8 @@ static HANDLE gen_forward_chain_testdll( char testdll_path[MAX_PATH],
static void subtest_export_forwarder_dep_chain( size_t num_chained_export_modules, size_t exporter_index, - BOOL test_static_import ) + BOOL test_static_import, + ULONG first_module_load_flags ) { size_t num_modules = num_chained_export_modules + !!test_static_import; size_t importer_index = test_static_import ? num_modules - 1 : 0; @@ -2711,7 +2712,7 @@ static void subtest_export_forwarder_dep_chain( size_t num_chained_export_module ok( !GetModuleHandleA( temp_paths[i] ), "%s already loaded\n", wine_dbgstr_a( temp_paths[i] ) );
- modules[i] = LoadLibraryA( temp_paths[i] ); + modules[i] = LoadLibraryExA( temp_paths[i], 0, i == 0 ? first_module_load_flags : 0 ); ok( !!modules[i], "LoadLibraryA(temp_paths[%Iu] = %s) err=%lu\n", i, wine_dbgstr_a( temp_paths[i] ), GetLastError() );
@@ -2765,6 +2766,17 @@ static void subtest_export_forwarder_dep_chain( size_t num_chained_export_module ultimate_depender_index = max( ultimate_depender_index, exporter_index ); }
+ if (first_module_load_flags & DONT_RESOLVE_DLL_REFERENCES) + { + LDR_DATA_TABLE_ENTRY *mod; + NTSTATUS status; + + status = LdrFindEntryForAddress( modules[0], &mod ); + ok( !status, "LdrFindEntryForAddress returned %#lx", status ); + todo_wine_if(importer_index == 1) + ok( !(mod->Flags & LDR_PROCESS_ATTACHED), "expected LDR_PROCESS_ATTACHED to be unset (Flags=%#lx)\n", mod->Flags ); + } + if (winetest_debug > 1) trace( "Unreference modules except the ultimate dependant DLL\n" );
@@ -2828,23 +2840,31 @@ static void test_export_forwarder_dep_chain(void) { winetest_push_context( "no import" ); /* export forwarder does not introduce a dependency on its own */ - subtest_export_forwarder_dep_chain( 2, 0, FALSE ); + subtest_export_forwarder_dep_chain( 2, 0, FALSE, 0 ); winetest_pop_context();
winetest_push_context( "static import of export forwarder" ); - subtest_export_forwarder_dep_chain( 2, 0, TRUE ); + subtest_export_forwarder_dep_chain( 2, 0, TRUE, 0 ); winetest_pop_context();
winetest_push_context( "static import of chained export forwarder" ); - subtest_export_forwarder_dep_chain( 3, 0, TRUE ); + subtest_export_forwarder_dep_chain( 3, 0, TRUE, 0 ); winetest_pop_context();
winetest_push_context( "dynamic import of export forwarder" ); - subtest_export_forwarder_dep_chain( 2, 1, FALSE ); + subtest_export_forwarder_dep_chain( 2, 1, FALSE, 0 ); winetest_pop_context();
winetest_push_context( "dynamic import of chained export forwarder" ); - subtest_export_forwarder_dep_chain( 3, 2, FALSE ); + subtest_export_forwarder_dep_chain( 3, 2, FALSE, 0 ); + winetest_pop_context(); + + winetest_push_context( "static import of dll already loaded with DONT_RESOLVE_DLL_REFERENCES" ); + subtest_export_forwarder_dep_chain( 1, 0, TRUE, DONT_RESOLVE_DLL_REFERENCES ); + winetest_pop_context(); + + winetest_push_context( "dynamic import of dll already loaded with DONT_RESOLVE_DLL_REFERENCES" ); + subtest_export_forwarder_dep_chain( 2, 1, FALSE, DONT_RESOLVE_DLL_REFERENCES ); winetest_pop_context(); }
From: Jinoh Kang jinoh.kang.kr@gmail.com
--- dlls/kernel32/tests/loader.c | 67 ++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+)
diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index c7872bd3522..c74256ad628 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -88,6 +88,7 @@ static BOOL (WINAPI *pWow64DisableWow64FsRedirection)(void **); static BOOL (WINAPI *pWow64RevertWow64FsRedirection)(void *); static HMODULE (WINAPI *pLoadPackagedLibrary)(LPCWSTR lpwLibFileName, DWORD Reserved); static NTSTATUS (WINAPI *pLdrRegisterDllNotification)(ULONG, PLDR_DLL_NOTIFICATION_FUNCTION, void *, void **); +static NTSTATUS (WINAPI *pLdrUnregisterDllNotification)(void *);
static PVOID RVAToAddr(DWORD_PTR rva, HMODULE module) { @@ -2667,6 +2668,38 @@ static HANDLE gen_forward_chain_testdll( char testdll_path[MAX_PATH], return file; }
+struct ldr_notify_counter +{ + WCHAR path[MAX_PATH]; + + unsigned int load_count; + unsigned int unload_count; +}; + +static void CALLBACK ldr_notify_counter_callback(ULONG reason, LDR_DLL_NOTIFICATION_DATA *data, void *context) +{ + struct ldr_notify_counter *lnc = context; + + switch (reason) + { + case LDR_DLL_NOTIFICATION_REASON_LOADED: + if (!wcsnicmp( data->Loaded.BaseDllName->Buffer, lnc->path, + data->Loaded.BaseDllName->Length / sizeof(WCHAR) )) + { + lnc->load_count++; + } + break; + + case LDR_DLL_NOTIFICATION_REASON_UNLOADED: + if (!wcsnicmp( data->Unloaded.BaseDllName->Buffer, lnc->path, + data->Unloaded.BaseDllName->Length / sizeof(WCHAR) )) + { + lnc->unload_count++; + } + break; + } +} + static void subtest_export_forwarder_dep_chain( size_t num_chained_export_modules, size_t exporter_index, BOOL test_static_import, @@ -2676,10 +2709,12 @@ static void subtest_export_forwarder_dep_chain( size_t num_chained_export_module size_t importer_index = test_static_import ? num_modules - 1 : 0; DWORD imp_thunk_base_rva, exp_func_base_rva; size_t ultimate_depender_index = 0; /* latest module depending on modules earlier in chain */ + struct ldr_notify_counter lnc; char temp_paths[4][MAX_PATH]; HANDLE temp_files[4]; UINT_PTR exports[2]; HMODULE modules[4]; + void *cookie; BOOL res; size_t i;
@@ -2702,6 +2737,24 @@ static void subtest_export_forwarder_dep_chain( size_t num_chained_export_module i == importer_index ? &imp_thunk_base_rva : NULL ); }
+ if (first_module_load_flags & DONT_RESOLVE_DLL_REFERENCES) + { + NTSTATUS status; + WCHAR *basename; + int cres; + + memset( &lnc, 0, sizeof(lnc) ); + + cres = MultiByteToWideChar( CP_ACP, 0, temp_paths[0], -1, lnc.path, ARRAY_SIZE(lnc.path) ); + ok( cres >= 0, "MultiByteToWideChar returned %d (err %lu)\n", cres, GetLastError() ); + + basename = wcsrchr( lnc.path, L'\' ) + 1; + memmove( lnc.path, basename, (char *)lnc.path + sizeof(lnc.path) - (char *)basename ); + + status = pLdrRegisterDllNotification( 0, ldr_notify_counter_callback, &lnc, &cookie ); + ok( !status, "LdrRegisterDllNotification returned %#lx.\n", status ); + } + if (winetest_debug > 1) trace( "Load the entire test DLL chain\n" );
@@ -2826,6 +2879,19 @@ static void subtest_export_forwarder_dep_chain( size_t num_chained_export_module ok( !GetModuleHandleA( temp_paths[i] ), "modules[%Iu] should not be kept loaded (3)\n", i ); }
+ if (first_module_load_flags & DONT_RESOLVE_DLL_REFERENCES) + { + NTSTATUS status; + + status = pLdrUnregisterDllNotification( cookie ); + ok( !status, "LdrUnregisterDllNotification returned %#lx.\n", status ); + + todo_wine_if(importer_index == 1) + ok( !lnc.load_count, "got %u for load count of first module\n", lnc.load_count ); + todo_wine_if(importer_index == 1) + ok( !lnc.unload_count, "got %u for unload count of first module\n", lnc.unload_count ); + } + if (winetest_debug > 1) trace( "Close and delete temp files\n" );
@@ -4704,6 +4770,7 @@ START_TEST(loader) pRtlImageDirectoryEntryToData = (void *)GetProcAddress(ntdll, "RtlImageDirectoryEntryToData"); pRtlImageNtHeader = (void *)GetProcAddress(ntdll, "RtlImageNtHeader"); pLdrRegisterDllNotification = (void *)GetProcAddress(ntdll, "LdrRegisterDllNotification"); + pLdrUnregisterDllNotification = (void *)GetProcAddress(ntdll, "LdrUnregisterDllNotification"); pFlsAlloc = (void *)GetProcAddress(kernel32, "FlsAlloc"); pFlsSetValue = (void *)GetProcAddress(kernel32, "FlsSetValue"); pFlsGetValue = (void *)GetProcAddress(kernel32, "FlsGetValue");
From: Jinoh Kang jinoh.kang.kr@gmail.com
--- dlls/kernel32/tests/loader.c | 3 --- dlls/ntdll/loader.c | 4 ++++ 2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index c74256ad628..388f7b13a6d 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -2826,7 +2826,6 @@ static void subtest_export_forwarder_dep_chain( size_t num_chained_export_module
status = LdrFindEntryForAddress( modules[0], &mod ); ok( !status, "LdrFindEntryForAddress returned %#lx", status ); - todo_wine_if(importer_index == 1) ok( !(mod->Flags & LDR_PROCESS_ATTACHED), "expected LDR_PROCESS_ATTACHED to be unset (Flags=%#lx)\n", mod->Flags ); }
@@ -2886,9 +2885,7 @@ static void subtest_export_forwarder_dep_chain( size_t num_chained_export_module status = pLdrUnregisterDllNotification( cookie ); ok( !status, "LdrUnregisterDllNotification returned %#lx.\n", status );
- todo_wine_if(importer_index == 1) ok( !lnc.load_count, "got %u for load count of first module\n", lnc.load_count ); - todo_wine_if(importer_index == 1) ok( !lnc.unload_count, "got %u for unload count of first module\n", lnc.unload_count ); }
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 5be6e44435f..1e43c089203 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -1727,6 +1727,10 @@ static NTSTATUS process_attach( LDR_DDAG_NODE *node, LPVOID lpReserved ) mod = CONTAINING_RECORD( node->Modules.Flink, LDR_DATA_TABLE_ENTRY, NodeModuleLink ); wm = CONTAINING_RECORD( mod, WINE_MODREF, ldr );
+ /* Skip initialization entirely if requested */ + if (wm->ldr.Flags & LDR_DONT_RESOLVE_REFS) + return status; + /* prevent infinite recursion in case of cyclical dependencies */ if ( ( wm->ldr.Flags & LDR_LOAD_IN_PROGRESS ) || ( wm->ldr.Flags & LDR_PROCESS_ATTACHED ) )
From: Jinoh Kang jinoh.kang.kr@gmail.com
This is now checked by process_attach() itself. --- dlls/ntdll/loader.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 1e43c089203..5a582ca14bb 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -918,8 +918,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS { WINE_MODREF *imp = get_modref( module ); TRACE( "delay loading %s for '%s'\n", debugstr_w(mod_name), forward ); - if (load_dll( load_path, mod_name, 0, &wm, imp->system ) == STATUS_SUCCESS && - !(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) + if (load_dll( load_path, mod_name, 0, &wm, imp->system ) == STATUS_SUCCESS) { if (!imports_fixup_done && importer) { @@ -3370,7 +3369,7 @@ NTSTATUS WINAPI DECLSPEC_HOTPATCH LdrLoadDll(LPCWSTR path_name, DWORD flags,
nts = load_dll( path_name, dllname ? dllname : libname->Buffer, flags, &wm, FALSE );
- if (nts == STATUS_SUCCESS && !(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) + if (nts == STATUS_SUCCESS) { nts = process_attach( wm->ldr.DdagNode, NULL ); if (nts != STATUS_SUCCESS)
Looks reasonable to me, but Windows CI failures seem valid.
-
Jacek Caban (＠jacek) -
Jinoh Kang
-
Jinoh Kang (＠iamahuman)