[PATCH v13 0/7] MR7: ntdll: Properly track refcount with forwarded exports.
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; ``` -- v13: 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: 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: Register module dependency for export forwarders only after successful resolution. https://gitlab.winehq.org/wine/wine/-/merge_requests/7
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/ntdll/loader.c | 47 +++++++++++++++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 5a582ca14bb..68b5cccee8a 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_owned_ref; if (!end) return NULL; if (build_import_name( importer, mod_name, forward, end - forward )) return NULL; @@ -918,26 +919,22 @@ 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; } + + /* load_dll() returns a new (owned) reference */ + wm_owned_ref = TRUE; } + else + { + /* find_basename_module() returns a borrowed reference */ + wm_owned_ref = FALSE; + } + if ((exports = RtlImageDirectoryEntryToData( wm->ldr.DllBase, TRUE, IMAGE_DIRECTORY_ENTRY_EXPORT, &exp_size ))) { @@ -957,6 +954,28 @@ 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_owned_ref) + { + if (!imports_fixup_done && importer) + { + /* Prepare for the callee stealing the reference */ + wm_owned_ref = FALSE; + add_module_dependency( importer->ldr.DdagNode, wm->ldr.DdagNode ); + } + else if (process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) + { + proc = NULL; + } + } + if (proc && wm_owned_ref) + { + /* Owned, but no way to bind to a dependency; leak the reference instead */ + wm_owned_ref = FALSE; + } + } + if (wm_owned_ref) LdrUnloadDll( wm->ldr.DllBase ); return proc; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7
From: Jinoh Kang <jinoh.kang.kr(a)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 68b5cccee8a..5ea71ed1206 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; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7
From: Jinoh Kang <jinoh.kang.kr(a)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 | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 5ea71ed1206..4fe7ada4c16 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -980,15 +980,16 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS } else { - if (wm_owned_ref) + if (wm->ldr.DdagNode != node_ntdll && wm->ldr.DdagNode != node_kernel32) { if (!imports_fixup_done && importer) { /* Prepare for the callee stealing the reference */ - wm_owned_ref = FALSE; + if (wm_owned_ref) wm_owned_ref = FALSE; + else if (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_owned_ref && process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) { proc = NULL; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7
From: Jinoh Kang <jinoh.kang.kr(a)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 4fe7ada4c16..bfa5b3385fb 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -982,7 +982,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_owned_ref) wm_owned_ref = FALSE; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/ntdll/loader.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index bfa5b3385fb..d3265d27cac 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 ) @@ -966,9 +968,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, FALSE ); } 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, FALSE ); } if (!proc) @@ -1013,7 +1017,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 ); @@ -1034,12 +1039,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; @@ -1076,7 +1081,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 ); @@ -1087,12 +1093,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 ); } @@ -1219,7 +1225,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) ); @@ -1235,7 +1241,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 ); @@ -2076,8 +2082,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; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7
From: Jinoh Kang <jinoh.kang.kr(a)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 d3265d27cac..c06bc3f3574 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -2082,8 +2082,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; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/ntdll/loader.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index c06bc3f3574..52596930dea 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 */ @@ -986,19 +986,12 @@ 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_owned_ref) wm_owned_ref = FALSE; - else if (wm->ldr.LoadCount != -1) wm->ldr.LoadCount++; - add_module_dependency( importer->ldr.DdagNode, wm->ldr.DdagNode ); - } - else if (wm_owned_ref && process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) - { - proc = NULL; - } + /* Prepare for the callee stealing the reference */ + if (wm_owned_ref) wm_owned_ref = FALSE; + else if (wm->ldr.LoadCount != -1) wm->ldr.LoadCount++; + add_module_dependency( importer->ldr.DdagNode, wm->ldr.DdagNode ); } - if (proc && wm_owned_ref) + if (wm_owned_ref) { /* Owned, but no way to bind to a dependency; leak the reference instead */ wm_owned_ref = FALSE; @@ -1039,12 +1032,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; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7
On Tue Feb 18 23:58:22 2025 +0000, Paul Gofman wrote:
What does a DAG have to do with this? The dependency graph isn’t acyclic. It can’t be, since circular dependencies are allowed. We already create cycles for regular imports, so I don’t see why forward imports should be treated any differently. Not sure if that is directly related, but I think the idea is that modules with cyclic dependencies should exist in one DDAG node while there should not be cyclic dependencies between DDAG nodes. E. g., user32, win32u and gdi32 get loaded in one DDAG node on Windows. This is currently not implemented and we always create a separate DDAG node for each module. Rebased, `LDR_DONT_RESOLVE_REFS` test is no longer part of the patch after !7347.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_95087
Jacek Caban (@jacek) commented about dlls/ntdll/loader.c:
+ /* Prepare for the callee stealing the reference */ + wm_owned_ref = FALSE; + add_module_dependency( importer->ldr.DdagNode, wm->ldr.DdagNode ); + } + else if (process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) + { + proc = NULL; + } + } + if (proc && wm_owned_ref) + { + /* Owned, but no way to bind to a dependency; leak the reference instead */ + wm_owned_ref = FALSE; + } + } + if (wm_owned_ref) LdrUnloadDll( wm->ldr.DllBase ); While placing it here avoids duplicating the `LdrUnloadDll` call in this commit, that won't be the case once the entire series is committed. In the end, it’s only useful in the branch handling the `!proc` case. I’d suggest moving it to the `!proc` branch (next to the `ERR`), duplicating it in the branch that handles process_attach failure, and removing the latter in the commit that eliminates the process_attach call. That way, you could also remove the need to reset `wm_owned_ref` above, simplifying the logic.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_95224
Jacek Caban (@jacek) commented about dlls/ntdll/loader.c:
WCHAR mod_name[256]; const char *end = strrchr(forward, '.'); FARPROC proc = NULL; + BOOL wm_owned_ref;
Initializing it to `FALSE` here would eliminate the need for the extra branch below. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_95225
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; 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;
I still hope we can simplify this. After giving it more thought, since relay behavior is ours to define, I don’t see an issue with adjusting it for dynamic resolution. Arguably, it makes sense to honor the importer in the is_dynamic case. I’d suggest dropping this change and simply accepting that the relay behavior changes in a later commit. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_95226
Looks mostly good to me now, thanks! I have a few comments for the final touches. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_95227
Jacek Caban (@jacek) commented about dlls/ntdll/loader.c:
{ if (wm->ldr.DdagNode != node_ntdll && wm->ldr.DdagNode != node_kernel32) { - if (importer) - { - /* Prepare for the callee stealing the reference */ - if (wm_owned_ref) wm_owned_ref = FALSE; - else if (wm->ldr.LoadCount != -1) wm->ldr.LoadCount++; - add_module_dependency( importer->ldr.DdagNode, wm->ldr.DdagNode ); - } - else if (wm_owned_ref && process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) - { - proc = NULL; - }
Actually, looking at this again, I think it breaks the case where `LdrGetProcedureAddress` triggers the DLL to load. With this change (or rather, the previous commit), we would no longer attach such a DLL, which seems incorrect. Maybe we do need is_dynamic for that after all. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_95326
On Wed Feb 19 18:41:31 2025 +0000, Jacek Caban wrote:
While placing it here avoids duplicating the `LdrUnloadDll` call in this commit, that won't be the case once the entire series is committed. In the end, it’s only useful in the branch handling the `!proc` case. I’d suggest moving it to the `!proc` branch (next to the `ERR`), duplicating it in the branch that handles process_attach failure, and removing the latter in the commit that eliminates the process_attach call. That way, you could also remove the need to reset `wm_owned_ref` above, simplifying the logic. Thanks for the insight.
The intention of `wm_owned_ref` is to explicitly document where exactly the ownership of `wm` is acquired, stolen, and released. It's a compromise between "full AddRef/Release-like smart pointer management" and "implicit refcount management not too visible in code, and may subtly break if the developer is unaware of the rules." I guess it's "only after successful resolution" part that's initiating the complexity. Let me drop it, and maybe re-introduce it in another MR with proper tests. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_95328
On Wed Feb 19 18:41:31 2025 +0000, Jacek Caban wrote:
Initializing it to `FALSE` here would eliminate the need for the extra branch below. I'm fine with initializing, but that means I would have to drop the comment regarding `wm_owned_ref = FALSE`.
Maybe I'll just rename it to `wm_loaded` so that it's not making any claims about proper memory management or anything. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_95329
On Wed Feb 19 18:41:32 2025 +0000, Jacek Caban wrote:
I still hope we can simplify this. After giving it more thought, since relay behavior is ours to define, I don’t see an issue with adjusting it for dynamic resolution. Arguably, it makes sense to honor the importer in the is_dynamic case. I’d suggest dropping this change and simply accepting that the relay behavior changes in a later commit. That would render relay useless.
The direct consequence is that we have no way to ignore calls coming direct from `kernel32`, without also losing the ability to trace `GetProcAddress( hKernel32, "<procname>" )` calls (which is a very common pattern ranging from OS version compatibility to hidden anticheat imports). Maybe we could implement RelayFromExclude in a better way, like checking the return address, but for the time being I don't think we should break an existing functionality too much. (Because, if relay/snoop is not used at all, perhaps we should drop it entirely.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_95330
On Thu Feb 20 11:51:19 2025 +0000, Jinoh Kang wrote:
That would render relay useless.[^1] The direct consequence is that we have no way to ignore calls coming direct from `kernel32`, without also losing the ability to trace _any_ `GetProcAddress( hKernel32, "<procname>" )` calls (which is a very common pattern ranging from OS version compatibility to hidden anticheat imports). Maybe we could implement RelayFromExclude in a better way, like checking the return address, but for the time being I don't think we should break an existing functionality too much. (Because, if relay/snoop is not used at all, perhaps we should drop it entirely.) [^1]: sorry for being a bit hyperbolic. maybe *partly*? See my comment about `process_attach`. It looks like we may need `is_dynamic` for another reason, making my comment here obsolete.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_95331
On Thu Feb 20 11:18:55 2025 +0000, Jacek Caban wrote:
Actually, looking at this again, I think it breaks the case where `LdrGetProcedureAddress` triggers the DLL to load. With this change (or rather, the previous commit), we would no longer attach such a DLL, which seems incorrect. Maybe we do need is_dynamic for that after all. That's probably solvable via calling `walk_node_dependencies( wm->ldr.DdagNode, process_attach, NULL );`, although maybe a bit inefficient and I haven't sorted out details.
(I also tend to think that calling `process_attach` *in the middle of* import resolution is a bad idea, when the graph is still changing. Seems fragile and easy way to introduce dangling pointers.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_95332
participants (3)
-
Jacek Caban (＠jacek) -
Jinoh Kang
-
Jinoh Kang (＠iamahuman)