This fixes https://bugs.winehq.org/show_bug.cgi?id=52094.
For reference, here's how current_modref is being passed around (before this patch):
```mermaid graph TB fii["fixup_imports_ilonly<br><em style='color:#ff0'>(directly sets current_modref)</em>"] fi["fixup_imports<br><em style='color:#ff0'>(directly sets current_modref)</em>"] pa["process_attach<br><em style='color:#ff0'>(directly sets current_modref)</em>"] ld[load_dll] id["import_dll<br><em style='color:#0f0'>(directly uses current_modref)</em>"] bin["build_import_name<br><em style='color:#0f0'>(directly uses current_modref)</em>"] foe["find_ordinal_export<br><em style='color:#0f0'>(uses current_modref for relay)</em>"] ffe["find_forwarded_export<br><em style='color:#0f0'>(directly uses current_modref)</em>"] fne[find_named_export] MI[MODULE_InitDLL] fii --> ld fi --> id pa --> MI -.-> DllMain id --> bin id --> ld id --> foe id --> fne --> foe --> ffe --> foe ffe --> fne ffe --> bin style DllMain color:red; ```
-- v15: ntdll: Remove superflous NULL check for importer. ntdll: Properly track refcount on dynamic imports of export forwarders. ntdll: Explicitly ignore dynamic (GetProcAddress) importers as relay/snoop user module. ntdll: Eagerly call process_attach() on dynamic imports of export forwarders. ntdll: Properly track refcount on static imports of export forwarders. ntdll: Register module dependency for export forwarders regardless of whether the dependency is already loaded. ntdll: Don't re-add a module dependency if it already exists. ntdll: Sink module dependency registration towards the end of the
From: Jinoh Kang jinoh.kang.kr@gmail.com
--- dlls/ntdll/loader.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 5a582ca14bb..a5dc675a8f2 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -910,6 +910,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS WCHAR mod_name[256]; const char *end = strrchr(forward, '.'); FARPROC proc = NULL; + BOOL wm_loaded = FALSE;
if (!end) return NULL; if (build_import_name( importer, mod_name, forward, end - forward )) return NULL; @@ -918,26 +919,30 @@ 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) + if (load_dll( load_path, mod_name, 0, &wm, imp->system ) != STATUS_SUCCESS) { - if (!imports_fixup_done && importer) - { - add_module_dependency( importer->ldr.DdagNode, wm->ldr.DdagNode ); - } - else if (process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) - { - LdrUnloadDll( wm->ldr.DllBase ); - wm = NULL; - } + ERR( "module not found for forward '%s' used by %s\n", + forward, debugstr_w(imp->ldr.FullDllName.Buffer) ); + return NULL; } + wm_loaded = TRUE; + }
- if (!wm) + if (wm_loaded) + { + if (!imports_fixup_done && importer) { - ERR( "module not found for forward '%s' used by %s\n", - forward, debugstr_w(imp->ldr.FullDllName.Buffer) ); + add_module_dependency( importer->ldr.DdagNode, wm->ldr.DdagNode ); + } + else if (process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) + { + ERR( "process_attach failed for forward '%s' used by %s\n", + forward, debugstr_w(get_modref( module )->ldr.FullDllName.Buffer) ); + LdrUnloadDll( wm->ldr.DllBase ); return NULL; } } + if ((exports = RtlImageDirectoryEntryToData( wm->ldr.DllBase, TRUE, IMAGE_DIRECTORY_ENTRY_EXPORT, &exp_size ))) {
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 a5dc675a8f2..c745a488c5b 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -837,6 +837,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_to == to && dep->dependency_from == from) return dep; + } + + return NULL; +} + /********************************************************************** * add_module_dependency_after */ @@ -845,6 +860,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 */ + WINE_MODREF *wm = CONTAINING_RECORD( to->Modules.Flink, WINE_MODREF, ldr.NodeModuleLink ); + assert( wm->ldr.LoadCount != 1 ); + if (wm->ldr.LoadCount != -1) wm->ldr.LoadCount--; + return TRUE; + } + if (!(dep = RtlAllocateHeap( GetProcessHeap(), 0, sizeof(*dep) ))) return FALSE;
dep->dependency_from = from;
From: Jinoh Kang jinoh.kang.kr@gmail.com
Prepare for generalization to imports from DLLs.
Calling add_module_dependency() multiple times for the same dependency edge no longer bloats memory usage. --- dlls/ntdll/loader.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index c745a488c5b..e0c107052b1 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -952,13 +952,15 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS wm_loaded = TRUE; }
- if (wm_loaded) + if (wm->ldr.DdagNode != node_ntdll && wm->ldr.DdagNode != node_kernel32) { if (!imports_fixup_done && importer) { + /* Prepare for the callee stealing the reference */ + if (!wm_loaded && wm->ldr.LoadCount != -1) wm->ldr.LoadCount++; add_module_dependency( importer->ldr.DdagNode, wm->ldr.DdagNode ); } - else if (process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) + else if (wm_loaded && process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) { ERR( "process_attach failed for forward '%s' used by %s\n", forward, debugstr_w(get_modref( module )->ldr.FullDllName.Buffer) );
From: Jinoh Kang jinoh.kang.kr@gmail.com
Today this is only done for the the main EXE. Generalize this to DLLs as well. --- dlls/kernel32/tests/loader.c | 4 ++-- dlls/ntdll/loader.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 1d7aa0dbb6f..3af04bbccb2 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -2839,7 +2839,7 @@ 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) + todo_wine_if(i < ultimate_depender_index && i + 1 != importer_index && !(i < importer_index)) ok( module == modules[i], "modules[%Iu] expected %p, got %p (unloaded?) err=%lu\n", i, modules[i], module, GetLastError() ); } @@ -2851,7 +2851,7 @@ 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) + todo_wine_if(i < ultimate_depender_index && i + 1 != importer_index && !(i < 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 e0c107052b1..6584a992fe0 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -954,7 +954,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS
if (wm->ldr.DdagNode != node_ntdll && wm->ldr.DdagNode != node_kernel32) { - if (!imports_fixup_done && importer) + if (importer) { /* Prepare for the callee stealing the reference */ if (!wm_loaded && wm->ldr.LoadCount != -1) wm->ldr.LoadCount++;
From: Jinoh Kang jinoh.kang.kr@gmail.com
--- dlls/ntdll/loader.c | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 6584a992fe0..4b89e20fc67 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -191,9 +191,11 @@ 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 ); static NTSTATUS process_attach( LDR_DDAG_NODE *node, LPVOID lpReserved ); static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, - DWORD exp_size, DWORD ordinal, LPCWSTR load_path, WINE_MODREF *importer ); + DWORD exp_size, DWORD ordinal, LPCWSTR load_path, + WINE_MODREF *importer, BOOL is_dynamic ); static FARPROC find_named_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, DWORD exp_size, - const char *name, int hint, LPCWSTR load_path, WINE_MODREF *importer ); + const char *name, int hint, LPCWSTR load_path, + WINE_MODREF *importer, BOOL is_dynamic );
/* check whether the file name contains a path */ static inline BOOL contains_path( LPCWSTR name ) @@ -926,7 +928,7 @@ static NTSTATUS walk_node_dependencies( LDR_DDAG_NODE *node, void *context, * Find the final function pointer for a forwarded function. * The loader_section must be locked while calling this function. */ -static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWSTR load_path, WINE_MODREF *importer ) +static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWSTR load_path, WINE_MODREF *importer, BOOL is_dynamic ) { const IMAGE_EXPORT_DIRECTORY *exports; DWORD exp_size; @@ -960,7 +962,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS if (!wm_loaded && wm->ldr.LoadCount != -1) wm->ldr.LoadCount++; add_module_dependency( importer->ldr.DdagNode, wm->ldr.DdagNode ); } - else if (wm_loaded && process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) + if (is_dynamic && wm_loaded && process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) { ERR( "process_attach failed for forward '%s' used by %s\n", forward, debugstr_w(get_modref( module )->ldr.FullDllName.Buffer) ); @@ -976,9 +978,11 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS
if (*name == '#') { /* ordinal */ proc = find_ordinal_export( wm->ldr.DllBase, exports, exp_size, - atoi(name+1) - exports->Base, load_path, importer ); + atoi(name+1) - exports->Base, load_path, + importer, is_dynamic ); } else - proc = find_named_export( wm->ldr.DllBase, exports, exp_size, name, -1, load_path, importer ); + proc = find_named_export( wm->ldr.DllBase, exports, exp_size, name, -1, load_path, + importer, is_dynamic ); }
if (!proc) @@ -1000,7 +1004,8 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS * The loader_section must be locked while calling this function. */ static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, - DWORD exp_size, DWORD ordinal, LPCWSTR load_path, WINE_MODREF *importer ) + DWORD exp_size, DWORD ordinal, LPCWSTR load_path, + WINE_MODREF *importer, BOOL is_dynamic ) { FARPROC proc; const DWORD *functions = get_rva( module, exports->AddressOfFunctions ); @@ -1017,7 +1022,7 @@ static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY /* if the address falls into the export dir, it's a forward */ if (((const char *)proc >= (const char *)exports) && ((const char *)proc < (const char *)exports + exp_size)) - return find_forwarded_export( module, (const char *)proc, load_path, importer ); + return find_forwarded_export( module, (const char *)proc, load_path, importer, is_dynamic );
if (TRACE_ON(snoop)) { @@ -1063,7 +1068,8 @@ static int find_name_in_exports( HMODULE module, const IMAGE_EXPORT_DIRECTORY *e * The loader_section must be locked while calling this function. */ static FARPROC find_named_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, DWORD exp_size, - const char *name, int hint, LPCWSTR load_path, WINE_MODREF *importer ) + const char *name, int hint, LPCWSTR load_path, WINE_MODREF *importer, + BOOL is_dynamic ) { const WORD *ordinals = get_rva( module, exports->AddressOfNameOrdinals ); const DWORD *names = get_rva( module, exports->AddressOfNames ); @@ -1074,12 +1080,12 @@ static FARPROC find_named_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY * { char *ename = get_rva( module, names[hint] ); if (!strcmp( ename, name )) - return find_ordinal_export( module, exports, exp_size, ordinals[hint], load_path, importer ); + return find_ordinal_export( module, exports, exp_size, ordinals[hint], load_path, importer, is_dynamic ); }
/* then do a binary search */ if ((ordinal = find_name_in_exports( module, exports, name )) == -1) return NULL; - return find_ordinal_export( module, exports, exp_size, ordinal, load_path, importer ); + return find_ordinal_export( module, exports, exp_size, ordinal, load_path, importer, is_dynamic );
}
@@ -1206,7 +1212,7 @@ static BOOL import_dll( WINE_MODREF *wm, const IMAGE_IMPORT_DESCRIPTOR *descr, L int ordinal = IMAGE_ORDINAL(import_list->u1.Ordinal);
thunk_list->u1.Function = (ULONG_PTR)find_ordinal_export( imp_mod, exports, exp_size, - ordinal - exports->Base, load_path, wm ); + ordinal - exports->Base, load_path, wm, FALSE ); if (!thunk_list->u1.Function) { thunk_list->u1.Function = allocate_stub( name, IntToPtr(ordinal) ); @@ -1222,7 +1228,7 @@ static BOOL import_dll( WINE_MODREF *wm, const IMAGE_IMPORT_DESCRIPTOR *descr, L pe_name = get_rva( module, (DWORD)import_list->u1.AddressOfData ); thunk_list->u1.Function = (ULONG_PTR)find_named_export( imp_mod, exports, exp_size, (const char*)pe_name->Name, - pe_name->Hint, load_path, wm ); + pe_name->Hint, load_path, wm, FALSE ); if (!thunk_list->u1.Function) { thunk_list->u1.Function = allocate_stub( name, (const char*)pe_name->Name ); @@ -2063,8 +2069,8 @@ 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, NULL ) - : find_ordinal_export( module, exports, exp_size, ord - exports->Base, NULL, NULL ); + void *proc = name ? find_named_export( module, exports, exp_size, name->Buffer, -1, NULL, NULL, TRUE ) + : find_ordinal_export( module, exports, exp_size, ord - exports->Base, NULL, NULL, TRUE ); if (proc) { *address = proc;
From: Jinoh Kang jinoh.kang.kr@gmail.com
--- 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 4b89e20fc67..58d41281641 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -1026,12 +1026,12 @@ static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY
if (TRACE_ON(snoop)) { - const WCHAR *user = importer ? importer->ldr.BaseDllName.Buffer : NULL; + const WCHAR *user = !is_dynamic ? importer->ldr.BaseDllName.Buffer : NULL; proc = SNOOP_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } if (TRACE_ON(relay)) { - const WCHAR *user = importer ? importer->ldr.BaseDllName.Buffer : NULL; + const WCHAR *user = !is_dynamic ? importer->ldr.BaseDllName.Buffer : NULL; proc = RELAY_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } return proc;
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 | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 3af04bbccb2..b14e59f210e 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -2839,7 +2839,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 && !(i < importer_index)) ok( module == modules[i], "modules[%Iu] expected %p, got %p (unloaded?) err=%lu\n", i, modules[i], module, GetLastError() ); } @@ -2851,7 +2850,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 && !(i < 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 58d41281641..7a8bbc77aef 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -2069,8 +2069,8 @@ 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, NULL, TRUE ) - : find_ordinal_export( module, exports, exp_size, ord - exports->Base, NULL, NULL, TRUE ); + void *proc = name ? find_named_export( module, exports, exp_size, name->Buffer, -1, NULL, wm, TRUE ) + : find_ordinal_export( module, exports, exp_size, ord - exports->Base, NULL, wm, TRUE ); if (proc) { *address = proc;
From: Jinoh Kang jinoh.kang.kr@gmail.com
--- dlls/ntdll/loader.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 7a8bbc77aef..144f0bb2096 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -739,7 +739,7 @@ static NTSTATUS build_import_name( WINE_MODREF *importer, WCHAR buffer[256], con { const API_SET_NAMESPACE *map = NtCurrentTeb()->Peb->ApiSetMap; const API_SET_NAMESPACE_ENTRY *entry; - const WCHAR *host = importer ? importer->ldr.BaseDllName.Buffer : NULL; + const WCHAR *host = importer->ldr.BaseDllName.Buffer; UNICODE_STRING str;
while (len && import[len-1] == ' ') len--; /* remove trailing spaces */ @@ -956,12 +956,9 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS
if (wm->ldr.DdagNode != node_ntdll && wm->ldr.DdagNode != node_kernel32) { - if (importer) - { - /* Prepare for the callee stealing the reference */ - if (!wm_loaded && wm->ldr.LoadCount != -1) wm->ldr.LoadCount++; - add_module_dependency( importer->ldr.DdagNode, wm->ldr.DdagNode ); - } + /* Prepare for the callee stealing the reference */ + if (!wm_loaded && wm->ldr.LoadCount != -1) wm->ldr.LoadCount++; + add_module_dependency( importer->ldr.DdagNode, wm->ldr.DdagNode ); if (is_dynamic && wm_loaded && process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) { ERR( "process_attach failed for forward '%s' used by %s\n",
v15:
- Unconditionally add module dependency regardless of proc resolution failure - Address review on commit splitting
This merge request was approved by Jacek Caban.
-
Jacek Caban (＠jacek) -
Jinoh Kang
-
Jinoh Kang (＠iamahuman)