[PATCH v11 0/1] MR364: kernel32/tests: Test module refcounting with forwarded exports.
Test module references created by resolving forwarded exports. The following diagram (best viewed in light mode due to fixed colors) explains what happens in bug #52094 under the hood: ```mermaid graph TB subgraph state_00["Initial"] direction LR s00_ida[ida64.dll] ~~~ s00_idapy[idapython64.dll] -. "(import)" .-> s00_py3[python3.dll] -. "(forwards to)" .-> s00_py311[python311.dll] end state_00 -. "1. <strong><code><span style='color:#28f'>Load</span>Library("</code></strong>configured Python version (e.g., python311.dll)<strong><code>")</code></strong>" .-> state_01 subgraph state_01["python311.dll loaded"] direction LR s01_ida[ida64.dll] ~~~ s01_idapy[idapython64.dll] -. "(import)" .-> s01_py3[python3.dll] -. "(forwards to)" .-> s01_py311[python311.dll] s01_ida -- "<em>dynamic</em> ref" --> s01_py311 end state_01 -. "2. <strong><code><span style='color:#28f'>Load</span>Library("</code></strong>idapython64.dll<strong><code>")</code></strong>" .-> state_02 subgraph state_02["idapython64.dll loaded"] direction LR s02_ida[ida64.dll] -- "<em>dynamic</em> ref" --> s02_idapy[idapython64.dll] -- "<em><strong>static</strong></em> ref" --> s02_py3[python3.dll] -. "(forwards to)" .-> s02_py311[python311.dll] s02_ida -- "<em>dynamic</em> ref" --> s02_py311 s02_idapy -- "<span style='color:red'><em><strong>static forwarded</strong></em> ref</span>" --> s02_py311 end state_02 -. "3. <strong><code><span style='color:#d70'>Free</span>Library("</code></strong>configured Python version (e.g., python311.dll)<strong><code>")</code></strong>" .-> state_03 subgraph state_03["python311.dll reference removed"] direction LR style s03_py311 color:#f00,stroke:#f00 s03_ida[ida64.dll] -- "<em>dynamic</em> ref" --> s03_idapy[idapython64.dll] -- "<em><strong>static</strong></em> ref" --> s03_py3[python3.dll] -. "(forwards to)" .-> s03_py311[python311.dll] s03_ida -. "<del><em>dynamic</em> ref (deleted)</del>" .-> s03_py311 s03_idapy -- "<span style='color:red'><em><strong>static forwarded</strong></em> ref</span>" --> s03_py311 end ``` On Windows, the <em><strong>static forwarded</strong></em> ref keeps `python311.dll` from being unloaded; however, Wine is missing this behavior, causing it to be unloaded and segfault immediately (since further initialization happens just after step 3 FreeLibrary). The tests aim to reproduce this condition. -- v11: kernel32/tests: Test module refcounting with forwarded exports. https://gitlab.winehq.org/wine/wine/-/merge_requests/364
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/364
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=144989 Your paranoid android. === w10pro64 (32 bit report) === kernel32: loader.c:3373: Test failed: attached thread count should be 2 === w1064_adm (64 bit report) === kernel32: loader.c:713: Test failed: 1415: wrong status c000007b/0 loader.c:718: Test failed: 1415: failed with c000007b expected fallback loader.c:713: Test failed: 1421: wrong status c000007b/0 loader.c:718: Test failed: 1421: failed with c000007b expected fallback loader.c:713: Test failed: 1427: wrong status c000007b/0 loader.c:718: Test failed: 1427: failed with c000007b expected fallback loader.c:713: Test failed: 1434: wrong status c000007b/0 loader.c:718: Test failed: 1434: failed with c000007b expected fallback loader.c:713: Test failed: 1440: wrong status c000007b/0
participants (3)
-
Jinoh Kang -
Jinoh Kang (＠iamahuman)
-
Marvin