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; ```
-- v14: 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 | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 5a582ca14bb..6a806369efb 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,15 @@ 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 (!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; - } - } - - if (!wm) + if (load_dll( load_path, mod_name, 0, &wm, imp->system ) != STATUS_SUCCESS) { ERR( "module not found for forward '%s' used by %s\n", forward, debugstr_w(imp->ldr.FullDllName.Buffer) ); return NULL; } + wm_loaded = TRUE; } + if ((exports = RtlImageDirectoryEntryToData( wm->ldr.DllBase, TRUE, IMAGE_DIRECTORY_ENTRY_EXPORT, &exp_size ))) { @@ -957,6 +947,18 @@ 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_loaded) + { + 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 ); + proc = NULL; + } + } 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 6a806369efb..40c53064059 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 40c53064059..52b47a217eb 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -971,13 +971,15 @@ 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_loaded) + else 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) { LdrUnloadDll( wm->ldr.DllBase ); proc = NULL;
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 52b47a217eb..c98f00f0645 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -973,7 +973,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS } else 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 c98f00f0645..336d4bc4e08 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; @@ -959,9 +961,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) @@ -979,7 +983,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 || !importer) && wm_loaded && process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) { LdrUnloadDll( wm->ldr.DllBase ); proc = NULL; @@ -997,7 +1001,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 ); @@ -1014,7 +1019,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)) { @@ -1060,7 +1065,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 ); @@ -1071,12 +1077,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 );
}
@@ -1203,7 +1209,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) ); @@ -1219,7 +1225,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 ); @@ -2060,8 +2066,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 336d4bc4e08..3fc641ac977 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -1023,12 +1023,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 ? 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 ? 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 3fc641ac977..1e6c4f3384f 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -2066,8 +2066,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 | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 1e6c4f3384f..043b5692e1c 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 */ @@ -977,13 +977,10 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS } else 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 ); - } - if ((is_dynamic || !importer) && wm_loaded && process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) + /* 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) { LdrUnloadDll( wm->ldr.DllBase ); proc = NULL; @@ -1023,12 +1020,12 @@ static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY
if (TRACE_ON(snoop)) { - const WCHAR *user = !is_dynamic && 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 = !is_dynamic && 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;
v13: Rebase
v14: - Fix `is_dynamic` status not being passed through `find_forwarded_export` - Call `process_attach()` for dynamic imports - Address reviews - Drop fix for leak on import resolution failure
Jacek Caban (@jacek) commented about dlls/ntdll/loader.c:
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 || !importer) && wm_loaded && process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS)
AFAICS, there is no need for `!importer` here.
Jacek Caban (@jacek) commented about dlls/ntdll/loader.c:
if (TRACE_ON(snoop)) {
const WCHAR *user = importer ? importer->ldr.BaseDllName.Buffer : NULL;
const WCHAR *user = !is_dynamic && importer ? importer->ldr.BaseDllName.Buffer : NULL;
Same here, you may just drop `importer` check.
Jacek Caban (@jacek) commented about dlls/ntdll/loader.c:
} else 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 );}if ((is_dynamic || !importer) && wm_loaded && process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS)
/* 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)
I think it would be better to just do that in "ntdll: Properly track refcount on dynamic imports of export forwarders." commit. It would make its impact cleaner.
- Drop fix for leak on import resolution failure
Simply dropping it makes things worse than they are now, we might end up with a non-attached module, causing future calls to treat it as loaded and skip attachment. I tested this on Windows, and it seems that its loader always keeps the loaded module attached with a dependency on the importer, even if the function isn’t found. So, the first commit doesn’t match Windows' behavior.
I also added some comments on patch splitting. I could live with it as is, but since another version is needed anyway, it would be nice to get it right.
I am sorry, I am probably very late with this (although mentioned such a possibility a few days ago). I've made an ad-hoc test and observed that the module which is auto-loaded for GetProcAddress() on forwarded export makes the dependency loaded in the same DDAG node on Windows. So this looks like it directly interferes with what this series is trying to do. Shouldn't we implement it the same way (maybe even handling cyclic dependencies with DDAG node sharing aside)? Otherwise these complicated-looking changes might be moving this reference tracking in the wrong direction.
On Thu Feb 20 16:23:15 2025 +0000, Paul Gofman wrote:
I am sorry, I am probably very late with this (although mentioned such a possibility a few days ago). I've made an ad-hoc test and observed that the module which is auto-loaded for GetProcAddress() on forwarded export makes the dependency loaded in the same DDAG node on Windows. So this looks like it directly interferes with what this series is trying to do. Shouldn't we implement it the same way (maybe even handling cyclic dependencies with DDAG node sharing aside)? Otherwise these complicated-looking changes might be moving this reference tracking in the wrong direction.
Uh, sorry, scratch that, I've got the test on Windows wrong with a missing dll... it is all right, it is loaded in a different node.
-
Jacek Caban (＠jacek) -
Jinoh Kang
-
Jinoh Kang (＠iamahuman)
-
Paul Gofman (＠gofman)