[PATCH v10 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; ``` -- v10: 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..e51ea7197f4 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) + { + if (!(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 | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index e51ea7197f4..c0cd5d41a05 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_from == from && dep->dependency_to == to) return dep; + } + + return NULL; +} + /********************************************************************** * add_module_dependency_after */ @@ -845,6 +860,16 @@ static BOOL add_module_dependency_after( LDR_DDAG_NODE *from, LDR_DDAG_NODE *to, { LDR_DEPENDENCY *dep; + if ((dep = find_module_dependency( from, to ))) + { + /* Dependency already exists; consume the module reference stolen from the caller */ + LDR_DATA_TABLE_ENTRY *mod = CONTAINING_RECORD( to->Modules.Flink, LDR_DATA_TABLE_ENTRY, NodeModuleLink ); + WINE_MODREF *wm = CONTAINING_RECORD( mod, WINE_MODREF, ldr ); + 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 c0cd5d41a05..cce46090754 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -979,17 +979,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_owned_ref) + else if (wm->ldr.DdagNode != node_ntdll && wm->ldr.DdagNode != node_kernel32) { if (!(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) { 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..d0a2c514136 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 && exporter_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 && exporter_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 cce46090754..2de180558c3 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)) { - 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 2de180558c3..0f229a5c257 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 ) @@ -967,9 +969,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 d0a2c514136..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 && exporter_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 && exporter_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 0f229a5c257..f354ddb1ab9 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 | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index f354ddb1ab9..8682b1d138c 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,17 +987,10 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS { if (!(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) { - 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) { @@ -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
v10: - Fix handling cyclical references in export forwarders - Only call LdrUnloadDll() when necessary -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_94403
participants (2)
-
Jinoh Kang -
Jinoh Kang (＠iamahuman)