[PATCH 0/3] MR2361: ntdll: Fix inverted TlsIndex check
Looks like I missed a TlsIndex use when implementing a30a5287f06ccf56a1d7184e8c22ea9e79e9efaf Also updates TlsIndex tests to include dlls that use tls Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54539 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2361
From: Evan Tang <etang(a)codeweavers.com> Fixes: a30a5287f06ccf56a1d7184e8c22ea9e79e9efaf Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54539 --- dlls/ntdll/loader.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index a0ac61d8156..f05cd1b4fd9 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -1573,7 +1573,7 @@ static NTSTATUS MODULE_InitDLL( WINE_MODREF *wm, UINT reason, LPVOID lpReserved /* Skip calls for modules loaded with special load flags */ if (wm->ldr.Flags & LDR_DONT_RESOLVE_REFS) return STATUS_SUCCESS; - if (wm->ldr.TlsIndex != -1) call_tls_callbacks( wm->ldr.DllBase, reason ); + if (wm->ldr.TlsIndex == -1) call_tls_callbacks( wm->ldr.DllBase, reason ); if (!entry) return STATUS_SUCCESS; if (TRACE_ON(relay)) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2361
From: Evan Tang <etang(a)codeweavers.com> --- dlls/kernel32/tests/loader.c | 51 +++++++++++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 365f4465fc7..ef9c68a7c3a 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -2134,14 +2134,38 @@ static void test_import_resolution(void) char module[16]; struct { WORD hint; char name[32]; } function; IMAGE_TLS_DIRECTORY tls; + UINT_PTR tls_init_fn_list[2]; char tls_data[16]; SHORT tls_index; + SHORT tls_index_hi; + UCHAR tls_init_fn[64]; /* Note: Uses rip-relative address of tls_index, don't separate */ } data, *ptr; IMAGE_NT_HEADERS nt; IMAGE_SECTION_HEADER section; int test; +#if defined(__i386__) + static const UCHAR tls_init_code[] = { + 0xE8, 0x00, 0x00, 0x00, 0x00, /* call 1f */ + 0x59, /* 1: pop ecx */ + 0x8B, 0x49, 0xF7, /* mov ecx, [ecx - 9] ; mov ecx, [tls_index] */ + 0x64, 0x8B, 0x15, 0x2C, 0x00, 0x00, 0x00, /* mov edx, fs:0x2c */ + 0x8B, 0x14, 0x8A, /* mov edx, [edx + edx * 4] */ + 0xC6, 0x42, 0x05, 0x21, /* mov byte [edx + 5], 0x21 */ + 0xC2, 0x0C, 0x00, /* ret 12 */ + }; +#elif defined(__x86_64__) + static const UCHAR tls_init_code[] = { + 0x8B, 0x0D, 0xF6, 0xFF, 0xFF, 0xFF, /* mov ecx, [rip + tls_index] */ + 0x65, 0x48, 0x8B, 0x14, 0x25, 0x58, 0x00, 0x00, 0x00, /* mov rdx, gs:0x58 */ + 0x48, 0x8B, 0x14, 0xCA, /* mov rdx, [rdx + rcx * 8] */ + 0xC6, 0x42, 0x05, 0x21, /* mov byte [rdx + 5], 0x21 */ + 0xC3, /* ret */ + }; +#else + static const UCHAR tls_init_code[] = { 0x00 }; +#endif - for (test = 0; test < 3; test++) + for (test = 0; test < 4; test++) { #define DATA_RVA(ptr) (page_size + ((char *)(ptr) - (char *)&data)) nt = nt_header_template; @@ -2175,6 +2199,15 @@ static void test_import_resolution(void) data.tls.AddressOfIndex = nt.OptionalHeader.ImageBase + DATA_RVA( &data.tls_index ); strcpy( data.tls_data, "hello world" ); data.tls_index = 9999; + data.tls_index_hi = 9999; + + if (test == 3 && sizeof(tls_init_code) > 1) + { + assert(sizeof(tls_init_code) <= sizeof(data.tls_init_fn)); + memcpy(data.tls_init_fn, tls_init_code, sizeof(tls_init_code)); + data.tls_init_fn_list[0] = nt.OptionalHeader.ImageBase + DATA_RVA(&data.tls_init_fn); + data.tls.AddressOfCallBacks = nt.OptionalHeader.ImageBase + DATA_RVA(&data.tls_init_fn_list); + } GetTempPathA(MAX_PATH, temp_path); GetTempFileNameA(temp_path, "ldr", 0, dll_name); @@ -2189,6 +2222,7 @@ static void test_import_resolution(void) section.Misc.VirtualSize = sizeof(data); section.SizeOfRawData = sizeof(data); section.Characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ | IMAGE_SCN_MEM_WRITE; + if (test == 3) section.Characteristics |= IMAGE_SCN_MEM_EXECUTE; WriteFile(hfile, &dos_header, sizeof(dos_header), &dummy, NULL); WriteFile(hfile, &nt, sizeof(nt), &dummy, NULL); @@ -2215,6 +2249,7 @@ static void test_import_resolution(void) { str = ((char **)NtCurrentTeb()->ThreadLocalStoragePointer)[ptr->tls_index]; ok( !strcmp( str, "hello world" ), "wrong tls data '%s' at %p\n", str, str ); + ok(ptr->tls_index_hi == 0, "TLS Index written as a short, high half: %d\n", ptr->tls_index_hi); } FreeLibrary( mod ); break; @@ -2245,6 +2280,20 @@ static void test_import_resolution(void) ok( ptr->tls_index == 9999, "wrong tls index %d\n", ptr->tls_index ); FreeLibrary( mod ); break; + case 3: /* load with tls init function */ + mod = LoadLibraryA( dll_name ); + ok( mod != NULL, "failed to load err %lu\n", GetLastError() ); + if (!mod) break; + ptr = (struct imports *)((char *)mod + page_size); + ok( ptr->tls_index < 32 || broken(ptr->tls_index == 9999), /* before vista */ + "wrong tls index %d\n", ptr->tls_index ); + if (ptr->tls_index != 9999 && sizeof(tls_init_code) > 1) + { + /* tls init function will write an '!' over the space in "hello world" */ + str = ((char **)NtCurrentTeb()->ThreadLocalStoragePointer)[ptr->tls_index]; + ok( !strcmp( str, "hello!world" ), "wrong tls data '%s' at %p\n", str, str ); + } + FreeLibrary( mod ); } DeleteFileA( dll_name ); #undef DATA_RVA -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2361
From: Evan Tang <etang(a)codeweavers.com> Gets us access to a dll with tls to verify --- dlls/kernel32/tests/loader.c | 31 +++++++++++++++++++++++++++++++ dlls/ntdll/tests/rtl.c | 26 -------------------------- 2 files changed, 31 insertions(+), 26 deletions(-) diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index ef9c68a7c3a..745dbfc6e14 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -2117,6 +2117,33 @@ nt4_is_broken: } } +static void check_tls_index(HANDLE dll, BOOL tls_initialized) +{ + BOOL found_dll = FALSE; + LIST_ENTRY *root = &NtCurrentTeb()->Peb->LdrData->InLoadOrderModuleList; + for (LIST_ENTRY *entry = root->Flink; entry != root; entry = entry->Flink) + { + LDR_DATA_TABLE_ENTRY *mod = CONTAINING_RECORD(entry, LDR_DATA_TABLE_ENTRY, InLoadOrderLinks); + if (lstrcmpiW(L"ntdll.dll", mod->BaseDllName.Buffer) == 0) + { + /* Pick ntdll as a dll that definitely won't have TLS */ + ok(mod->TlsIndex == 0, "ntdll.dll TlsIndex: %d instead of 0\n", mod->TlsIndex); + } + else if (mod->DllBase == dll) + { + SHORT expected = tls_initialized ? -1 : 0; + ok(mod->TlsIndex == expected, "Test exe TlsIndex: %d instead of %d\n", mod->TlsIndex, expected); + found_dll = TRUE; + } + else + { + ok(mod->TlsIndex == 0 || mod->TlsIndex == -1, "%s TlsIndex: %d\n", + debugstr_w(mod->BaseDllName.Buffer), mod->TlsIndex); + } + } + ok(found_dll, "Couldn't find dll %p in module list\n", dll); +} + static void test_import_resolution(void) { char temp_path[MAX_PATH]; @@ -2251,6 +2278,7 @@ static void test_import_resolution(void) ok( !strcmp( str, "hello world" ), "wrong tls data '%s' at %p\n", str, str ); ok(ptr->tls_index_hi == 0, "TLS Index written as a short, high half: %d\n", ptr->tls_index_hi); } + check_tls_index(mod, ptr->tls_index != 9999); FreeLibrary( mod ); break; case 1: /* load with DONT_RESOLVE_DLL_REFERENCES doesn't resolve imports */ @@ -2267,6 +2295,7 @@ static void test_import_resolution(void) ok( ptr->thunks[0].u1.Function == 0xdeadbeef, "thunk resolved to %p for %s.%s\n", (void *)ptr->thunks[0].u1.Function, data.module, data.function.name ); ok( ptr->tls_index == 9999, "wrong tls index %d\n", ptr->tls_index ); + check_tls_index(mod, ptr->tls_index != 9999); FreeLibrary( mod2 ); FreeLibrary( mod ); break; @@ -2278,6 +2307,7 @@ static void test_import_resolution(void) ok( ptr->thunks[0].u1.Function == 0xdeadbeef, "thunk resolved to %p for %s.%s\n", (void *)ptr->thunks[0].u1.Function, data.module, data.function.name ); ok( ptr->tls_index == 9999, "wrong tls index %d\n", ptr->tls_index ); + check_tls_index(mod, ptr->tls_index != 9999); FreeLibrary( mod ); break; case 3: /* load with tls init function */ @@ -2293,6 +2323,7 @@ static void test_import_resolution(void) str = ((char **)NtCurrentTeb()->ThreadLocalStoragePointer)[ptr->tls_index]; ok( !strcmp( str, "hello!world" ), "wrong tls data '%s' at %p\n", str, str ); } + check_tls_index(mod, ptr->tls_index != 9999); FreeLibrary( mod ); } DeleteFileA( dll_name ); diff --git a/dlls/ntdll/tests/rtl.c b/dlls/ntdll/tests/rtl.c index 493cdc92cac..3a5564fc6b8 100644 --- a/dlls/ntdll/tests/rtl.c +++ b/dlls/ntdll/tests/rtl.c @@ -3608,31 +3608,6 @@ static void test_RtlFirstFreeAce(void) HeapFree(GetProcessHeap(), 0, acl); } -static void test_TlsIndex(void) -{ - LIST_ENTRY *root = &NtCurrentTeb()->Peb->LdrData->InLoadOrderModuleList; - for (LIST_ENTRY *entry = root->Flink; entry != root; entry = entry->Flink) - { - LDR_DATA_TABLE_ENTRY *mod = CONTAINING_RECORD(entry, LDR_DATA_TABLE_ENTRY, InLoadOrderLinks); - if (lstrcmpiW(L"ntdll.dll", mod->BaseDllName.Buffer) == 0) - { - /* Pick ntdll as a dll that definitely won't have TLS */ - ok(mod->TlsIndex == 0, "ntdll.dll TlsIndex: %d instead of 0\n", mod->TlsIndex); - } - else if (mod->DllBase == GetModuleHandleA(NULL)) - { - /* mingw gcc doesn't support MSVC-style TLS */ - /* If we do get a way to add tls to this exe, uncomment the following test: */ - /* ok(mod->TlsIndex == -1, "Test exe TlsIndex: %d instead of -1\n", mod->TlsIndex); */ - } - else - { - ok(mod->TlsIndex == 0 || mod->TlsIndex == -1, "%s TlsIndex: %d\n", - debugstr_w(mod->BaseDllName.Buffer), mod->TlsIndex); - } - } -} - START_TEST(rtl) { InitFunctionPtrs(); @@ -3677,5 +3652,4 @@ START_TEST(rtl) test_DbgPrint(); test_RtlDestroyHeap(); test_RtlFirstFreeAce(); - test_TlsIndex(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2361
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=130422 Your paranoid android. === w7u_2qxl (32 bit report) === kernel32: loader.c:2324: Test failed: wrong tls data 'hello world' at 00C54600 === w7u_adm (32 bit report) === kernel32: loader.c:2324: Test failed: wrong tls data 'hello world' at 00BE4600 === w7u_el (32 bit report) === kernel32: loader.c:2324: Test failed: wrong tls data 'hello world' at 00C546A0 === w7pro64 (64 bit report) === kernel32: loader.c:2324: Test failed: wrong tls data 'hello world' at 0000000000D405F0
participants (3)
-
Evan Tang -
Evan Tang (＠etang-cw)
-
Marvin