This fixes https://bugs.winehq.org/show_bug.cgi?id=52094.
-- v6: ntdll: Properly track refcount with forwarded exports. ntdll: Don't re-add a module dependency if it already exists. ntdll: Remove some NULL checks for current_importer. ntdll: Set export forwarder DLL as the dynamic importer in LdrGetProcedureAddress(). ntdll: Wrap current_modref variable in a new structure.
From: Jinoh Kang jinoh.kang.kr@gmail.com
Prepare for adding context information (e.g. static or dynamic) about the current importer, which is needed for correct RelayFromExclude handling. --- dlls/ntdll/loader.c | 70 ++++++++++++++++++++++++++++++--------------- 1 file changed, 47 insertions(+), 23 deletions(-)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 2f2a7fe5427..0ce704da2b5 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -181,9 +181,16 @@ static RTL_BITMAP tls_bitmap; static RTL_BITMAP tls_expansion_bitmap;
static WINE_MODREF *cached_modref; -static WINE_MODREF *current_modref; static WINE_MODREF *last_failed_modref;
+struct importer +{ + struct importer *prev; + WINE_MODREF *modref; +}; + +static struct importer *current_importer; + static LDR_DDAG_NODE *node_ntdll, *node_kernel32;
static NTSTATUS load_dll( const WCHAR *load_path, const WCHAR *libname, DWORD flags, WINE_MODREF** pwm, BOOL system ); @@ -563,6 +570,20 @@ static ULONG_PTR allocate_stub( const char *dll, const char *name ) static inline ULONG_PTR allocate_stub( const char *dll, const char *name ) { return 0xdeadbeef; } #endif /* __i386__ */
+/* The loader_section must be locked while calling this function. */ +static void push_importer( struct importer *importer, WINE_MODREF *modref ) +{ + importer->modref = modref; + importer->prev = current_importer; + current_importer = importer; +} + +/* The loader_section must be locked while calling this function. */ +static void pop_importer( struct importer *importer ) +{ + current_importer = importer->prev; +} + /* call ldr notifications */ static void call_ldr_notifications( ULONG reason, LDR_DATA_TABLE_ENTRY *module ) { @@ -788,7 +809,7 @@ static NTSTATUS build_import_name( WCHAR buffer[256], const char *import, int le { const API_SET_NAMESPACE *map = NtCurrentTeb()->Peb->ApiSetMap; const API_SET_NAMESPACE_ENTRY *entry; - const WCHAR *host = current_modref ? current_modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *host = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; UNICODE_STRING str;
while (len && import[len-1] == ' ') len--; /* remove trailing spaces */ @@ -972,9 +993,9 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS if (load_dll( load_path, mod_name, 0, &wm, imp->system ) == STATUS_SUCCESS && !(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) { - if (!imports_fixup_done && current_modref) + if (!imports_fixup_done && current_importer) { - add_module_dependency( current_modref->ldr.DdagNode, wm->ldr.DdagNode ); + add_module_dependency( current_importer->modref->ldr.DdagNode, wm->ldr.DdagNode ); } else if (process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) { @@ -1042,12 +1063,12 @@ static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY
if (TRACE_ON(snoop)) { - const WCHAR *user = current_modref ? current_modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *user = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; proc = SNOOP_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } if (TRACE_ON(relay)) { - const WCHAR *user = current_modref ? current_modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *user = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; proc = RELAY_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } return proc; @@ -1140,7 +1161,8 @@ void * WINAPI RtlFindExportedRoutineByName( HMODULE module, const char *name ) */ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LPCWSTR load_path, WINE_MODREF **pwm ) { - BOOL system = current_modref->system || (current_modref->ldr.Flags & LDR_WINE_INTERNAL); + struct importer *importer = current_importer; + BOOL system = importer->modref->system || (importer->modref->ldr.Flags & LDR_WINE_INTERNAL); NTSTATUS status; WINE_MODREF *wmImp; HMODULE imp_mod; @@ -1175,10 +1197,10 @@ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LP { if (status == STATUS_DLL_NOT_FOUND) ERR("Library %s (which is needed by %s) not found\n", - name, debugstr_w(current_modref->ldr.FullDllName.Buffer)); + name, debugstr_w(importer->modref->ldr.FullDllName.Buffer)); else ERR("Loading library %s (which is needed by %s) failed (error %lx).\n", - name, debugstr_w(current_modref->ldr.FullDllName.Buffer), status); + name, debugstr_w(importer->modref->ldr.FullDllName.Buffer), status); return FALSE; }
@@ -1211,7 +1233,7 @@ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LP thunk_list->u1.Function = allocate_stub( name, (const char*)pe_name->Name ); } WARN(" imported from %s, allocating stub %p\n", - debugstr_w(current_modref->ldr.FullDllName.Buffer), + debugstr_w(importer->modref->ldr.FullDllName.Buffer), (void *)thunk_list->u1.Function ); import_list++; thunk_list++; @@ -1231,7 +1253,7 @@ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LP { thunk_list->u1.Function = allocate_stub( name, IntToPtr(ordinal) ); WARN("No implementation for %s.%d imported from %s, setting to %p\n", - name, ordinal, debugstr_w(current_modref->ldr.FullDllName.Buffer), + name, ordinal, debugstr_w(importer->modref->ldr.FullDllName.Buffer), (void *)thunk_list->u1.Function ); } TRACE_(imports)("--- Ordinal %s.%d = %p\n", name, ordinal, (void *)thunk_list->u1.Function ); @@ -1247,7 +1269,7 @@ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LP { thunk_list->u1.Function = allocate_stub( name, (const char*)pe_name->Name ); WARN("No implementation for %s.%s imported from %s, setting to %p\n", - name, pe_name->Name, debugstr_w(current_modref->ldr.FullDllName.Buffer), + name, pe_name->Name, debugstr_w(importer->modref->ldr.FullDllName.Buffer), (void *)thunk_list->u1.Function ); } TRACE_(imports)("--- %s %s.%d = %p\n", @@ -1446,21 +1468,21 @@ static void free_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) */ static NTSTATUS fixup_imports_ilonly( WINE_MODREF *wm, LPCWSTR load_path, void **entry ) { + struct importer importer; NTSTATUS status; void *proc; const char *name; - WINE_MODREF *prev, *imp; + WINE_MODREF *imp;
if (!(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) return STATUS_SUCCESS; /* already done */ wm->ldr.Flags &= ~LDR_DONT_RESOLVE_REFS;
- prev = current_modref; - current_modref = wm; + push_importer( &importer, wm ); assert( !wm->ldr.DdagNode->Dependencies.Tail ); if (!(status = load_dll( load_path, L"mscoree.dll", 0, &imp, FALSE )) && !add_module_dependency_after( wm->ldr.DdagNode, imp->ldr.DdagNode, NULL )) status = STATUS_NO_MEMORY; - current_modref = prev; + pop_importer( &importer ); if (status) { ERR( "mscoree.dll not found, IL-only binary %s cannot be loaded\n", @@ -1487,7 +1509,8 @@ static NTSTATUS fixup_imports( WINE_MODREF *wm, LPCWSTR load_path ) { const IMAGE_IMPORT_DESCRIPTOR *imports; SINGLE_LIST_ENTRY *dep_after; - WINE_MODREF *prev, *imp; + struct importer importer; + WINE_MODREF *imp; int i, nb_imports; DWORD size; NTSTATUS status; @@ -1513,8 +1536,7 @@ static NTSTATUS fixup_imports( WINE_MODREF *wm, LPCWSTR load_path ) /* load the imported modules. They are automatically * added to the modref list of the process. */ - prev = current_modref; - current_modref = wm; + push_importer( &importer, wm ); status = STATUS_SUCCESS; for (i = 0; i < nb_imports; i++) { @@ -1524,7 +1546,7 @@ static NTSTATUS fixup_imports( WINE_MODREF *wm, LPCWSTR load_path ) else if (imp && imp->ldr.DdagNode != node_ntdll && imp->ldr.DdagNode != node_kernel32) add_module_dependency_after( wm->ldr.DdagNode, imp->ldr.DdagNode, dep_after ); } - current_modref = prev; + pop_importer( &importer ); if (wm->ldr.ActivationContext) RtlDeactivateActivationContext( 0, cookie ); return status; } @@ -1804,8 +1826,9 @@ static NTSTATUS process_attach( LDR_DDAG_NODE *node, LPVOID lpReserved ) /* Call DLL entry point */ if (status == STATUS_SUCCESS) { - WINE_MODREF *prev = current_modref; - current_modref = wm; + struct importer importer; + + push_importer( &importer, wm );
call_ldr_notifications( LDR_DLL_NOTIFICATION_REASON_LOADED, &wm->ldr ); status = MODULE_InitDLL( wm, DLL_PROCESS_ATTACH, lpReserved ); @@ -1822,7 +1845,8 @@ static NTSTATUS process_attach( LDR_DDAG_NODE *node, LPVOID lpReserved ) last_failed_modref = wm; WARN("Initialization of %s failed\n", debugstr_w(wm->ldr.BaseDllName.Buffer)); } - current_modref = prev; + + pop_importer( &importer ); }
if (wm->ldr.ActivationContext) RtlDeactivateActivationContext( 0, cookie );
From: Jinoh Kang jinoh.kang.kr@gmail.com
This matches the Windows GetProcAddress() behavior when handling dynamic module dependencies.
To avoid breaking WINEDEBUG=+{relay,snoop}, flag dynamic importers (`is_dynamic = TRUE`) and explicitly ignore them when testing for RelayFromExclude modules.
Otherwise, GetProcAddress() on kernel32 export forwarders (which compromise most of the exports) will be recognized as relaying *from* kernel32 itself (instead of the actual importer) and will be subsequently excluded from WINEDEBUG=+relay due to the default `RelayFromExclude=(...);kernel32`. The current behavior is to treat it as relaying from the actual importer, not kernel32.
This bit is true for all export forwarder DLLs in general, and also affects RelayFromInclude as well as SnoopFrom{Exclude,Include}. --- dlls/ntdll/loader.c | 38 ++++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 0ce704da2b5..ea01263d023 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -187,6 +187,7 @@ struct importer { struct importer *prev; WINE_MODREF *modref; + BOOL is_dynamic; };
static struct importer *current_importer; @@ -571,9 +572,10 @@ static inline ULONG_PTR allocate_stub( const char *dll, const char *name ) { ret #endif /* __i386__ */
/* The loader_section must be locked while calling this function. */ -static void push_importer( struct importer *importer, WINE_MODREF *modref ) +static void push_importer( struct importer *importer, WINE_MODREF *modref, BOOL is_dynamic ) { importer->modref = modref; + importer->is_dynamic = is_dynamic; importer->prev = current_importer; current_importer = importer; } @@ -584,6 +586,20 @@ static void pop_importer( struct importer *importer ) current_importer = importer->prev; }
+/* The loader_section must be locked while calling this function. */ +static const WCHAR *get_last_static_importer_name(void) +{ + struct importer *importer; + for (importer = current_importer; importer != NULL; importer = importer->prev) + { + if (!importer->is_dynamic) + { + return importer->modref->ldr.BaseDllName.Buffer; + } + } + return NULL; +} + /* call ldr notifications */ static void call_ldr_notifications( ULONG reason, LDR_DATA_TABLE_ENTRY *module ) { @@ -1063,12 +1079,12 @@ static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY
if (TRACE_ON(snoop)) { - const WCHAR *user = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *user = get_last_static_importer_name(); proc = SNOOP_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } if (TRACE_ON(relay)) { - const WCHAR *user = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *user = get_last_static_importer_name(); proc = RELAY_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } return proc; @@ -1477,7 +1493,7 @@ static NTSTATUS fixup_imports_ilonly( WINE_MODREF *wm, LPCWSTR load_path, void * if (!(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) return STATUS_SUCCESS; /* already done */ wm->ldr.Flags &= ~LDR_DONT_RESOLVE_REFS;
- push_importer( &importer, wm ); + push_importer( &importer, wm, FALSE ); assert( !wm->ldr.DdagNode->Dependencies.Tail ); if (!(status = load_dll( load_path, L"mscoree.dll", 0, &imp, FALSE )) && !add_module_dependency_after( wm->ldr.DdagNode, imp->ldr.DdagNode, NULL )) @@ -1536,7 +1552,7 @@ static NTSTATUS fixup_imports( WINE_MODREF *wm, LPCWSTR load_path ) /* load the imported modules. They are automatically * added to the modref list of the process. */ - push_importer( &importer, wm ); + push_importer( &importer, wm, FALSE ); status = STATUS_SUCCESS; for (i = 0; i < nb_imports; i++) { @@ -1828,7 +1844,7 @@ static NTSTATUS process_attach( LDR_DDAG_NODE *node, LPVOID lpReserved ) { struct importer importer;
- push_importer( &importer, wm ); + push_importer( &importer, wm, FALSE );
call_ldr_notifications( LDR_DLL_NOTIFICATION_REASON_LOADED, &wm->ldr ); status = MODULE_InitDLL( wm, DLL_PROCESS_ATTACH, lpReserved ); @@ -2112,8 +2128,14 @@ NTSTATUS WINAPI LdrGetProcedureAddress(HMODULE module, const ANSI_STRING *name, else 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 ); + struct importer importer; + void *proc; + + push_importer( &importer, wm, TRUE ); + proc = name ? find_named_export( module, exports, exp_size, name->Buffer, -1, NULL ) + : find_ordinal_export( module, exports, exp_size, ord - exports->Base, NULL ); + pop_importer( &importer ); + if (proc) { *address = proc;
From: Jinoh Kang jinoh.kang.kr@gmail.com
current_importer is now set by all callers of build_import_name, find_forwarded_export, and find_ordinal_export. --- 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 ea01263d023..5f7ce313113 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -825,7 +825,7 @@ static NTSTATUS build_import_name( WCHAR buffer[256], const char *import, int le { const API_SET_NAMESPACE *map = NtCurrentTeb()->Peb->ApiSetMap; const API_SET_NAMESPACE_ENTRY *entry; - const WCHAR *host = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *host = current_importer->modref->ldr.BaseDllName.Buffer; UNICODE_STRING str;
while (len && import[len-1] == ' ') len--; /* remove trailing spaces */ @@ -1009,7 +1009,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS if (load_dll( load_path, mod_name, 0, &wm, imp->system ) == STATUS_SUCCESS && !(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) { - if (!imports_fixup_done && current_importer) + if (!imports_fixup_done) { add_module_dependency( current_importer->modref->ldr.DdagNode, wm->ldr.DdagNode ); }
From: Jinoh Kang jinoh.kang.kr@gmail.com
Today, calling add_module_dependency() multiple times with the same arguments results in duplicate edges.
Duplicate edges are harmless, but bloats memory usage. The number of duplicate edges does not affect the dependency graph; the graph is determined by the set of unique edges.
Consciously avoid duplicates by checking for them in add_module_dependency_after(). This allows us to generate a unique dependency edge for all imports of export forwarders that belong to the same DLL. --- dlls/ntdll/loader.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 5f7ce313113..29108c401cc 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -925,6 +925,21 @@ static void remove_single_list_entry( LDRP_CSLIST *list, SINGLE_LIST_ENTRY *entr entry->Next = NULL; }
+static LDR_DEPENDENCY *find_module_dependency( LDR_DDAG_NODE *from, LDR_DDAG_NODE *to ) +{ + SINGLE_LIST_ENTRY *entry, *mark = from->Dependencies.Tail; + + if (!mark) return NULL; + + for (entry = mark->Next; entry != mark; entry = entry->Next) + { + LDR_DEPENDENCY *dep = CONTAINING_RECORD( entry, LDR_DEPENDENCY, dependency_to_entry ); + if (dep->dependency_from == from && dep->dependency_to == to) return dep; + } + + return NULL; +} + /********************************************************************** * add_module_dependency_after */ @@ -933,6 +948,15 @@ static BOOL add_module_dependency_after( LDR_DDAG_NODE *from, LDR_DDAG_NODE *to, { LDR_DEPENDENCY *dep;
+ if ((dep = find_module_dependency( from, to ))) + { + /* Dependency already exists; consume the module reference stolen from the caller */ + LDR_DATA_TABLE_ENTRY *mod = CONTAINING_RECORD( to->Modules.Flink, LDR_DATA_TABLE_ENTRY, NodeModuleLink ); + WINE_MODREF *wm = CONTAINING_RECORD( mod, WINE_MODREF, ldr ); + LdrUnloadDll( wm->ldr.DllBase ); + return TRUE; + } + if (!(dep = RtlAllocateHeap( GetProcessHeap(), 0, sizeof(*dep) ))) return FALSE;
dep->dependency_from = from;
From: Jinoh Kang jinoh.kang.kr@gmail.com
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52094 --- dlls/kernel32/tests/loader.c | 2 -- dlls/ntdll/loader.c | 17 ++++++++++++----- 2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 9b0f8f6bff2..36f04afbf3d 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -2775,7 +2775,6 @@ static void subtest_export_forwarder_dep_chain( size_t num_chained_export_module
/* FreeLibrary() should *not* unload the DLL immediately */ module = GetModuleHandleA( temp_paths[i] ); - todo_wine_if(i < ultimate_depender_index && i + 1 != importer_index) ok( module == modules[i], "modules[%Iu] expected %p, got %p (unloaded?) err=%lu\n", i, modules[i], module, GetLastError() ); } @@ -2787,7 +2786,6 @@ static void subtest_export_forwarder_dep_chain( size_t num_chained_export_module { HMODULE module = GetModuleHandleA( temp_paths[i] );
- todo_wine_if(i < ultimate_depender_index && i + 1 != importer_index) ok( module == modules[i], "modules[%Iu] expected %p, got %p (unloaded?) err=%lu\n", i, modules[i], module, GetLastError() ); } diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 29108c401cc..eadda01ac14 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -1033,11 +1033,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS if (load_dll( load_path, mod_name, 0, &wm, imp->system ) == STATUS_SUCCESS && !(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) { - if (!imports_fixup_done) - { - add_module_dependency( current_importer->modref->ldr.DdagNode, wm->ldr.DdagNode ); - } - else if (process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) + if (imports_fixup_done && process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) { LdrUnloadDll( wm->ldr.DllBase ); wm = NULL; @@ -1051,6 +1047,11 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS return NULL; } } + else + { + if (wm->ldr.LoadCount != -1) wm->ldr.LoadCount++; + } + if ((exports = RtlImageDirectoryEntryToData( wm->ldr.DllBase, TRUE, IMAGE_DIRECTORY_ENTRY_EXPORT, &exp_size ))) { @@ -1070,6 +1071,12 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS forward, debugstr_w(get_modref(module)->ldr.FullDllName.Buffer), debugstr_w(get_modref(module)->ldr.BaseDllName.Buffer) ); } + else if (wm->ldr.DdagNode != node_ntdll && wm->ldr.DdagNode != node_kernel32) + { + add_module_dependency( current_importer->modref->ldr.DdagNode, wm->ldr.DdagNode ); + wm = NULL; + } + if (wm) LdrUnloadDll( wm->ldr.DllBase ); return proc; }
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 tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details:
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=149566
Your paranoid android.
=== debian11b (64 bit WoW report) ===
d2d1: d2d1.c:10856: Test failed: Didn't expect timeout.
v6: * Added justification for importer struct in commit message. * Fix `test_ddag_node` breakage due to missing special case foe ntdll/kernel32
-
Jinoh Kang -
Jinoh Kang (＠iamahuman)
-
Marvin