This fixes https://bugs.winehq.org/show_bug.cgi?id=52094.
-- v5: 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: Update current 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. --- 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 is needed to handle dynamic module dependencies generated by GetProcAddress() correctly.
Take care to leave the relay/snoop filtering behavior unaffected. In particular, dynamic imports are explicitly ignored when computing the "from" module. --- dlls/ntdll/loader.c | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 0ce704da2b5..1debae6bc86 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,18 @@ static void pop_importer( struct importer *importer ) current_importer = importer->prev; }
+/* The loader_section must be locked while calling this function. */ +static WINE_MODREF *get_last_static_importer_modref(void) +{ + struct importer *importer = current_importer; + while (importer) + { + if (!importer->is_dynamic) return importer->modref; + importer = importer->prev; + } + return NULL; +} + /* call ldr notifications */ static void call_ldr_notifications( ULONG reason, LDR_DATA_TABLE_ENTRY *module ) { @@ -1063,12 +1077,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 = current_importer ? get_last_static_importer_modref()->ldr.BaseDllName.Buffer : NULL; 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 = current_importer ? get_last_static_importer_modref()->ldr.BaseDllName.Buffer : NULL; proc = RELAY_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } return proc; @@ -1477,7 +1491,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 +1550,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 +1842,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 +2126,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 | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 1debae6bc86..f47f77c1e6a 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -823,7 +823,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 */ @@ -1007,7 +1007,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 ); } @@ -1077,12 +1077,12 @@ static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY
if (TRACE_ON(snoop)) { - const WCHAR *user = current_importer ? get_last_static_importer_modref()->ldr.BaseDllName.Buffer : NULL; + const WCHAR *user = get_last_static_importer_modref()->ldr.BaseDllName.Buffer; proc = SNOOP_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } if (TRACE_ON(relay)) { - const WCHAR *user = current_importer ? get_last_static_importer_modref()->ldr.BaseDllName.Buffer : NULL; + const WCHAR *user = get_last_static_importer_modref()->ldr.BaseDllName.Buffer; proc = RELAY_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } return proc;
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 f47f77c1e6a..73fd6e97109 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -923,6 +923,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 */ @@ -931,6 +946,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 | 16 +++++++++++----- 2 files changed, 11 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 73fd6e97109..52c4ae2b33d 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -1031,11 +1031,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; @@ -1049,6 +1045,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 ))) { @@ -1067,6 +1068,11 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS " If you are using builtin %s, try using the native one instead.\n", forward, debugstr_w(get_modref(module)->ldr.FullDllName.Buffer), debugstr_w(get_modref(module)->ldr.BaseDllName.Buffer) ); + if (wm) LdrUnloadDll( wm->ldr.DllBase ); + } + else + { + add_module_dependency( current_importer->modref->ldr.DdagNode, wm->ldr.DdagNode ); } 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=149550
Your paranoid android.
=== w10pro64 (32 bit report) ===
kernel32: loader.c:3412: Test failed: attached thread count should be 2
=== debian11b (64 bit WoW report) ===
kernel32: module.c:1644: Test failed: Dep 1 (L"msvcrt.dll"): Got unexpected module L"ntdll.dll". module.c:1637: Test failed: Dep 2 (L"user32.dll"): Module is not directly referenced. module.c:1663: Test failed: Expected end of the list.
-
Jinoh Kang -
Jinoh Kang (＠iamahuman)
-
Marvin