[PATCH v8 0/5] MR7: ntdll: Properly track refcount with forwarded exports.
This fixes https://bugs.winehq.org/show_bug.cgi?id=52094. -- v8: ntdll: Properly track refcount with forwarded exports. ntdll: Don't re-add a module dependency if it already exists. ntdll: Remove some unnecessary NULL checks for current_importer. ntdll: Set export forwarder DLL as the dynamic importer in LdrGetProcedureAddress(). ntdll: Wrap current_modref variable in a new structure. https://gitlab.winehq.org/wine/wine/-/merge_requests/7
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Prepare for adding context information (e.g. static or dynamic) about the current importer, which is needed for correct RelayFromExclude handling. --- dlls/ntdll/loader.c | 70 ++++++++++++++++++++++++++++++--------------- 1 file changed, 47 insertions(+), 23 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 0c25fe14133..ffee8e8d68c 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -184,9 +184,16 @@ static RTL_BITMAP tls_bitmap; static RTL_BITMAP tls_expansion_bitmap; static WINE_MODREF *cached_modref; -static WINE_MODREF *current_modref; static WINE_MODREF *last_failed_modref; +struct importer +{ + struct importer *prev; + WINE_MODREF *modref; +}; + +static struct importer *current_importer; + 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 ); @@ -504,6 +511,20 @@ static ULONG_PTR allocate_stub( const char *dll, const char *name ) static inline ULONG_PTR allocate_stub( const char *dll, const char *name ) { return 0xdeadbeef; } #endif /* __i386__ */ +/* The loader_section must be locked while calling this function. */ +static void push_importer( struct importer *importer, WINE_MODREF *modref ) +{ + importer->modref = modref; + importer->prev = current_importer; + current_importer = importer; +} + +/* The loader_section must be locked while calling this function. */ +static void pop_importer( struct importer *importer ) +{ + current_importer = importer->prev; +} + /* call ldr notifications */ static void call_ldr_notifications( ULONG reason, LDR_DATA_TABLE_ENTRY *module ) { @@ -738,7 +759,7 @@ static NTSTATUS build_import_name( WCHAR buffer[256], const char *import, int le { const API_SET_NAMESPACE *map = NtCurrentTeb()->Peb->ApiSetMap; const API_SET_NAMESPACE_ENTRY *entry; - const WCHAR *host = current_modref ? current_modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *host = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; UNICODE_STRING str; while (len && import[len-1] == ' ') len--; /* remove trailing spaces */ @@ -922,9 +943,9 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS if (load_dll( load_path, mod_name, 0, &wm, imp->system ) == STATUS_SUCCESS && !(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) { - if (!imports_fixup_done && current_modref) + if (!imports_fixup_done && current_importer) { - add_module_dependency( current_modref->ldr.DdagNode, wm->ldr.DdagNode ); + add_module_dependency( current_importer->modref->ldr.DdagNode, wm->ldr.DdagNode ); } else if (process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) { @@ -992,12 +1013,12 @@ static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY if (TRACE_ON(snoop)) { - const WCHAR *user = current_modref ? current_modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *user = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; proc = SNOOP_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } if (TRACE_ON(relay)) { - const WCHAR *user = current_modref ? current_modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *user = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; proc = RELAY_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } return proc; @@ -1090,7 +1111,8 @@ void * WINAPI RtlFindExportedRoutineByName( HMODULE module, const char *name ) */ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LPCWSTR load_path, WINE_MODREF **pwm ) { - BOOL system = current_modref->system || (current_modref->ldr.Flags & LDR_WINE_INTERNAL); + struct importer *importer = current_importer; + BOOL system = importer->modref->system || (importer->modref->ldr.Flags & LDR_WINE_INTERNAL); NTSTATUS status; WINE_MODREF *wmImp; HMODULE imp_mod; @@ -1125,10 +1147,10 @@ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LP { if (status == STATUS_DLL_NOT_FOUND) ERR("Library %s (which is needed by %s) not found\n", - name, debugstr_w(current_modref->ldr.FullDllName.Buffer)); + name, debugstr_w(importer->modref->ldr.FullDllName.Buffer)); else ERR("Loading library %s (which is needed by %s) failed (error %lx).\n", - name, debugstr_w(current_modref->ldr.FullDllName.Buffer), status); + name, debugstr_w(importer->modref->ldr.FullDllName.Buffer), status); return FALSE; } @@ -1161,7 +1183,7 @@ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LP thunk_list->u1.Function = allocate_stub( name, (const char*)pe_name->Name ); } WARN(" imported from %s, allocating stub %p\n", - debugstr_w(current_modref->ldr.FullDllName.Buffer), + debugstr_w(importer->modref->ldr.FullDllName.Buffer), (void *)thunk_list->u1.Function ); import_list++; thunk_list++; @@ -1181,7 +1203,7 @@ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LP { thunk_list->u1.Function = allocate_stub( name, IntToPtr(ordinal) ); WARN("No implementation for %s.%d imported from %s, setting to %p\n", - name, ordinal, debugstr_w(current_modref->ldr.FullDllName.Buffer), + name, ordinal, debugstr_w(importer->modref->ldr.FullDllName.Buffer), (void *)thunk_list->u1.Function ); } TRACE_(imports)("--- Ordinal %s.%d = %p\n", name, ordinal, (void *)thunk_list->u1.Function ); @@ -1197,7 +1219,7 @@ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LP { thunk_list->u1.Function = allocate_stub( name, (const char*)pe_name->Name ); WARN("No implementation for %s.%s imported from %s, setting to %p\n", - name, pe_name->Name, debugstr_w(current_modref->ldr.FullDllName.Buffer), + name, pe_name->Name, debugstr_w(importer->modref->ldr.FullDllName.Buffer), (void *)thunk_list->u1.Function ); } TRACE_(imports)("--- %s %s.%d = %p\n", @@ -1396,21 +1418,21 @@ static void free_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) */ static NTSTATUS fixup_imports_ilonly( WINE_MODREF *wm, LPCWSTR load_path, void **entry ) { + struct importer importer; NTSTATUS status; void *proc; const char *name; - WINE_MODREF *prev, *imp; + WINE_MODREF *imp; if (!(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) return STATUS_SUCCESS; /* already done */ wm->ldr.Flags &= ~LDR_DONT_RESOLVE_REFS; - prev = current_modref; - current_modref = wm; + push_importer( &importer, wm ); assert( !wm->ldr.DdagNode->Dependencies.Tail ); if (!(status = load_dll( load_path, L"mscoree.dll", 0, &imp, FALSE )) && !add_module_dependency_after( wm->ldr.DdagNode, imp->ldr.DdagNode, NULL )) status = STATUS_NO_MEMORY; - current_modref = prev; + pop_importer( &importer ); if (status) { ERR( "mscoree.dll not found, IL-only binary %s cannot be loaded\n", @@ -1437,7 +1459,8 @@ static NTSTATUS fixup_imports( WINE_MODREF *wm, LPCWSTR load_path ) { const IMAGE_IMPORT_DESCRIPTOR *imports; SINGLE_LIST_ENTRY *dep_after; - WINE_MODREF *prev, *imp; + struct importer importer; + WINE_MODREF *imp; int i, nb_imports; DWORD size; NTSTATUS status; @@ -1463,8 +1486,7 @@ static NTSTATUS fixup_imports( WINE_MODREF *wm, LPCWSTR load_path ) /* load the imported modules. They are automatically * added to the modref list of the process. */ - prev = current_modref; - current_modref = wm; + push_importer( &importer, wm ); status = STATUS_SUCCESS; for (i = 0; i < nb_imports; i++) { @@ -1474,7 +1496,7 @@ static NTSTATUS fixup_imports( WINE_MODREF *wm, LPCWSTR load_path ) else if (imp && imp->ldr.DdagNode != node_ntdll && imp->ldr.DdagNode != node_kernel32) add_module_dependency_after( wm->ldr.DdagNode, imp->ldr.DdagNode, dep_after ); } - current_modref = prev; + pop_importer( &importer ); if (wm->ldr.ActivationContext) RtlDeactivateActivationContext( 0, cookie ); return status; } @@ -1755,8 +1777,9 @@ static NTSTATUS process_attach( LDR_DDAG_NODE *node, LPVOID lpReserved ) /* Call DLL entry point */ if (status == STATUS_SUCCESS) { - WINE_MODREF *prev = current_modref; - current_modref = wm; + struct importer importer; + + push_importer( &importer, wm ); call_ldr_notifications( LDR_DLL_NOTIFICATION_REASON_LOADED, &wm->ldr ); status = MODULE_InitDLL( wm, DLL_PROCESS_ATTACH, lpReserved ); @@ -1773,7 +1796,8 @@ static NTSTATUS process_attach( LDR_DDAG_NODE *node, LPVOID lpReserved ) last_failed_modref = wm; WARN("Initialization of %s failed\n", debugstr_w(wm->ldr.BaseDllName.Buffer)); } - current_modref = prev; + + pop_importer( &importer ); } if (wm->ldr.ActivationContext) RtlDeactivateActivationContext( 0, cookie ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> This matches the Windows GetProcAddress() behavior when handling dynamic module dependencies. To avoid breaking WINEDEBUG=+relay, flag dynamic importers (`is_dynamic = TRUE`) and explicitly ignore them when testing for RelayFromExclude modules. Otherwise, GetProcAddress() on kernel32 export forwarders (which comprise most of the exports) will be recognized as relaying *from* kernel32 itself (instead of the actual importer) and will be subsequently excluded from WINEDEBUG=+relay due to the default `RelayFromExclude` which includes `kernel32`. The current behavior is to treat it as relaying from the actual importer, not kernel32, so this doen't become a problem. This bit is true for all export forwarder DLLs in general, and also affects RelayFromInclude as well as SnoopFrom{Exclude,Include} (+snoop). --- dlls/ntdll/loader.c | 38 ++++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index ffee8e8d68c..2fa31b34d4d 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -190,6 +190,7 @@ struct importer { struct importer *prev; WINE_MODREF *modref; + BOOL is_dynamic; }; static struct importer *current_importer; @@ -512,9 +513,10 @@ static inline ULONG_PTR allocate_stub( const char *dll, const char *name ) { ret #endif /* __i386__ */ /* The loader_section must be locked while calling this function. */ -static void push_importer( struct importer *importer, WINE_MODREF *modref ) +static void push_importer( struct importer *importer, WINE_MODREF *modref, BOOL is_dynamic ) { importer->modref = modref; + importer->is_dynamic = is_dynamic; importer->prev = current_importer; current_importer = importer; } @@ -525,6 +527,20 @@ static void pop_importer( struct importer *importer ) current_importer = importer->prev; } +/* The loader_section must be locked while calling this function. */ +static const WCHAR *get_last_static_importer_name(void) +{ + struct importer *importer; + for (importer = current_importer; importer != NULL; importer = importer->prev) + { + if (!importer->is_dynamic) + { + return importer->modref->ldr.BaseDllName.Buffer; + } + } + return NULL; +} + /* call ldr notifications */ static void call_ldr_notifications( ULONG reason, LDR_DATA_TABLE_ENTRY *module ) { @@ -1013,12 +1029,12 @@ static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY if (TRACE_ON(snoop)) { - const WCHAR *user = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *user = get_last_static_importer_name(); proc = SNOOP_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } if (TRACE_ON(relay)) { - const WCHAR *user = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *user = get_last_static_importer_name(); proc = RELAY_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } return proc; @@ -1427,7 +1443,7 @@ static NTSTATUS fixup_imports_ilonly( WINE_MODREF *wm, LPCWSTR load_path, void * if (!(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) return STATUS_SUCCESS; /* already done */ wm->ldr.Flags &= ~LDR_DONT_RESOLVE_REFS; - push_importer( &importer, wm ); + push_importer( &importer, wm, FALSE ); assert( !wm->ldr.DdagNode->Dependencies.Tail ); if (!(status = load_dll( load_path, L"mscoree.dll", 0, &imp, FALSE )) && !add_module_dependency_after( wm->ldr.DdagNode, imp->ldr.DdagNode, NULL )) @@ -1486,7 +1502,7 @@ static NTSTATUS fixup_imports( WINE_MODREF *wm, LPCWSTR load_path ) /* load the imported modules. They are automatically * added to the modref list of the process. */ - push_importer( &importer, wm ); + push_importer( &importer, wm, FALSE ); status = STATUS_SUCCESS; for (i = 0; i < nb_imports; i++) { @@ -1779,7 +1795,7 @@ static NTSTATUS process_attach( LDR_DDAG_NODE *node, LPVOID lpReserved ) { struct importer importer; - push_importer( &importer, wm ); + push_importer( &importer, wm, FALSE ); call_ldr_notifications( LDR_DLL_NOTIFICATION_REASON_LOADED, &wm->ldr ); status = MODULE_InitDLL( wm, DLL_PROCESS_ATTACH, lpReserved ); @@ -2063,8 +2079,14 @@ 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 ) - : find_ordinal_export( module, exports, exp_size, ord - exports->Base, NULL ); + struct importer importer; + void *proc; + + push_importer( &importer, wm, TRUE ); + proc = name ? find_named_export( module, exports, exp_size, name->Buffer, -1, NULL ) + : find_ordinal_export( module, exports, exp_size, ord - exports->Base, NULL ); + pop_importer( &importer ); + if (proc) { *address = proc; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> current_importer is now set by all callers of build_import_name, find_forwarded_export, and find_ordinal_export. --- 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 2fa31b34d4d..9e7da8ae1b5 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -775,7 +775,7 @@ static NTSTATUS build_import_name( WCHAR buffer[256], const char *import, int le { const API_SET_NAMESPACE *map = NtCurrentTeb()->Peb->ApiSetMap; const API_SET_NAMESPACE_ENTRY *entry; - const WCHAR *host = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *host = current_importer->modref->ldr.BaseDllName.Buffer; UNICODE_STRING str; while (len && import[len-1] == ' ') len--; /* remove trailing spaces */ @@ -959,7 +959,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS if (load_dll( load_path, mod_name, 0, &wm, imp->system ) == STATUS_SUCCESS && !(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) { - if (!imports_fixup_done && current_importer) + if (!imports_fixup_done) { add_module_dependency( current_importer->modref->ldr.DdagNode, wm->ldr.DdagNode ); } -- 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 9e7da8ae1b5..17f4595c7d6 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -875,6 +875,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 */ @@ -883,6 +898,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 */ + LDR_DATA_TABLE_ENTRY *mod = CONTAINING_RECORD( to->Modules.Flink, LDR_DATA_TABLE_ENTRY, NodeModuleLink ); + WINE_MODREF *wm = CONTAINING_RECORD( mod, WINE_MODREF, ldr ); + LdrUnloadDll( wm->ldr.DllBase ); + 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> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52094 --- dlls/kernel32/tests/loader.c | 2 -- dlls/ntdll/loader.c | 17 ++++++++++++----- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 2c7cc784be4..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) 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) 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 17f4595c7d6..5cbb2a61c32 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -983,11 +983,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS if (load_dll( load_path, mod_name, 0, &wm, imp->system ) == STATUS_SUCCESS && !(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) { - if (!imports_fixup_done) - { - add_module_dependency( current_importer->modref->ldr.DdagNode, wm->ldr.DdagNode ); - } - else if (process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) + if (imports_fixup_done && process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) { LdrUnloadDll( wm->ldr.DllBase ); wm = NULL; @@ -1001,6 +997,11 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS return NULL; } } + else + { + if (wm->ldr.LoadCount != -1) wm->ldr.LoadCount++; + } + if ((exports = RtlImageDirectoryEntryToData( wm->ldr.DllBase, TRUE, IMAGE_DIRECTORY_ENTRY_EXPORT, &exp_size ))) { @@ -1020,6 +1021,12 @@ 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->ldr.DdagNode != node_ntdll && wm->ldr.DdagNode != node_kernel32) + { + add_module_dependency( current_importer->modref->ldr.DdagNode, wm->ldr.DdagNode ); + wm = NULL; + } + if (wm) LdrUnloadDll( wm->ldr.DllBase ); return proc; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=151123 Your paranoid android. === w8 (32 bit report) === kernel32: loader.c:716: Test failed: 1291: wrong status c000007b/c000035a loader.c:716: Test failed: 1300: wrong status c000007b/c000035a loader.c:716: Test failed: 1305: wrong status c000007b/c000035a loader.c:716: Test failed: 1312: wrong status c000007b/c000035a loader.c:716: Test failed: 1319: wrong status c000007b/c000035a loader.c:716: Test failed: 1327: wrong status c000007b/c000035a loader.c:716: Test failed: 1334: wrong status c000007b/c000035a loader.c:716: Test failed: 1344: wrong status c000007b/c000035a loader.c:716: Test failed: 1349: wrong status c000007b/c000035a loader.c:716: Test failed: 1354: wrong status c000007b/c000035a loader.c:716: Test failed: 1359: wrong status c000007b/c000035a loader.c:716: Test failed: 1364: wrong status c000007b/c000035a
Jacek Caban (@jacek) commented about dlls/ntdll/loader.c:
return NULL; } } + else + { + if (wm->ldr.LoadCount != -1) wm->ldr.LoadCount++;
Unless I'm missing something, this means that we'd increase `LoadCount` for each lookup, meaning that multiple `GetProcAddress` would keep incrementing it. Is that intentional? My guess would be to do that when adding a new dependency. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_93656
Jacek Caban (@jacek) commented about dlls/ntdll/loader.c:
if (TRACE_ON(snoop)) { - const WCHAR *user = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *user = get_last_static_importer_name(); proc = SNOOP_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } if (TRACE_ON(relay)) { - const WCHAR *user = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *user = get_last_static_importer_name();
With the current approach, is relay exclude from effective even for `GetProcAddress` calls inside `DllMain`? My guess is that this is more of a side effect than an intentional behavior, and not something we necessarily need to preserve. Since this mechanism can't be fully precise (`DllMain` might call functions from a different module) and we don't use such `GetProcAddress` calls inside Wine DLLs, where this feature is typically applied, I'm wondering if we could simplify it by treating `is_dynamic` as a `NULL` user here. Perhaps we could even pass an argument like `user_modref` and exclude `LdrGetProcedureAddress` from the relay exclude from mechanism by passing `NULL`? I haven’t tried implementing this to confirm, but I’m curious if you’ve considered it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_93657
On Tue Feb 4 23:14:59 2025 +0000, Jacek Caban wrote:
Unless I'm missing something, this means that we'd increase `LoadCount` for each lookup, meaning that multiple `GetProcAddress` would keep incrementing it. Is that intentional? My guess would be to do that when adding a new dependency. In patch 4/5[^1], `add_module_dependency` detects existing reference and decrements[^d] LoadCount back to the previous value. This is true for both static and dynamic dependencies.
In fact static ref is what prompted the need for the deduplication in the first place. Kernel32/kernelbase together has about a hundred export forwarders to ntdll, and not deduplicating them would result in a nontrivial memory bloat. So, in a sense, `add_module_dependency` holds the final authority over whether we want to actually increase LoadCount or not. In case it's not called because it forwards to ntdll or kernel32, `wm` is simply released[^p] at the end of the function anyway. [^1]: "ntdll: Don't re-add a module dependency if it already exists." [^d]: Indirectly via `LdrUnloadDll`. [^p]: kernel32 and ntdll are effectively pinned DLLs, so we don't keep their incoming edges. This is consistent with the rest of the loader logic. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_93693
On Wed Feb 5 10:38:24 2025 +0000, Jinoh Kang wrote:
In patch 4/5[^1], `add_module_dependency` detects existing reference and decrements[^d] LoadCount back to the previous value. This is true for both static and dynamic dependencies. In fact static ref is what prompted the need for the deduplication in the first place. Kernel32/kernelbase together has about a hundred export forwarders to ntdll, and not deduplicating them would result in not *just* extra LoadCount but also allocation of extra LDR\_DEPENDENCY edges, a nontrivial memory bloat. So, in a sense, `add_module_dependency` holds the final authority over whether we want to actually increase LoadCount or not. For duplicate references, LoadCount is incremented (AddRef'd) by either `load_dll` or the else branch, and then decremented back (Release'd) on dupe detection, resulting in a net delta of 0. In case `add_module_dependency` is not called because it forwards to ntdll or kernel32, `wm` is simply released[^p] at the end of the function anyway, also a net delta of 0. [^1]: "ntdll: Don't re-add a module dependency if it already exists." [^d]: Indirectly via `LdrUnloadDll`. [^p]: kernel32 and ntdll are effectively pinned DLLs, so we don't keep their incoming edges. This is consistent with the rest of the loader logic. Besides, extra refcounts would have resulted in test failure, because the test calls GetProcAddress() twice.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_93697
With the current approach, is relay exclude from effective even for `GetProcAddress` calls inside `DllMain`? My guess is that this is more of a side effect than an intentional behavior, and not something we necessarily need to preserve.
I guess. To be honest I have very little idea how developers (or users) use +relay or +snoop.
Since this mechanism can't be fully precise (`DllMain` might call functions from a different module) and we don't use such `GetProcAddress` calls inside Wine DLLs, where this feature is typically applied, I'm wondering if we could simplify it by treating `is_dynamic` as a `NULL` user here.
Like this? ```suggestion:-0+0 const WCHAR *user = !current_importer->is_dynamic ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; ```
Perhaps we could even pass an argument like `user_modref`
You mean turning the `current_importer` global into a local parameter with such name? Here's a graph for how `current_modref` value is being passed around: ```mermaid graph TB fixup_imports_ilonly["fixup_imports_ilonly<br><em style='color:yellow'>(directly sets current_modref)</em>"] fixup_imports["fixup_imports<br><em style='color:yellow'>(directly sets current_modref)</em>"] process_attach["process_attach<br><em style='color:yellow'>(directly sets current_modref)</em>"] fixup_imports_ilonly --> load_dll fixup_imports --> import_dll process_attach --> MODULE_InitDLL MODULE_InitDLL -.-> DllMain import_dll["import_dll<br><em style='color:lime'>(directly uses current_modref)</em>"] import_dll --> build_import_name["build_import_name<br><em style='color:lime'>(directly uses current_modref)</em>"] import_dll --> load_dll import_dll --> find_ordinal_export["find_ordinal_export<br><em style='color:lime'>(uses current_modref for relay)</em>"] import_dll --> find_named_export find_ordinal_export --> find_forwarded_export["find_forwarded_export<br><em style='color:lime'>(directly uses current_modref)</em>"] find_named_export --> find_ordinal_export find_forwarded_export --> find_ordinal_export find_forwarded_export --> find_named_export find_forwarded_export --> build_import_name style DllMain color:red; ``` The arrows indicate how the `current_modref` value flows from which caller to which callee. If we make it a explicit parameter, all the edges above will be turned into an argument. This is a nontrivlal refactoring and I'm slightly uncomfortable with any possible regressions it might cause, especially around passing `current_modref` through user code (e.g., DllMain).
and exclude `LdrGetProcedureAddress` from the relay exclude from mechanism by passing `NULL`? I haven’t tried implementing this to confirm, but I’m curious if you’ve considered it.
`current_modref` is not just for +relay. It's also needed for builtin/native differentiation for handling DLL paths (`import_dll`), accurate dependency tracking between modules (`find_forwarded_export`), and building import names (`build_import_name`). Overall, !364 has a nice graph that visualizes what this PR solves. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_93714
So, in a sense, `add_module_dependency` holds the final authority over whether we want to actually increase LoadCount or not.
I see, thanks for explanation. It would be easier to follow if `add_module_dependency` would be responsible for increasing `LoadCount` instead, but it would mean that we'd initially create a module with 0 `LoadCount`, creating more corner cases, so in the end something like would likely not be worth it. `LdrUnloadDll` is rather a heavy tool just to decrease the count in that patch. Since we know that we have another edge, we know that we won't actually need to unload the DLL, so just a direct decrement of `LoadCount` should be enough? I'm mentioning that for consideration, it's not a strong opinion. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_93812
Like this?
Yes, that's what I meant.
You mean turning the `current_importer` global into a local parameter with such name?
Yes. I originally meant it only for relay's needs, but if we can do that for all cases that's even better. Here is a draft of something like that: https://gitlab.winehq.org/jacek/wine/-/commits/modref. It's barely tested and I'm not yet sure about it (esp. apisets), but I think it makes things easier to follow. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_93816
Since this mechanism can't be fully precise (`DllMain` might call functions from a different module) and we don't use such `GetProcAddress` calls inside Wine DLLs, where this feature is typically applied, I'm wondering if we could simplify it by treating `is_dynamic` as a `NULL` user here.
On second look, there seems to be multiple Wine builtin DLLs that *do* call GetProcAddress() in DllMain: - shlwapi: https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... - wintab32: https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... There may be other DLLs that I have missed (e.g., because GetProcAddress is called indirectly via some helper function in the same DLL). Maybe something needs to be done with these DLLs before we can get rid of dynamic dependency handling? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_93899
participants (4)
-
Jacek Caban (＠jacek) -
Jinoh Kang
-
Jinoh Kang (＠iamahuman)
-
Marvin