This is a cleanup in preparation for !7. Originally, !7 extended the use of `current_modref`, but I think we could avoid that with a more direct approach. Most of these changes are no-ops.
For the relay handler, we no longer treat `GetProcAddress` calls inside `DllMain` as coming from a loaded DLL. I believe this was more of a side effect of how the code was structured rather than an intentional behavior that needs to be preserved.
One more use of `current_modref` remains, which will be removed by !7.
From: Jacek Caban jacek@codeweavers.com
--- dlls/ntdll/loader.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 0c25fe14133..b48a950e705 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -1088,9 +1088,10 @@ void * WINAPI RtlFindExportedRoutineByName( HMODULE module, const char *name ) * Import the dll specified by the given import descriptor. * The loader_section must be locked while calling this function. */ -static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LPCWSTR load_path, WINE_MODREF **pwm ) +static BOOL import_dll( WINE_MODREF *wm, const IMAGE_IMPORT_DESCRIPTOR *descr, LPCWSTR load_path, WINE_MODREF **pwm ) { - BOOL system = current_modref->system || (current_modref->ldr.Flags & LDR_WINE_INTERNAL); + HMODULE module = wm->ldr.DllBase; + BOOL system = wm->system || (wm->ldr.Flags & LDR_WINE_INTERNAL); NTSTATUS status; WINE_MODREF *wmImp; HMODULE imp_mod; @@ -1124,11 +1125,10 @@ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LP if (status) { 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)); + ERR("Library %s (which is needed by %s) not found\n", name, debugstr_w(wm->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(wm->ldr.FullDllName.Buffer), status); return FALSE; }
@@ -1161,7 +1161,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(wm->ldr.FullDllName.Buffer), (void *)thunk_list->u1.Function ); import_list++; thunk_list++; @@ -1181,7 +1181,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(wm->ldr.FullDllName.Buffer), (void *)thunk_list->u1.Function ); } TRACE_(imports)("--- Ordinal %s.%d = %p\n", name, ordinal, (void *)thunk_list->u1.Function ); @@ -1197,7 +1197,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(wm->ldr.FullDllName.Buffer), (void *)thunk_list->u1.Function ); } TRACE_(imports)("--- %s %s.%d = %p\n", @@ -1469,7 +1469,7 @@ static NTSTATUS fixup_imports( WINE_MODREF *wm, LPCWSTR load_path ) for (i = 0; i < nb_imports; i++) { dep_after = wm->ldr.DdagNode->Dependencies.Tail; - if (!import_dll( wm->ldr.DllBase, &imports[i], load_path, &imp )) + if (!import_dll( wm, &imports[i], load_path, &imp )) status = STATUS_DLL_NOT_FOUND; 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 );
From: Jacek Caban jacek@codeweavers.com
--- dlls/ntdll/loader.c | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index b48a950e705..d8ea6750cbc 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -191,9 +191,9 @@ 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, +static FARPROC find_ordinal_export( WINE_MODREF *wm, const IMAGE_EXPORT_DIRECTORY *exports, DWORD exp_size, DWORD ordinal, LPCWSTR load_path ); -static FARPROC find_named_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, +static FARPROC find_named_export( WINE_MODREF *wm, const IMAGE_EXPORT_DIRECTORY *exports, DWORD exp_size, const char *name, int hint, LPCWSTR load_path );
/* check whether the file name contains a path */ @@ -903,7 +903,7 @@ static NTSTATUS walk_node_dependencies( LDR_DDAG_NODE *node, void *context, * Find the final function pointer for a forwarded function. * The loader_section must be locked while calling this function. */ -static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWSTR load_path ) +static FARPROC find_forwarded_export( WINE_MODREF *imp, const char *forward, LPCWSTR load_path ) { const IMAGE_EXPORT_DIRECTORY *exports; DWORD exp_size; @@ -917,7 +917,6 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS
if (!(wm = find_basename_module( mod_name ))) { - 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)) @@ -946,18 +945,18 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS const char *name = end + 1;
if (*name == '#') { /* ordinal */ - proc = find_ordinal_export( wm->ldr.DllBase, exports, exp_size, + proc = find_ordinal_export( wm, exports, exp_size, atoi(name+1) - exports->Base, load_path ); } else - proc = find_named_export( wm->ldr.DllBase, exports, exp_size, name, -1, load_path ); + proc = find_named_export( wm, exports, exp_size, name, -1, load_path ); }
if (!proc) { ERR("function not found for forward '%s' used by %s." " If you are using builtin %s, try using the native one instead.\n", - forward, debugstr_w(get_modref(module)->ldr.FullDllName.Buffer), - debugstr_w(get_modref(module)->ldr.BaseDllName.Buffer) ); + forward, debugstr_w(imp->ldr.FullDllName.Buffer), + debugstr_w(imp->ldr.BaseDllName.Buffer) ); } return proc; } @@ -970,9 +969,10 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS * The exports base must have been subtracted from the ordinal already. * The loader_section must be locked while calling this function. */ -static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, +static FARPROC find_ordinal_export( WINE_MODREF *wm, const IMAGE_EXPORT_DIRECTORY *exports, DWORD exp_size, DWORD ordinal, LPCWSTR load_path ) { + HMODULE module = wm->ldr.DllBase; FARPROC proc; const DWORD *functions = get_rva( module, exports->AddressOfFunctions );
@@ -988,7 +988,7 @@ static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY /* if the address falls into the export dir, it's a forward */ if (((const char *)proc >= (const char *)exports) && ((const char *)proc < (const char *)exports + exp_size)) - return find_forwarded_export( module, (const char *)proc, load_path ); + return find_forwarded_export( wm, (const char *)proc, load_path );
if (TRACE_ON(snoop)) { @@ -1033,9 +1033,10 @@ static int find_name_in_exports( HMODULE module, const IMAGE_EXPORT_DIRECTORY *e * Find an exported function by name. * The loader_section must be locked while calling this function. */ -static FARPROC find_named_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, +static FARPROC find_named_export( WINE_MODREF *wm, const IMAGE_EXPORT_DIRECTORY *exports, DWORD exp_size, const char *name, int hint, LPCWSTR load_path ) { + const HMODULE module = wm->ldr.DllBase; const WORD *ordinals = get_rva( module, exports->AddressOfNameOrdinals ); const DWORD *names = get_rva( module, exports->AddressOfNames ); int ordinal; @@ -1045,12 +1046,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 ); + return find_ordinal_export( wm, exports, exp_size, ordinals[hint], load_path ); }
/* 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 ); + return find_ordinal_export( wm, exports, exp_size, ordinal, load_path );
}
@@ -1094,7 +1095,6 @@ static BOOL import_dll( WINE_MODREF *wm, const IMAGE_IMPORT_DESCRIPTOR *descr, L BOOL system = wm->system || (wm->ldr.Flags & LDR_WINE_INTERNAL); NTSTATUS status; WINE_MODREF *wmImp; - HMODULE imp_mod; const IMAGE_EXPORT_DIRECTORY *exports; DWORD exp_size; const IMAGE_THUNK_DATA *import_list; @@ -1140,8 +1140,7 @@ static BOOL import_dll( WINE_MODREF *wm, const IMAGE_IMPORT_DESCRIPTOR *descr, L NtProtectVirtualMemory( NtCurrentProcess(), &protect_base, &protect_size, PAGE_READWRITE, &protect_old );
- imp_mod = wmImp->ldr.DllBase; - exports = RtlImageDirectoryEntryToData( imp_mod, TRUE, IMAGE_DIRECTORY_ENTRY_EXPORT, &exp_size ); + exports = RtlImageDirectoryEntryToData( wmImp->ldr.DllBase, TRUE, IMAGE_DIRECTORY_ENTRY_EXPORT, &exp_size );
if (!exports) { @@ -1175,7 +1174,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, + thunk_list->u1.Function = (ULONG_PTR)find_ordinal_export( wmImp, exports, exp_size, ordinal - exports->Base, load_path ); if (!thunk_list->u1.Function) { @@ -1190,7 +1189,7 @@ static BOOL import_dll( WINE_MODREF *wm, const IMAGE_IMPORT_DESCRIPTOR *descr, L { IMAGE_IMPORT_BY_NAME *pe_name; pe_name = get_rva( module, (DWORD)import_list->u1.AddressOfData ); - thunk_list->u1.Function = (ULONG_PTR)find_named_export( imp_mod, exports, exp_size, + thunk_list->u1.Function = (ULONG_PTR)find_named_export( wmImp, exports, exp_size, (const char*)pe_name->Name, pe_name->Hint, load_path ); if (!thunk_list->u1.Function) @@ -2039,8 +2038,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 ) - : find_ordinal_export( module, exports, exp_size, ord - exports->Base, NULL ); + void *proc = name ? find_named_export( wm, exports, exp_size, name->Buffer, -1, NULL ) + : find_ordinal_export( wm, exports, exp_size, ord - exports->Base, NULL ); if (proc) { *address = proc;
From: Jacek Caban jacek@codeweavers.com
--- dlls/ntdll/loader.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index d8ea6750cbc..0a7eddfb4e5 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -734,11 +734,11 @@ static NTSTATUS get_apiset_target( const API_SET_NAMESPACE *map, const API_SET_N /********************************************************************** * build_import_name */ -static NTSTATUS build_import_name( WCHAR buffer[256], const char *import, int len ) +static NTSTATUS build_import_name( WINE_MODREF *wm, WCHAR buffer[256], const char *import, int len ) { 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 = wm->ldr.BaseDllName.Buffer; UNICODE_STRING str;
while (len && import[len-1] == ' ') len--; /* remove trailing spaces */ @@ -913,7 +913,7 @@ static FARPROC find_forwarded_export( WINE_MODREF *imp, const char *forward, LPC FARPROC proc = NULL;
if (!end) return NULL; - if (build_import_name( mod_name, forward, end - forward )) return NULL; + if (build_import_name( imp, mod_name, forward, end - forward )) return NULL;
if (!(wm = find_basename_module( mod_name ))) { @@ -1119,7 +1119,7 @@ static BOOL import_dll( WINE_MODREF *wm, const IMAGE_IMPORT_DESCRIPTOR *descr, L return TRUE; }
- status = build_import_name( buffer, name, len ); + status = build_import_name( wm, buffer, name, len ); if (!status) status = load_dll( load_path, buffer, 0, &wmImp, system );
if (status)
From: Jacek Caban jacek@codeweavers.com
--- dlls/ntdll/loader.c | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 0a7eddfb4e5..803edec5d15 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -192,9 +192,9 @@ 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( WINE_MODREF *wm, const IMAGE_EXPORT_DIRECTORY *exports, - DWORD exp_size, DWORD ordinal, LPCWSTR load_path ); + DWORD exp_size, DWORD ordinal, LPCWSTR load_path, const WCHAR *user ); static FARPROC find_named_export( WINE_MODREF *wm, const IMAGE_EXPORT_DIRECTORY *exports, - DWORD exp_size, const char *name, int hint, LPCWSTR load_path ); + DWORD exp_size, const char *name, int hint, LPCWSTR load_path, const WCHAR *user );
/* check whether the file name contains a path */ static inline BOOL contains_path( LPCWSTR name ) @@ -903,7 +903,7 @@ static NTSTATUS walk_node_dependencies( LDR_DDAG_NODE *node, void *context, * Find the final function pointer for a forwarded function. * The loader_section must be locked while calling this function. */ -static FARPROC find_forwarded_export( WINE_MODREF *imp, const char *forward, LPCWSTR load_path ) +static FARPROC find_forwarded_export( WINE_MODREF *imp, const char *forward, LPCWSTR load_path, const WCHAR *user ) { const IMAGE_EXPORT_DIRECTORY *exports; DWORD exp_size; @@ -945,10 +945,10 @@ static FARPROC find_forwarded_export( WINE_MODREF *imp, const char *forward, LPC const char *name = end + 1;
if (*name == '#') { /* ordinal */ - proc = find_ordinal_export( wm, exports, exp_size, - atoi(name+1) - exports->Base, load_path ); + proc = find_ordinal_export( wm, exports, exp_size, atoi(name+1) - exports->Base, + load_path, user ); } else - proc = find_named_export( wm, exports, exp_size, name, -1, load_path ); + proc = find_named_export( wm, exports, exp_size, name, -1, load_path, user ); }
if (!proc) @@ -970,7 +970,7 @@ static FARPROC find_forwarded_export( WINE_MODREF *imp, const char *forward, LPC * The loader_section must be locked while calling this function. */ static FARPROC find_ordinal_export( WINE_MODREF *wm, const IMAGE_EXPORT_DIRECTORY *exports, - DWORD exp_size, DWORD ordinal, LPCWSTR load_path ) + DWORD exp_size, DWORD ordinal, LPCWSTR load_path, const WCHAR *user ) { HMODULE module = wm->ldr.DllBase; FARPROC proc; @@ -988,18 +988,12 @@ static FARPROC find_ordinal_export( WINE_MODREF *wm, const IMAGE_EXPORT_DIRECTOR /* if the address falls into the export dir, it's a forward */ if (((const char *)proc >= (const char *)exports) && ((const char *)proc < (const char *)exports + exp_size)) - return find_forwarded_export( wm, (const char *)proc, load_path ); + return find_forwarded_export( wm, (const char *)proc, load_path, user );
if (TRACE_ON(snoop)) - { - const WCHAR *user = current_modref ? current_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; proc = RELAY_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); - } return proc; }
@@ -1034,7 +1028,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( WINE_MODREF *wm, const IMAGE_EXPORT_DIRECTORY *exports, - DWORD exp_size, const char *name, int hint, LPCWSTR load_path ) + DWORD exp_size, const char *name, int hint, LPCWSTR load_path, + const WCHAR *user ) { const HMODULE module = wm->ldr.DllBase; const WORD *ordinals = get_rva( module, exports->AddressOfNameOrdinals ); @@ -1046,12 +1041,12 @@ static FARPROC find_named_export( WINE_MODREF *wm, const IMAGE_EXPORT_DIRECTORY { char *ename = get_rva( module, names[hint] ); if (!strcmp( ename, name )) - return find_ordinal_export( wm, exports, exp_size, ordinals[hint], load_path ); + return find_ordinal_export( wm, exports, exp_size, ordinals[hint], load_path, user ); }
/* then do a binary search */ if ((ordinal = find_name_in_exports( module, exports, name )) == -1) return NULL; - return find_ordinal_export( wm, exports, exp_size, ordinal, load_path ); + return find_ordinal_export( wm, exports, exp_size, ordinal, load_path, user );
}
@@ -1175,7 +1170,8 @@ 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( wmImp, exports, exp_size, - ordinal - exports->Base, load_path ); + ordinal - exports->Base, load_path, + wm->ldr.BaseDllName.Buffer ); if (!thunk_list->u1.Function) { thunk_list->u1.Function = allocate_stub( name, IntToPtr(ordinal) ); @@ -1191,7 +1187,8 @@ 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( wmImp, exports, exp_size, (const char*)pe_name->Name, - pe_name->Hint, load_path ); + pe_name->Hint, load_path, + wm->ldr.BaseDllName.Buffer ); if (!thunk_list->u1.Function) { thunk_list->u1.Function = allocate_stub( name, (const char*)pe_name->Name ); @@ -2038,8 +2035,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( wm, exports, exp_size, name->Buffer, -1, NULL ) - : find_ordinal_export( wm, exports, exp_size, ord - exports->Base, NULL ); + void *proc = name ? find_named_export( wm, exports, exp_size, name->Buffer, -1, NULL, NULL ) + : find_ordinal_export( wm, exports, exp_size, ord - exports->Base, NULL, NULL ); if (proc) { *address = proc;
Thanks! I also largely agree that `current_modref` can go away and replaced with explicit parameters.[^1]
I think, at least for testing, each commit replacing `current_modref` can be split into the following with much finer granularity:
1. Introduce the `wm` parameter if not already, *but* don't use it just yet. Instead, simply assert() that `wm == current_modref`. That way, if a function writes to `current_modref`, make the same changes to `wm` as well. This will catch any de-sync bugs due to incorrect arguments. 2. Replace all reads to `current_modref` with `wm`.[^2] If (1) passes tests, then this commit will pass them too. 3. If, **and only if,** no callees are known to use `current_modref`, replace all writes to `current_modref` with `current_moderef = (WINE_MODREF *)-0x10000;` or any other poison value. This will catch any unintended use of `current_modref` from one of its callees.
Then, when we actually eliminate the `current_modref`, the poison value will finally go away.
Once we have assurance that the commits splitted as above are indeed atomic and do no cause (test) regressions, I think we can proceed with the split. If you don't have time, I can volunteer.
[^1]: In general, I believe that eliminating dynamically scoped (global or TLS) variabls in favor of lexically scoped variables (function parameters and locals) helps dissect data flow and make function behavior more predictable. [^2]: We just asserted that `current_modref == wm` in all circumstances, barring any callees failing to restore `current_modref` before returning due to e.g., SEH unwinding.
-
Jacek Caban -
Jacek Caban (＠jacek)
-
Jinoh Kang (＠iamahuman)