[PATCH v2 0/5] MR7: ntdll: Properly track refcount with forwarded exports.
This fixes https://bugs.winehq.org/show_bug.cgi?id=52094. -- v2: ntdll: Properly track refcount with forwarded exports. ntdll: Update current_modref in LdrGetProcedureAddress(). ntdll: De-duplicate module dependency edges. ntdll: Introduce struct dependency_edge as wrapper around LDR_DEPENDENCY. kernel32/tests: Test module refcounting with forwarded exports. https://gitlab.winehq.org/wine/wine/-/merge_requests/7
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/kernel32/tests/loader.c | 405 +++++++++++++++++++++++++++++++++++ 1 file changed, 405 insertions(+) diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 8f418ef09a9..1898bf3594f 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -2409,6 +2409,410 @@ static void test_import_resolution(void) } } +static HANDLE gen_forward_chain_testdll( char testdll_path[MAX_PATH], const char source_dll[MAX_PATH], + BOOL is_export, BOOL is_import, + DWORD *exp_func_base_rva, + DWORD *imp_thunk_base_rva ) +{ + DWORD text_rva = page_size; + DWORD text_size = page_size; + DWORD edata_rva = text_rva + text_size; + DWORD edata_size = page_size; + DWORD idata_rva = edata_rva + text_size; + DWORD idata_size = page_size; + DWORD eof_rva = idata_rva + edata_size; + const IMAGE_SECTION_HEADER sections[3] = { + { + .Name = ".text", + .Misc = { .VirtualSize = text_size }, + .VirtualAddress = text_rva, + .SizeOfRawData = text_size, + .PointerToRawData = text_rva, + .Characteristics = IMAGE_SCN_CNT_CODE | IMAGE_SCN_MEM_READ | IMAGE_SCN_MEM_EXECUTE, + }, + { + .Name = ".edata", + .Misc = { .VirtualSize = edata_size }, + .VirtualAddress = edata_rva, + .SizeOfRawData = edata_size, + .PointerToRawData = edata_rva, + .Characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ, + }, + { + .Name = ".idata", + .Misc = { .VirtualSize = edata_size }, + .VirtualAddress = idata_rva, + .SizeOfRawData = idata_size, + .PointerToRawData = idata_rva, + .Characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ | IMAGE_SCN_MEM_WRITE, + }, + }; + struct expdesc + { + IMAGE_EXPORT_DIRECTORY dir; + DWORD functions[2]; + DWORD names[2]; + WORD name_ords[2]; + char str_forward_test_func[32]; + char str_forward_test_func2[32]; + char dll_name[MAX_PATH]; + char strpool[2][MAX_PATH + 16]; + } expdesc = { + .dir = { + .Characteristics = 0, + .TimeDateStamp = 0x12345678, + .Name = edata_rva + offsetof(struct expdesc, dll_name), + .Base = 1, + .NumberOfFunctions = ARRAY_SIZE(expdesc.functions), + .NumberOfNames = ARRAY_SIZE(expdesc.names), + .AddressOfFunctions = edata_rva + offsetof(struct expdesc, functions), + .AddressOfNames = edata_rva + offsetof(struct expdesc, names), + .AddressOfNameOrdinals = edata_rva + offsetof(struct expdesc, name_ords), + }, + .functions = { + text_rva + 0x4, + text_rva + 0x8, + }, + .names = { + edata_rva + offsetof(struct expdesc, str_forward_test_func), + edata_rva + offsetof(struct expdesc, str_forward_test_func2), + }, + .name_ords = { + 0, + 1, + }, + .str_forward_test_func = "forward_test_func", + .str_forward_test_func2 = "forward_test_func2", + }; + struct impdesc + { + IMAGE_IMPORT_DESCRIPTOR descr[2]; + IMAGE_THUNK_DATA original_thunks[3]; + IMAGE_THUNK_DATA thunks[3]; + struct { WORD hint; char name[32]; } impname_forward_test_func; + char module[MAX_PATH]; + } impdesc = { + .descr = { + { + .OriginalFirstThunk = idata_rva + offsetof(struct impdesc, original_thunks), + .TimeDateStamp = 0, + .ForwarderChain = -1, + .Name = idata_rva + offsetof(struct impdesc, module), + .FirstThunk = idata_rva + offsetof(struct impdesc, thunks), + }, + {{ 0 }}, + }, + .original_thunks = { + {{ idata_rva + offsetof(struct impdesc, impname_forward_test_func) }}, + {{ IMAGE_ORDINAL_FLAG | 2 }}, + {{ 0 }}, + }, + .thunks = { + {{ idata_rva + offsetof(struct impdesc, impname_forward_test_func) }}, + {{ IMAGE_ORDINAL_FLAG | 2 }}, + {{ 0 }}, + }, + .impname_forward_test_func = { 0, "forward_test_func" }, + }; + IMAGE_NT_HEADERS nt_header; + char temp_path[MAX_PATH]; + HANDLE file, file_w; + LARGE_INTEGER qpc; + DWORD outlen; + BOOL ret; + int res; + + QueryPerformanceCounter( &qpc ); + res = snprintf( expdesc.dll_name, ARRAY_SIZE(expdesc.dll_name), + "ldr%05lx.dll", qpc.LowPart & 0xfffffUL ); + ok( res > 0 && res < ARRAY_SIZE(expdesc.dll_name), "snprintf failed\n" ); + + if (source_dll) + { + const char *export_names[2] = { + "forward_test_func", + "#2", + }; + const char *backslash = strrchr( source_dll, '\\' ); + const char *dllname = backslash ? backslash + 1 : source_dll; + const char *dot = strrchr( dllname, '.' ); + size_t ext_start = dot ? dot - dllname : strlen(dllname); + size_t i; + + res = snprintf( impdesc.module, ARRAY_SIZE(impdesc.module), "%s", dllname ); + ok( res > 0 && res < ARRAY_SIZE(impdesc.module), "snprintf() failed\n" ); + + for (i = 0; i < ARRAY_SIZE(export_names); i++) + { + char *buf; + size_t buf_size; + + assert( i < ARRAY_SIZE(expdesc.strpool) ); + buf = expdesc.strpool[i]; + buf_size = ARRAY_SIZE(expdesc.strpool[i]); + + assert( ext_start < buf_size ); + memcpy( buf, dllname, ext_start ); + buf += ext_start; + buf_size -= ext_start; + + res = snprintf( buf, buf_size, ".%s", export_names[i] ); + ok( res > 0 && res < buf_size, "snprintf() failed\n" ); + + assert( i < ARRAY_SIZE(expdesc.functions) ); + expdesc.functions[i] = edata_rva + (expdesc.strpool[i] - (char *)&expdesc); + } + } + + nt_header = nt_header_template; + nt_header.FileHeader.TimeDateStamp = 0x12345678; + nt_header.FileHeader.NumberOfSections = ARRAY_SIZE(sections); + nt_header.FileHeader.SizeOfOptionalHeader = sizeof(IMAGE_OPTIONAL_HEADER); + + nt_header.OptionalHeader.SizeOfCode = text_size; + nt_header.OptionalHeader.SectionAlignment = page_size; + nt_header.OptionalHeader.FileAlignment = page_size; + nt_header.OptionalHeader.SizeOfHeaders = sizeof(dos_header) + sizeof(nt_header) + sizeof(sections); + nt_header.OptionalHeader.SizeOfImage = eof_rva; + if (is_export) + { + nt_header.OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_EXPORT].VirtualAddress = edata_rva; + nt_header.OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_EXPORT].Size = sizeof(expdesc); + } + nt_header.OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_IMPORT].VirtualAddress = idata_rva; + nt_header.OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_IMPORT].Size = is_import ? sizeof(impdesc) : sizeof(IMAGE_IMPORT_DESCRIPTOR); + + outlen = GetTempPathA( ARRAY_SIZE(temp_path), temp_path ); + ok( outlen > 0 && outlen < ARRAY_SIZE(temp_path), "GetTempPathA() err=%lu\n", GetLastError() ); + + res = snprintf( testdll_path, MAX_PATH, "%s\\%s", temp_path, expdesc.dll_name ); + ok( res > 0 && res < MAX_PATH, "snprintf failed\n" ); + + /* Open file handle that will be deleted on close or process termination */ + file = CreateFileA( testdll_path, + DELETE, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + NULL, + CREATE_NEW, + FILE_ATTRIBUTE_NORMAL | FILE_FLAG_DELETE_ON_CLOSE, + NULL ); + ok( file != INVALID_HANDLE_VALUE, "CreateFile(%s) for delete returned error %lu\n", + wine_dbgstr_a( testdll_path ), GetLastError() ); + + /* Open file again with write access */ + file_w = CreateFileA( testdll_path, + GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_DELETE, + NULL, + OPEN_EXISTING, + FILE_ATTRIBUTE_NORMAL, + NULL ); + ok( file_w != INVALID_HANDLE_VALUE, "CreateFile(%s) for write returned error %lu\n", + wine_dbgstr_a( testdll_path ), GetLastError() ); + + ret = WriteFile( file_w, &dos_header, sizeof(dos_header), &outlen, NULL ); + ok( ret && outlen == sizeof(dos_header), "write dos_header: err=%lu outlen=%lu\n", GetLastError(), outlen ); + + ret = WriteFile( file_w, &nt_header, sizeof(nt_header), &outlen, NULL ); + ok( ret && outlen == sizeof(nt_header), "write nt_header: err=%lu outlen=%lu\n", GetLastError(), outlen ); + + ret = WriteFile( file_w, sections, sizeof(sections), &outlen, NULL ); + ok( ret && outlen == sizeof(sections), "write sections: err=%lu outlen=%lu\n", GetLastError(), outlen ); + + if (is_export) + { + SetFilePointer( file_w, edata_rva, NULL, FILE_BEGIN ); + ret = WriteFile( file_w, &expdesc, sizeof(expdesc), &outlen, NULL ); + ok( ret && outlen == sizeof(expdesc), "write expdesc: err=%lu outlen=%lu\n", GetLastError(), outlen ); + } + + if (is_import) + { + SetFilePointer( file_w, idata_rva, NULL, FILE_BEGIN ); + ret = WriteFile( file_w, &impdesc, sizeof(impdesc), &outlen, NULL ); + ok( ret && outlen == sizeof(impdesc), "write impdesc: err=%lu outlen=%lu\n", GetLastError(), outlen ); + } + + ret = SetFilePointer( file_w, eof_rva, NULL, FILE_BEGIN ); + ok( ret, "%lu\n", GetLastError() ); + ret = SetEndOfFile( file_w ); + ok( ret, "%lu\n", GetLastError() ); + + ret = CloseHandle( file_w ); + ok( ret, "%lu\n", GetLastError() ); + + if (exp_func_base_rva) + { + *exp_func_base_rva = is_export ? edata_rva + ((char *)&expdesc.functions - (char *)&expdesc) : 0; + } + + if (imp_thunk_base_rva) + { + *imp_thunk_base_rva = is_import ? idata_rva + ((char *)&impdesc.thunks - (char *)&impdesc) : 0; + } + + return file; +} + +static void subtest_export_forwarder_dep_chain( size_t num_chained_export_modules, size_t exporter_index, BOOL test_static_import ) +{ + size_t num_modules = num_chained_export_modules + !!test_static_import; + size_t importer_index = test_static_import ? num_modules - 1 : 0; + DWORD imp_thunk_base_rva, exp_func_base_rva; + size_t i, ultimate_depender_index = 0; + char temp_paths[4][MAX_PATH]; + HANDLE temp_files[4]; + UINT_PTR exports[2]; + HMODULE modules[4]; + BOOL res; + + assert(exporter_index < num_chained_export_modules); + assert(num_modules > 1); + assert(num_modules <= ARRAY_SIZE(temp_paths)); + assert(num_modules <= ARRAY_SIZE(temp_files)); + assert(num_modules <= ARRAY_SIZE(modules)); + + if (winetest_debug > 1) + trace( "Generate a chain of test DLL fixtures\n" ); + + for (i = 0; i < num_modules; i++) + { + temp_files[i] = gen_forward_chain_testdll( temp_paths[i], + i >= 1 ? temp_paths[i - 1] : NULL, + i < num_chained_export_modules, + importer_index && i == importer_index, + i == 0 ? &exp_func_base_rva : NULL, + i == importer_index ? &imp_thunk_base_rva : NULL ); + } + + if (winetest_debug > 1) + trace( "Load the entire test DLL chain\n" ); + + for (i = 0; i < num_modules; i++) + { + ok( !GetModuleHandleA( temp_paths[i] ), "%s already loaded\n", + wine_dbgstr_a( temp_paths[i] ) ); + + modules[i] = LoadLibraryA( temp_paths[i] ); + ok( !!modules[i], "LoadLibraryA(temp_paths[%Iu] = %s) err=%lu\n", + i, wine_dbgstr_a( temp_paths[i] ), GetLastError() ); + } + + if (winetest_debug > 1) + trace( "Get address of exported functions from the source module\n" ); + + for (i = 0; i < ARRAY_SIZE(exports); i++) + { + char *mod_base = (char *)modules[0]; + exports[i] = (UINT_PTR)(mod_base + ((DWORD *)(mod_base + exp_func_base_rva))[i]); + } + + if (winetest_debug > 1) + trace( "Check import address table of the importer DLL, if any\n" ); + + if (importer_index) + { + UINT_PTR *imp_thunk_base = (UINT_PTR *)((char *)modules[importer_index] + imp_thunk_base_rva); + for (i = 0; i < ARRAY_SIZE(exports); i++) + { + ok( imp_thunk_base[i] == exports[i], "import thunk mismatch [%Iu]: (%#Ix, %#Ix)\n", + i, imp_thunk_base[i], exports[i] ); + } + + ultimate_depender_index = max( ultimate_depender_index, importer_index ); + } + + if (winetest_debug > 1) + trace( "Call GetProcAddress() on the exporter DLL, if any\n" ); + + if (exporter_index) + { + UINT_PTR proc; + + proc = (UINT_PTR)GetProcAddress( modules[exporter_index], "forward_test_func" ); + ok( proc == exports[0], "GetProcAddress mismatch [0]: (%#Ix, %#Ix)\n", proc, exports[0] ); + + proc = (UINT_PTR)GetProcAddress( modules[exporter_index], (LPSTR)2 ); + ok( proc == exports[1], "GetProcAddress mismatch [1]: (%#Ix, %#Ix)\n", proc, exports[1] ); + + ultimate_depender_index = max( ultimate_depender_index, exporter_index ); + } + + if (winetest_debug > 1) + trace( "Unreference modules except the ultimate dependant DLL\n" ); + + for (i = 0; i < ultimate_depender_index; i++) + { + res = FreeLibrary( modules[i] ); + ok( res, "FreeLibrary(modules[%Iu]) err=%lu\n", i, GetLastError() ); + } + + if (winetest_debug > 1) + trace( "The ultimate dependant DLL should keep other DLLs from being unloaded\n" ); + + for (i = 0; i < num_modules; i++) + { + 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?)\n", + i, modules[i], module ); + } + + if (winetest_debug > 1) + trace( "Unreference the remaining modules (including the dependant DLL)\n" ); + + for (i = ultimate_depender_index; i < num_modules; i++) + { + res = FreeLibrary( modules[i] ); + ok( res, "FreeLibrary(modules[%Iu]) err=%lu\n", i, GetLastError() ); + + /* FreeLibrary() should now unload the DLL immediately */ + ok( !GetModuleHandleA( temp_paths[i] ), "modules[%Iu] unexpectedly kept loaded (2)\n", i ); + } + + if (winetest_debug > 1) + trace( "All modules should be unloaded\n" ); + + for (i = 0; i < num_modules; i++) + { + ok( !GetModuleHandleA( temp_paths[i] ), "modules[%Iu] unexpectedly kept loaded (3)\n", i ); + } + + if (winetest_debug > 1) + trace( "Close and delete temp files\n" ); + + for (i = 0; i < num_modules; i++) + { + /* handles are delete-on-close */ + CloseHandle( temp_files[i] ); + } +} + +static void test_export_forwarder_dep_chain(void) +{ + winetest_push_context( "no import" ); + /* export forwarder does not introduce a dependency on its own */ + subtest_export_forwarder_dep_chain( 2, FALSE, 0 ); + winetest_pop_context(); + + winetest_push_context( "static import of export forwarder" ); + subtest_export_forwarder_dep_chain( 2, TRUE, 0 ); + winetest_pop_context(); + + winetest_push_context( "static import of chained export forwarder" ); + subtest_export_forwarder_dep_chain( 3, TRUE, 0 ); + winetest_pop_context(); + + winetest_push_context( "dynamic import of export forwarder" ); + subtest_export_forwarder_dep_chain( 2, FALSE, 1 ); + winetest_pop_context(); + + winetest_push_context( "dynamic import of chained export forwarder" ); + subtest_export_forwarder_dep_chain( 3, FALSE, 2 ); + winetest_pop_context(); +} + #define MAX_COUNT 10 static HANDLE attached_thread[MAX_COUNT]; static DWORD attached_thread_count; @@ -4282,6 +4686,7 @@ START_TEST(loader) test_ImportDescriptors(); test_section_access(); test_import_resolution(); + test_export_forwarder_dep_chain(); test_ExitProcess(); test_InMemoryOrderModuleList(); test_LoadPackagedLibrary(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> This allows extension of LDR_DEPENDENCY, while trying to maintain compatibility with applications that may depend on native loader internals. In preparation for a patch that keeps track of all dependency edges between all modules globally, in turn enabling de-duplication of identical edges. --- dlls/ntdll/loader.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index c5d82e8c68c..35cf76d24dc 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -115,6 +115,11 @@ struct ldr_notification static struct list ldr_notifications = LIST_INIT( ldr_notifications ); +struct dependency_edge +{ + LDR_DEPENDENCY ldr; +}; + static const char * const reason_names[] = { "PROCESS_DETACH", @@ -856,14 +861,14 @@ static void remove_single_list_entry( LDRP_CSLIST *list, SINGLE_LIST_ENTRY *entr static BOOL add_module_dependency_after( LDR_DDAG_NODE *from, LDR_DDAG_NODE *to, SINGLE_LIST_ENTRY *dep_after ) { - LDR_DEPENDENCY *dep; + struct dependency_edge *dep; if (!(dep = RtlAllocateHeap( GetProcessHeap(), 0, sizeof(*dep) ))) return FALSE; - dep->dependency_from = from; - insert_single_list_after( &from->Dependencies, dep_after, &dep->dependency_to_entry ); - dep->dependency_to = to; - insert_single_list_after( &to->IncomingDependencies, NULL, &dep->dependency_from_entry ); + dep->ldr.dependency_from = from; + insert_single_list_after( &from->Dependencies, dep_after, &dep->ldr.dependency_to_entry ); + dep->ldr.dependency_to = to; + insert_single_list_after( &to->IncomingDependencies, NULL, &dep->ldr.dependency_from_entry ); return TRUE; } @@ -879,10 +884,10 @@ static BOOL add_module_dependency( LDR_DDAG_NODE *from, LDR_DDAG_NODE *to ) /********************************************************************** * remove_module_dependency */ -static void remove_module_dependency( LDR_DEPENDENCY *dep ) +static void remove_module_dependency( struct dependency_edge *dep ) { - remove_single_list_entry( &dep->dependency_to->IncomingDependencies, &dep->dependency_from_entry ); - remove_single_list_entry( &dep->dependency_from->Dependencies, &dep->dependency_to_entry ); + remove_single_list_entry( &dep->ldr.dependency_to->IncomingDependencies, &dep->ldr.dependency_from_entry ); + remove_single_list_entry( &dep->ldr.dependency_from->Dependencies, &dep->ldr.dependency_to_entry ); RtlFreeHeap( GetProcessHeap(), 0, dep ); } @@ -3914,7 +3919,7 @@ void WINAPI LdrShutdownThread(void) static void free_modref( WINE_MODREF *wm ) { SINGLE_LIST_ENTRY *entry; - LDR_DEPENDENCY *dep; + struct dependency_edge *dep; RemoveEntryList(&wm->ldr.InLoadOrderLinks); RemoveEntryList(&wm->ldr.InMemoryOrderLinks); @@ -3923,15 +3928,15 @@ static void free_modref( WINE_MODREF *wm ) while ((entry = wm->ldr.DdagNode->Dependencies.Tail)) { - dep = CONTAINING_RECORD( entry, LDR_DEPENDENCY, dependency_to_entry ); - assert( dep->dependency_from == wm->ldr.DdagNode ); + dep = CONTAINING_RECORD( entry, struct dependency_edge, ldr.dependency_to_entry ); + assert( dep->ldr.dependency_from == wm->ldr.DdagNode ); remove_module_dependency( dep ); } while ((entry = wm->ldr.DdagNode->IncomingDependencies.Tail)) { - dep = CONTAINING_RECORD( entry, LDR_DEPENDENCY, dependency_from_entry ); - assert( dep->dependency_to == wm->ldr.DdagNode ); + dep = CONTAINING_RECORD( entry, struct dependency_edge, ldr.dependency_from_entry ); + assert( dep->ldr.dependency_to == wm->ldr.DdagNode ); remove_module_dependency( dep ); } -- 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. To avoid duplicates, keep track of edges in a red-black tree, and discard duplicate ones. 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 | 46 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 35cf76d24dc..fedeb5746b8 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -34,6 +34,7 @@ #include "wine/exception.h" #include "wine/debug.h" #include "wine/list.h" +#include "wine/rbtree.h" #include "ntdll_misc.h" #include "ddk/ntddk.h" #include "ddk/wdm.h" @@ -115,10 +116,18 @@ struct ldr_notification static struct list ldr_notifications = LIST_INIT( ldr_notifications ); +struct dependency_key +{ + const LDR_DDAG_NODE *dependency_to; + const LDR_DDAG_NODE *dependency_from; +}; struct dependency_edge { + struct rb_entry entry; LDR_DEPENDENCY ldr; }; +static int dependency_edge_cmp_rb( const void *key, const struct rb_entry *entry ); +static struct rb_tree dependency_edges = { dependency_edge_cmp_rb }; static const char * const reason_names[] = { @@ -855,16 +864,47 @@ static void remove_single_list_entry( LDRP_CSLIST *list, SINGLE_LIST_ENTRY *entr entry->Next = NULL; } +static int dependency_edge_cmp_rb( const void *key, const struct rb_entry *entry ) +{ + const struct dependency_edge *entry_edge = RB_ENTRY_VALUE( entry, const struct dependency_edge, entry ); + const struct dependency_key *key_ldr = key; + + if ((ULONG_PTR)key_ldr->dependency_to < (ULONG_PTR)entry_edge->ldr.dependency_to) return -1; + if ((ULONG_PTR)key_ldr->dependency_to > (ULONG_PTR)entry_edge->ldr.dependency_to) return 1; + if ((ULONG_PTR)key_ldr->dependency_from < (ULONG_PTR)entry_edge->ldr.dependency_from) return -1; + if ((ULONG_PTR)key_ldr->dependency_from > (ULONG_PTR)entry_edge->ldr.dependency_from) return 1; + + return 0; +} + /********************************************************************** * add_module_dependency_after + * + * The loader_section must be locked while calling this function. */ static BOOL add_module_dependency_after( LDR_DDAG_NODE *from, LDR_DDAG_NODE *to, SINGLE_LIST_ENTRY *dep_after ) { + const struct dependency_key key = { to, from }; struct dependency_edge *dep; if (!(dep = RtlAllocateHeap( GetProcessHeap(), 0, sizeof(*dep) ))) return FALSE; + if (rb_put( &dependency_edges, &key, &dep->entry ) == -1) + { + /* Duplicate dependency edge; discard edge and decrement reference of target */ + LDR_DATA_TABLE_ENTRY *mod; + WINE_MODREF *wm; + + RtlFreeHeap( GetProcessHeap(), 0, dep ); + + mod = CONTAINING_RECORD( to->Modules.Flink, LDR_DATA_TABLE_ENTRY, NodeModuleLink ); + wm = CONTAINING_RECORD( mod, WINE_MODREF, ldr ); + LdrUnloadDll( wm->ldr.DllBase ); + + return TRUE; + } + dep->ldr.dependency_from = from; insert_single_list_after( &from->Dependencies, dep_after, &dep->ldr.dependency_to_entry ); dep->ldr.dependency_to = to; @@ -875,6 +915,8 @@ static BOOL add_module_dependency_after( LDR_DDAG_NODE *from, LDR_DDAG_NODE *to, /********************************************************************** * add_module_dependency + * + * The loader_section must be locked while calling this function. */ static BOOL add_module_dependency( LDR_DDAG_NODE *from, LDR_DDAG_NODE *to ) { @@ -883,11 +925,14 @@ static BOOL add_module_dependency( LDR_DDAG_NODE *from, LDR_DDAG_NODE *to ) /********************************************************************** * remove_module_dependency + * + * The loader_section must be locked while calling this function. */ static void remove_module_dependency( struct dependency_edge *dep ) { remove_single_list_entry( &dep->ldr.dependency_to->IncomingDependencies, &dep->ldr.dependency_from_entry ); remove_single_list_entry( &dep->ldr.dependency_from->Dependencies, &dep->ldr.dependency_to_entry ); + rb_remove( &dependency_edges, &dep->entry ); RtlFreeHeap( GetProcessHeap(), 0, dep ); } @@ -3915,6 +3960,7 @@ void WINAPI LdrShutdownThread(void) /*********************************************************************** * free_modref * + * The loader_section must be locked while calling this function. */ static void free_modref( WINE_MODREF *wm ) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> This is needed to handle dynamic module dependencies generated by GetProcAddress() etc. correctly. --- dlls/ntdll/loader.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index fedeb5746b8..955fe703929 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -2077,8 +2077,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 ); + WINE_MODREF *prev; + void *proc; + + prev = current_modref; + current_modref = wm; + + proc = name ? find_named_export( module, exports, exp_size, name->Buffer, -1, NULL ) + : find_ordinal_export( module, exports, exp_size, ord - exports->Base, NULL ); if (proc) { *address = proc; @@ -2089,6 +2095,8 @@ NTSTATUS WINAPI LdrGetProcedureAddress(HMODULE module, const ANSI_STRING *name, WARN( "%s (ordinal %lu) not found in %s\n", debugstr_a(name ? name->Buffer : NULL), ord, debugstr_us(&wm->ldr.FullDllName) ); } + + current_modref = prev; } RtlLeaveCriticalSection( &loader_section ); -- 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 | 1 - dlls/ntdll/loader.c | 35 ++++++++++++++++++----------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 1898bf3594f..b66089dac9b 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -2754,7 +2754,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?)\n", i, modules[i], module ); } diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 955fe703929..a1757bb591f 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -969,7 +969,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS { const IMAGE_EXPORT_DIRECTORY *exports; DWORD exp_size; - WINE_MODREF *wm; + WINE_MODREF *wm = NULL, *imp; WCHAR mod_name[256]; const char *end = strrchr(forward, '.'); FARPROC proc = NULL; @@ -977,30 +977,26 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS if (!end) return NULL; if (build_import_name( mod_name, forward, end - forward )) return NULL; - if (!(wm = find_basename_module( mod_name ))) + 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)) { - 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 || !current_modref) { - if (!imports_fixup_done && current_modref) - { - add_module_dependency( current_modref->ldr.DdagNode, wm->ldr.DdagNode ); - } - else if (process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) + if (process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) { LdrUnloadDll( wm->ldr.DllBase ); wm = NULL; } } + } - if (!wm) - { - ERR( "module not found for forward '%s' used by %s\n", - forward, debugstr_w(imp->ldr.FullDllName.Buffer) ); - return NULL; - } + if (!wm) + { + ERR( "module not found for forward '%s' used by %s\n", + forward, debugstr_w(imp->ldr.FullDllName.Buffer) ); + return NULL; } if ((exports = RtlImageDirectoryEntryToData( wm->ldr.DllBase, TRUE, IMAGE_DIRECTORY_ENTRY_EXPORT, &exp_size ))) @@ -1020,6 +1016,11 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS " 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) ); + if (wm) LdrUnloadDll( wm->ldr.DllBase ); + } + else if (current_modref) + { + add_module_dependency( current_modref->ldr.DdagNode, wm->ldr.DdagNode ); } 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=144992 Your paranoid android. === w10pro64 (32 bit report) === kernel32: loader.c:3372: Test failed: attached thread count should be 2 === debian11b (64 bit WoW report) === kernel32: module.c:1608: Test failed: Dep 1 (L"msvcrt.dll"): Got unexpected module L"ntdll.dll". module.c:1608: Test failed: Dep 2 (L"user32.dll"): Got unexpected module L"msvcrt.dll". module.c:1627: Test failed: Expected end of the list.
participants (3)
-
Jinoh Kang -
Jinoh Kang (＠iamahuman)
-
Marvin