[PATCH v12 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; ``` -- v12: 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 | 48 +++++++++++++++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 3872f2b237b..04a68116c1e 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,27 +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 && - !(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) - { - 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 ))) { @@ -958,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 && !(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) + { + 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 04a68116c1e..7424b1898a2 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 | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 7424b1898a2..7ecc17e7cae 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -980,15 +980,17 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS } else { - if (wm_owned_ref && !(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) + if (!(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS) && + 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 2c7cc784be4..697bb5a8944 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -2777,7 +2777,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() ); } @@ -2789,7 +2789,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 7ecc17e7cae..b34a8a8d02f 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -983,7 +983,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS if (!(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS) && 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 b34a8a8d02f..f5fff86f5d2 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) @@ -1014,7 +1018,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 ); @@ -1035,12 +1040,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; @@ -1077,7 +1082,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 ); @@ -1088,12 +1094,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 ); } @@ -1220,7 +1226,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) ); @@ -1236,7 +1242,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 ); @@ -2073,8 +2079,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 697bb5a8944..5ea169beb51 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -2777,7 +2777,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() ); } @@ -2789,7 +2788,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 f5fff86f5d2..e1b1db3b273 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -2079,8 +2079,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 e1b1db3b273..4e71e217eba 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 */ @@ -987,19 +987,12 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS if (!(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS) && 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; @@ -1040,12 +1033,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
v12: - Prevent accidental unload of ntdll and kernel32 if ever possible - Restructure code a bit -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_94445
Jacek Caban (@jacek) commented about dlls/ntdll/loader.c:
+ 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; + } Is this about modules loaded with `LDR_DONT_RESOLVE_REFS`? Should `find_forwarded_export` still resolve them in that case?
With your series complete, we always have an importer, so unless I’m missing something, this seems to be the only scenario. I’m trying to understand the logic here: if `find_forwarded_export` should still load such dependencies, I can see the reasoning behind skipping `process_attach` in that case, but binding the dependency still seems possible. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_94631
On Fri Feb 14 17:21:09 2025 +0000, Jacek Caban wrote:
Is this about modules loaded with `LDR_DONT_RESOLVE_REFS`? Should `find_forwarded_export` still resolve them in that case? With your series complete, we always have an importer, so unless I’m missing something, this seems to be the only scenario. I’m trying to understand the logic here: if `find_forwarded_export` should still load such dependencies, I can see the reasoning behind skipping `process_attach` in that case, but binding the dependency still seems possible. I assume it's there to detect dependency cycles. If there is a dep cycle like `A --(import)--> B.proc2 --(forward)--> A.proc3` and we are processing the forward, then `A` would have `LDR_DONT_RESOLVE_REFS` set since module `A` hasn't completed `fixup_imports` (it's still resolving import of B.proc2). In that case adding a module dependency would result in a directed cycle `A --> B --> A`, which is unacceptable in a DAG.
The logic was there without comment before this patch, so I kept it as-is after this patch. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_94644
I assume it's there to detect dependency cycles. If there is a dep cycle like `A --(import)--> B.proc2 --(forward)--> A.proc3` and we are processing the forward, then `A` would have `LDR_DONT_RESOLVE_REFS` set since module `A` hasn't completed `fixup_imports` (it's still resolving import of B.proc2). In that case adding a module dependency would result in a directed cycle `A --> B --> A`, which is unacceptable in a DAG.
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. That said, while looking into this, I noticed that our handling of LoadCount in cyclic dependencies seemed suspicious, so I tested it. I think it's buggy. For example, if a.dll has a cyclic dependency with b.dll, and you dynamically load a.dll, take a reference to b.dll, and then unload a.dll, we would unload a.dll while keeping b.dll loaded. On Windows, neither would be unloaded until the b.dll reference is released. This isn’t strictly related to this MR, but I think it's worth mentioning.
The logic was there without comment before this patch, so I kept it as-is after this patch.
I believe it's better to have no comment than a misleading one, so if we add one, we should ensure it's accurate. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_94845
On Sun Feb 16 18:30:32 2025 +0000, Jacek Caban wrote:
I assume it's there to detect dependency cycles. If there is a dep cycle like `A --(import)--> B.proc2 --(forward)--> A.proc3` and we are processing the forward, then `A` would have `LDR_DONT_RESOLVE_REFS` set since module `A` hasn't completed `fixup_imports` (it's still resolving import of B.proc2). In that case adding a module dependency would result in a directed cycle `A --> B --> A`, which is unacceptable in a DAG. 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. That said, while looking into this, I noticed that our handling of LoadCount in cyclic dependencies seemed suspicious, so I tested it. I think it's buggy. For example, if a.dll has a cyclic dependency with b.dll, and you dynamically load a.dll, take a reference to b.dll, and then unload a.dll, we would unload a.dll while keeping b.dll loaded. On Windows, neither would be unloaded until the b.dll reference is released. This isn’t strictly related to this MR, but I think it's worth mentioning. The logic was there without comment before this patch, so I kept it as-is after this patch. I believe it's better to have no comment than a misleading one, so if we add one, we should ensure it's accurate. I'm not sure I understand. Are you saying that the directed graph formed by D*dag*Nodes isn't acyclic? That it is a misnomer?
Also, reference counts can only deal with acyclic (strong) references in nature. Cycles should be broken with "weak" references (e.g., adding an edge without incrementing LoadCount), or we shouldn't use "LoadCount" for dependency edges at all. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_94848
Are you saying that the directed graph formed by D_dag_Nodes isn't acyclic?
Yes (at least with the current implementation).
Also, reference counts can only deal with acyclic (strong) references in nature.
Yes, that's why we have the problem that I mentioned. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_94849
On Sun Feb 16 19:19:02 2025 +0000, Jacek Caban wrote:
Are you saying that the directed graph formed by D_dag_Nodes isn't acyclic? Yes (at least with the current implementation). Also, reference counts can only deal with acyclic (strong) references in nature. Yes, that's why we have the problem that I mentioned. Well, there's one problem: even if we don't call `process_attach` directly, it will still be called at the end of the module load via `walk_node_dependencies`. So I'm not sure how to go forward. Maybe move it (the `LDR_DONT_RESOLVE_REFS` check) into `process_attach` itself?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_94854
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_94855
participants (4)
-
Jacek Caban (＠jacek) -
Jinoh Kang
-
Jinoh Kang (＠iamahuman)
-
Paul Gofman (＠gofman)