[PATCH v5 0/2] MR1578: ntdll: TlsIndex should not actually contain tls indices
(The TlsIndex field in the LDR_DATA_TABLE_ENTRY structure appears to be unused except as a flag that the module has TLS (being always set to -1), at least as far back as Windows XP. It is worth mentioning that the WINE implementation of implicit TLS incorrectly uses TlsIndex as the real module TLS index, so it may be unreliable to assume that it is always -1 if you care about working on WINE.)
and the "links to that article but still doesn't work in wine" award goes to... [the D runtime](https://github.com/dlang/dmd/blob/6bf60ea0eb174631ede0074a77d3898d943e0b30/d... (Admittedly, there aren't too many ways to do what they're trying to do.) With this, the D runtime will now work in Wine, even if in a dll loaded into an exe with no tls (which gets it the tls index 0) The changes to the debugger are a bit icky, a possible alternative is to find some other easily-debugger-accessible place to stuff the tls index. -- v5: ntdll: TlsIndex should not actually contain tls indices winedbg: Don't expect TlsIndex to contain tls indices https://gitlab.winehq.org/wine/wine/-/merge_requests/1578
From: Evan Tang <etang(a)codeweavers.com> It doesn't on Windows and won't soon in wine --- programs/winedbg/symbol.c | 58 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/programs/winedbg/symbol.c b/programs/winedbg/symbol.c index 961dc95585a..5c6bebf47a6 100644 --- a/programs/winedbg/symbol.c +++ b/programs/winedbg/symbol.c @@ -61,6 +61,51 @@ static BOOL symbol_get_debug_start(const struct dbg_type* func, ULONG64* start) return FALSE; } +static BOOL read_image_directory_data(ULONG64 modbase, WORD dir, void* buffer, size_t* size, BOOL* is_64) +{ + IMAGE_DOS_HEADER dos_header; + IMAGE_DATA_DIRECTORY entry; + DWORD_PTR addr; + union { IMAGE_NT_HEADERS32 h32; IMAGE_NT_HEADERS64 h64; } nt_header; + + addr = (DWORD_PTR)modbase; + if (!dbg_read_memory((void*)addr, &dos_header, sizeof(dos_header))) + return FALSE; + if (dos_header.e_magic != IMAGE_DOS_SIGNATURE) + return FALSE; + addr = (DWORD_PTR)(modbase + dos_header.e_lfanew); + if (!dbg_read_memory((void*)addr, &nt_header, sizeof(nt_header))) + return FALSE; + if (nt_header.h32.Signature != IMAGE_NT_SIGNATURE) + return FALSE; + if (nt_header.h32.OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) + { + if (dir >= nt_header.h32.OptionalHeader.NumberOfRvaAndSizes) + return FALSE; + if (!(entry = nt_header.h32.OptionalHeader.DataDirectory[dir]).VirtualAddress) + return FALSE; + *is_64 = FALSE; + } + else if (nt_header.h64.OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) + { + if (dir >= nt_header.h64.OptionalHeader.NumberOfRvaAndSizes) + return FALSE; + if (!(entry = nt_header.h64.OptionalHeader.DataDirectory[dir]).VirtualAddress) + return FALSE; + *is_64 = TRUE; + } + else + { + return FALSE; + } + if (entry.Size < *size) + *size = entry.Size; + addr = (DWORD_PTR)(modbase + entry.VirtualAddress); + if (!dbg_read_memory((void*)addr, buffer, *size)) + return FALSE; + return TRUE; +} + static BOOL fill_sym_lvalue(const SYMBOL_INFO* sym, ULONG_PTR base, struct dbg_lvalue* lvalue, char* buffer, size_t sz) { @@ -130,6 +175,9 @@ static BOOL fill_sym_lvalue(const SYMBOL_INFO* sym, ULONG_PTR base, { PROCESS_BASIC_INFORMATION pbi; THREAD_BASIC_INFORMATION tbi; + union { IMAGE_TLS_DIRECTORY32 dir32; IMAGE_TLS_DIRECTORY64 dir64; } tls; + size_t read_size = sizeof(tls); + BOOL is_dir64; DWORD_PTR addr; PEB peb; PEB_LDR_DATA ldr_data; @@ -159,7 +207,15 @@ static BOOL fill_sym_lvalue(const SYMBOL_INFO* sym, ULONG_PTR base, &ldr_module, sizeof(ldr_module))) goto tls_error; if ((DWORD_PTR)ldr_module.DllBase == sym->ModBase) { - tlsindex = ldr_module.TlsIndex; + if (ldr_module.TlsIndex != -1) + goto tls_error; + if (!read_image_directory_data(sym->ModBase, IMAGE_DIRECTORY_ENTRY_TLS, &tls, &read_size, &is_dir64)) + goto tls_error; + if (read_size < (is_dir64 ? sizeof(IMAGE_TLS_DIRECTORY64) : sizeof(IMAGE_TLS_DIRECTORY32))) + goto tls_error; + addr = (DWORD_PTR)(is_dir64 ? tls.dir64.AddressOfIndex : tls.dir32.AddressOfIndex); + if (!dbg_read_memory((void*)addr, &tlsindex, sizeof(tlsindex))) + goto tls_error; break; } current = ldr_module.InLoadOrderLinks.Flink; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1578
From: Evan Tang <etang(a)codeweavers.com> It actually contains a -1 if the module has a tls slot and a 0 if it doesn't. Putting tls indices in it breaks initialization of the D runtime if a D dll is loaded into a tls-free exe and gets assigned tls slot 0, as it makes the D runtime think the OS hasn't initialized a tls slot: https://github.com/dlang/dmd/blob/6bf60ea0eb174631ede0074a77d3898d943e0b30/d... --- dlls/ntdll/loader.c | 35 +++++++++++++++++++++-------------- dlls/ntdll/tests/rtl.c | 19 +++++++++++++++++++ 2 files changed, 40 insertions(+), 14 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 895a3f5b138..51e8519abc5 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -1222,7 +1222,7 @@ static BOOL is_dll_native_subsystem( LDR_DATA_TABLE_ENTRY *mod, const IMAGE_NT_H * Allocate a TLS slot for a newly-loaded module. * The loader_section must be locked while calling this function. */ -static SHORT alloc_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) +static BOOL alloc_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) { const IMAGE_TLS_DIRECTORY *dir; ULONG i, size; @@ -1230,10 +1230,10 @@ static SHORT alloc_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) LIST_ENTRY *entry; if (!(dir = RtlImageDirectoryEntryToData( mod->DllBase, TRUE, IMAGE_DIRECTORY_ENTRY_TLS, &size ))) - return -1; + return FALSE; size = dir->EndAddressOfRawData - dir->StartAddressOfRawData; - if (!size && !dir->SizeOfZeroFill && !dir->AddressOfCallBacks) return -1; + if (!size && !dir->SizeOfZeroFill && !dir->AddressOfCallBacks) return FALSE; for (i = 0; i < tls_module_count; i++) { @@ -1255,7 +1255,7 @@ static SHORT alloc_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) else new_ptr = RtlReAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, tls_dirs, new_count * sizeof(*tls_dirs) ); - if (!new_ptr) return -1; + if (!new_ptr) return FALSE; /* resize the pointer block in all running threads */ for (entry = tls_links.Flink; entry != &tls_links; entry = entry->Flink) @@ -1264,7 +1264,7 @@ static SHORT alloc_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) void **old = teb->ThreadLocalStoragePointer; void **new = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, new_count * sizeof(*new)); - if (!new) return -1; + if (!new) return FALSE; if (old) memcpy( new, old, tls_module_count * sizeof(*new) ); teb->ThreadLocalStoragePointer = new; #ifdef __x86_64__ /* macOS-specific hack */ @@ -1296,7 +1296,7 @@ static SHORT alloc_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) *(DWORD *)dir->AddressOfIndex = i; tls_dirs[i] = *dir; - return i; + return TRUE; } @@ -1308,9 +1308,15 @@ static SHORT alloc_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) */ static void free_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) { - ULONG i = (USHORT)mod->TlsIndex; + const IMAGE_TLS_DIRECTORY *dir; + ULONG i, size; + + if (mod->TlsIndex != -1) + return; + if (!(dir = RtlImageDirectoryEntryToData( mod->DllBase, TRUE, IMAGE_DIRECTORY_ENTRY_TLS, &size ))) + return; - if (mod->TlsIndex == -1) return; + i = *(ULONG*)dir->AddressOfIndex; assert( i < tls_module_count ); memset( &tls_dirs[i], 0, sizeof(tls_dirs[i]) ); } @@ -1374,7 +1380,8 @@ static NTSTATUS fixup_imports( WINE_MODREF *wm, LPCWSTR load_path ) if (!(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) return STATUS_SUCCESS; /* already done */ wm->ldr.Flags &= ~LDR_DONT_RESOLVE_REFS; - wm->ldr.TlsIndex = alloc_tls_slot( &wm->ldr ); + if (alloc_tls_slot( &wm->ldr )) + wm->ldr.TlsIndex = -1; if (!(imports = RtlImageDirectoryEntryToData( wm->ldr.DllBase, TRUE, IMAGE_DIRECTORY_ENTRY_IMPORT, &size ))) @@ -1431,7 +1438,7 @@ static WINE_MODREF *alloc_module( HMODULE hModule, const UNICODE_STRING *nt_name wm->ldr.DllBase = hModule; wm->ldr.SizeOfImage = nt->OptionalHeader.SizeOfImage; wm->ldr.Flags = LDR_DONT_RESOLVE_REFS | (builtin ? LDR_WINE_INTERNAL : 0); - wm->ldr.TlsIndex = -1; + wm->ldr.TlsIndex = 0; wm->ldr.LoadCount = 1; wm->CheckSum = nt->OptionalHeader.CheckSum; wm->ldr.TimeDateStamp = nt->FileHeader.TimeDateStamp; @@ -1783,7 +1790,7 @@ NTSTATUS WINAPI LdrDisableThreadCalloutsForDll(HMODULE hModule) RtlEnterCriticalSection( &loader_section ); wm = get_modref( hModule ); - if (!wm || wm->ldr.TlsIndex != -1) + if (!wm || wm->ldr.TlsIndex == -1) ret = STATUS_DLL_NOT_FOUND; else wm->ldr.Flags |= LDR_NO_DLL_CALLS; @@ -3712,7 +3719,7 @@ void WINAPI LdrShutdownThread(void) DLL_THREAD_DETACH, NULL ); } - if (wm->ldr.TlsIndex != -1) call_tls_callbacks( wm->ldr.DllBase, DLL_THREAD_DETACH ); + if (wm->ldr.TlsIndex == -1) call_tls_callbacks( wm->ldr.DllBase, DLL_THREAD_DETACH ); RtlAcquirePebLock(); if (NtCurrentTeb()->TlsLinks.Flink) RemoveEntryList( &NtCurrentTeb()->TlsLinks ); @@ -4216,7 +4223,7 @@ void WINAPI LdrInitializeThunk( CONTEXT *context, ULONG_PTR unknown2, ULONG_PTR NtTerminateProcess( GetCurrentProcess(), status ); } release_address_space(); - if (wm->ldr.TlsIndex != -1) call_tls_callbacks( wm->ldr.DllBase, DLL_PROCESS_ATTACH ); + if (wm->ldr.TlsIndex == -1) call_tls_callbacks( wm->ldr.DllBase, DLL_PROCESS_ATTACH ); if (wm->ldr.ActivationContext) RtlDeactivateActivationContext( 0, cookie ); process_breakpoint(); } @@ -4225,7 +4232,7 @@ void WINAPI LdrInitializeThunk( CONTEXT *context, ULONG_PTR unknown2, ULONG_PTR if ((status = alloc_thread_tls()) != STATUS_SUCCESS) NtTerminateThread( GetCurrentThread(), status ); thread_attach(); - if (wm->ldr.TlsIndex != -1) call_tls_callbacks( wm->ldr.DllBase, DLL_THREAD_ATTACH ); + if (wm->ldr.TlsIndex == -1) call_tls_callbacks( wm->ldr.DllBase, DLL_THREAD_ATTACH ); } RtlLeaveCriticalSection( &loader_section ); diff --git a/dlls/ntdll/tests/rtl.c b/dlls/ntdll/tests/rtl.c index 2e0bfb650e4..7ffdc9b9382 100644 --- a/dlls/ntdll/tests/rtl.c +++ b/dlls/ntdll/tests/rtl.c @@ -3730,6 +3730,24 @@ 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(); @@ -3774,4 +3792,5 @@ START_TEST(rtl) test_DbgPrint(); test_RtlDestroyHeap(); test_RtlFirstFreeAce(); + test_TlsIndex(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1578
dbghelp.ImageDirectoryEntryToDataEx does most of the painful job for you
I didn't see anything in there that indicated that it would properly read memory from the target process
(line 216 in winedbg/symbol: addr = (DWORD_PTR)(is_dir64 ? tls.dir64.AddressOfIndex : tls.dir32.AddressOfIndex);) doesn't seem to be relocated
I think it is. You can see ntdll functions dereferencing it directly here https://gitlab.winehq.org/wine/wine/-/blob/7acd3aa429f2d45d96f4699906dc81e47... I couldn't figure out how to load a pdb in winedbg to test it though (or is there another way to get debug symbols that contain msvc-style tls) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1578#note_19192
I tested the proposed patch with a simple .exe containing TLS variables the attached patch is needed on top of yours to make it work[missing](/uploads/42cc9b24aa94243c17ef04dceb509163/missing) (and it could be better written <g>) I still think we do too many things just to get the address of the tls. This calls for shadowing in winedbg some module information (here this could be the address of AddressOfIndex). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1578#note_19327
participants (3)
-
eric pouech (＠epo) -
Evan Tang
-
Evan Tang (＠etang-cw)