[PATCH v4 0/4] MR7251: TLS slots alignment.
This MR enforces alignment of TLS slots as described in PE file. -- v4: ntdll: Let layout of TLS vector be closer to native. ntdll: Enforce the alignment of TLS directory entries. kernel32: Add test for TLS memory layout. kernel32/tests: Add a test about TLS slot alignment. https://gitlab.winehq.org/wine/wine/-/merge_requests/7251
From: Eric Pouech <epouech(a)codeweavers.com> Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/kernel32/tests/loader.c | 39 ++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 5eef938ca1f..d0249b35b97 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -2107,6 +2107,34 @@ static void check_tls_index(HANDLE dll, BOOL tls_initialized) ok(found_dll, "Couldn't find dll %p in module list\n", dll); } +static BOOL is_old_loader_struct(void) +{ + LDR_DATA_TABLE_ENTRY *mod, *mod2; + LDR_DDAG_NODE *ddag_node; + NTSTATUS status; + HMODULE hexe; + + /* Check for old LDR data strcuture. */ + hexe = GetModuleHandleW( NULL ); + ok( !!hexe, "Got NULL exe handle.\n" ); + status = LdrFindEntryForAddress( hexe, &mod ); + ok( !status, "got %#lx.\n", status ); + if (!(ddag_node = mod->DdagNode)) + { + win_skip( "DdagNode is NULL, skipping tests.\n" ); + return TRUE; + } + ok( !!ddag_node->Modules.Flink, "Got NULL module link.\n" ); + mod2 = CONTAINING_RECORD(ddag_node->Modules.Flink, LDR_DATA_TABLE_ENTRY, NodeModuleLink); + ok( mod2 == mod || broken( (void **)mod2 == (void **)mod - 1 ), "got %p, expected %p.\n", mod2, mod ); + if (mod2 != mod) + { + win_skip( "Old LDR_DATA_TABLE_ENTRY structure, skipping tests.\n" ); + return TRUE; + } + return FALSE; +} + static int tls_init_fn_output; static DWORD WINAPI tls_thread_fn(void* tlsidx_v) @@ -2230,6 +2258,7 @@ static void test_import_resolution(void) ADD_RELOC( tls.EndAddressOfRawData ); data.tls.AddressOfIndex = nt.OptionalHeader.ImageBase + DATA_RVA( &data.tls_index ); ADD_RELOC( tls.AddressOfIndex ); + data.tls.Characteristics = IMAGE_SCN_ALIGN_1024BYTES; strcpy( data.tls_data, "hello world" ); data.tls_index = 9999; data.tls_index_hi = 9999; @@ -2293,6 +2322,16 @@ static void test_import_resolution(void) ok( ptr->tls_index < 32, "wrong tls index %d\n", ptr->tls_index ); str = ((char **)NtCurrentTeb()->ThreadLocalStoragePointer)[ptr->tls_index]; ok( !strcmp( str, "hello world" ), "wrong tls data '%s' at %p\n", str, str ); + if (!is_old_loader_struct()) + { + SIZE_T s; + /* should be aligned on 1024 bytes, 10 lower bits at 0 */ + todo_wine_if( (DWORD_PTR)str & 0x3ff ) + ok( !((DWORD_PTR)str & 0x3ff), "wrong alignment %p\n", str ); + s = HeapSize( GetProcessHeap(), 0, ((void **)str)[-1] ); + todo_wine + ok( s != (SIZE_T)-1, "Error %Ix\n", s); + } 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 ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7251
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/kernel32/tests/module.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/dlls/kernel32/tests/module.c b/dlls/kernel32/tests/module.c index f26c412f748..b3ac6917a8f 100644 --- a/dlls/kernel32/tests/module.c +++ b/dlls/kernel32/tests/module.c @@ -1708,7 +1708,10 @@ static void test_tls_links(void) TEB *teb = NtCurrentTeb(), *thread_teb; THREAD_BASIC_INFORMATION tbi; NTSTATUS status; + ULONG i, count; HANDLE thread; + SIZE_T size; + void **ptr; ok(!!teb->ThreadLocalStoragePointer, "got NULL.\n"); @@ -1728,6 +1731,23 @@ static void test_tls_links(void) ResumeThread(thread); WaitForSingleObject(test_tls_links_started, INFINITE); + if (!is_old_loader_struct()) + { + ptr = teb->ThreadLocalStoragePointer; + count = (ULONG)(ULONG_PTR)ptr[-2]; + size = HeapSize(GetProcessHeap(), 0, ptr - 2); + todo_wine + ok(size == (count + 2) * sizeof(void *), "got count %lu, size %Iu.\n", count, size); + + for (i = 0; i < count; ++i) + { + if (!ptr[i]) continue; + todo_wine + size = HeapSize(GetProcessHeap(), 0, ((void **)ptr[i])[-1]); + ok(size && size < 100000, "got %Iu.\n", size); + } + } + ok(!!thread_teb->ThreadLocalStoragePointer, "got NULL.\n"); ok(!teb->TlsLinks.Flink, "got %p.\n", teb->TlsLinks.Flink); ok(!teb->TlsLinks.Blink, "got %p.\n", teb->TlsLinks.Blink); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7251
From: Eric Pouech <epouech(a)codeweavers.com> Required for some Chrome integration. Wine-Bugs: https://bugs.winehq.org/show_bug.cgi?id=57700 Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/kernel32/tests/loader.c | 2 -- dlls/kernel32/tests/module.c | 1 - dlls/ntdll/loader.c | 70 +++++++++++++++++++++++++++--------- 3 files changed, 54 insertions(+), 19 deletions(-) diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index d0249b35b97..38ee1f0a6c7 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -2326,10 +2326,8 @@ static void test_import_resolution(void) { SIZE_T s; /* should be aligned on 1024 bytes, 10 lower bits at 0 */ - todo_wine_if( (DWORD_PTR)str & 0x3ff ) ok( !((DWORD_PTR)str & 0x3ff), "wrong alignment %p\n", str ); s = HeapSize( GetProcessHeap(), 0, ((void **)str)[-1] ); - todo_wine ok( s != (SIZE_T)-1, "Error %Ix\n", s); } ok(ptr->tls_index_hi == 0, "TLS Index written as a short, high half: %d\n", ptr->tls_index_hi); diff --git a/dlls/kernel32/tests/module.c b/dlls/kernel32/tests/module.c index b3ac6917a8f..9deb9463c7f 100644 --- a/dlls/kernel32/tests/module.c +++ b/dlls/kernel32/tests/module.c @@ -1742,7 +1742,6 @@ static void test_tls_links(void) for (i = 0; i < count; ++i) { if (!ptr[i]) continue; - todo_wine size = HeapSize(GetProcessHeap(), 0, ((void **)ptr[i])[-1]); ok(size && size < 100000, "got %Iu.\n", size); } diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 67c4aa546bc..5d386f49130 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -1314,6 +1314,52 @@ static BOOL is_dll_native_subsystem( LDR_DATA_TABLE_ENTRY *mod, const IMAGE_NT_H return TRUE; } +/* use in pair heap_(alloc|free)_aligned to manage aligned memory allocations */ +static void *heap_alloc_aligned( unsigned size, unsigned align ) +{ + char *ptr; + DWORD_PTR mask = ((DWORD_PTR)1 << align) - 1; + + size += sizeof(void *) + mask; + if ((ptr = RtlAllocateHeap( GetProcessHeap(), 0, size ))) + { + void *unaligned_ptr = ptr; + + /* align block and store unaligned pointer just below the returned block */ + ptr = (char *)(((DWORD_PTR)ptr + sizeof(void *) + mask) & ~mask); + ((void **)ptr)[-1] = unaligned_ptr; + } + return ptr; +} + + +static BOOLEAN heap_free_aligned( void *ptr ) +{ + return ptr ? RtlFreeHeap( GetProcessHeap(), 0, ((void **)ptr)[-1] ) : TRUE; +} + + +/* Allocates the slot from a TLS directory (potentially realigning if necessary) + * - must be freed with heap_free_aligned + */ +static void *alloc_thread_tls_slot( const IMAGE_TLS_DIRECTORY *dir ) +{ + UINT size = dir->EndAddressOfRawData - dir->StartAddressOfRawData; + unsigned align; + void *ptr; + + align = (dir->Characteristics & IMAGE_SCN_ALIGN_MASK) >> 20; + if (!align) align = 5; /* default */ + if ((ptr = heap_alloc_aligned( size + dir->SizeOfZeroFill, align ))) + { + memcpy( ptr, (void *)dir->StartAddressOfRawData, size ); + memset( (char *)ptr + size, 0, dir->SizeOfZeroFill ); + } + + return ptr; +} + + /************************************************************************* * alloc_tls_slot * @@ -1389,15 +1435,12 @@ static BOOL alloc_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) /* FIXME: can't free old block here, should be freed at thread exit */ } - if (!(new_ptr = RtlAllocateHeap( GetProcessHeap(), 0, size + dir->SizeOfZeroFill ))) return -1; - memcpy( new_ptr, (void *)dir->StartAddressOfRawData, size ); - memset( (char *)new_ptr + size, 0, dir->SizeOfZeroFill ); + if (!(new_ptr = alloc_thread_tls_slot( dir ))) return -1; TRACE( "thread %04lx slot %lu: %lu/%lu bytes at %p\n", HandleToULong(teb->ClientId.UniqueThread), i, size, dir->SizeOfZeroFill, new_ptr ); - RtlFreeHeap( GetProcessHeap(), 0, - InterlockedExchangePointer( (void **)teb->ThreadLocalStoragePointer + i, new_ptr )); + heap_free_aligned( InterlockedExchangePointer( (void **)teb->ThreadLocalStoragePointer + i, new_ptr )); } if (thread) NtClose( thread ); @@ -1604,7 +1647,7 @@ static WINE_MODREF *alloc_module( HMODULE hModule, const UNICODE_STRING *nt_name static NTSTATUS alloc_thread_tls(void) { void **pointers; - UINT i, size; + UINT i; if (!(pointers = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, tls_module_count * sizeof(*pointers) ))) @@ -1614,20 +1657,15 @@ static NTSTATUS alloc_thread_tls(void) { const IMAGE_TLS_DIRECTORY *dir = &tls_dirs[i]; - if (!dir) continue; - size = dir->EndAddressOfRawData - dir->StartAddressOfRawData; - if (!size && !dir->SizeOfZeroFill) continue; + if (!dir || (dir->EndAddressOfRawData == dir->StartAddressOfRawData && !dir->SizeOfZeroFill)) continue; - if (!(pointers[i] = RtlAllocateHeap( GetProcessHeap(), 0, size + dir->SizeOfZeroFill ))) + if (!(pointers[i] = alloc_thread_tls_slot( dir ))) { - while (i) RtlFreeHeap( GetProcessHeap(), 0, pointers[--i] ); + while (i) heap_free_aligned( pointers[--i] ); RtlFreeHeap( GetProcessHeap(), 0, pointers ); return STATUS_NO_MEMORY; } - memcpy( pointers[i], (void *)dir->StartAddressOfRawData, size ); - memset( (char *)pointers[i] + size, 0, dir->SizeOfZeroFill ); - - TRACE( "slot %u: %u/%lu bytes at %p\n", i, size, dir->SizeOfZeroFill, pointers[i] ); + TRACE( "slot %u: %u/%lu bytes at %p\n", i, (UINT)(dir->EndAddressOfRawData - dir->StartAddressOfRawData), dir->SizeOfZeroFill, pointers[i] ); } NtCurrentTeb()->ThreadLocalStoragePointer = pointers; return STATUS_SUCCESS; @@ -3934,7 +3972,7 @@ void WINAPI LdrShutdownThread(void) if ((pointers = NtCurrentTeb()->ThreadLocalStoragePointer)) { NtCurrentTeb()->ThreadLocalStoragePointer = NULL; - for (i = 0; i < tls_module_count; i++) RtlFreeHeap( GetProcessHeap(), 0, pointers[i] ); + for (i = 0; i < tls_module_count; i++) heap_free_aligned( pointers[i] ); RtlFreeHeap( GetProcessHeap(), 0, pointers ); } RtlProcessFlsData( NtCurrentTeb()->FlsSlots, 2 ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7251
From: Eric Pouech <epouech(a)codeweavers.com> Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/kernel32/tests/module.c | 1 - dlls/ntdll/loader.c | 37 +++++++++++++++++++++++++++++++----- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/dlls/kernel32/tests/module.c b/dlls/kernel32/tests/module.c index 9deb9463c7f..5c2fe51b4de 100644 --- a/dlls/kernel32/tests/module.c +++ b/dlls/kernel32/tests/module.c @@ -1736,7 +1736,6 @@ static void test_tls_links(void) ptr = teb->ThreadLocalStoragePointer; count = (ULONG)(ULONG_PTR)ptr[-2]; size = HeapSize(GetProcessHeap(), 0, ptr - 2); - todo_wine ok(size == (count + 2) * sizeof(void *), "got count %lu, size %Iu.\n", count, size); for (i = 0; i < count; ++i) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 5d386f49130..9670d0503a4 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -1360,6 +1360,34 @@ static void *alloc_thread_tls_slot( const IMAGE_TLS_DIRECTORY *dir ) } +/* native places a control structure just below the TLS array */ +struct tls_array_control +{ + DWORD count; + void *unknown; +}; + + +static void *heap_alloc_tls_array( unsigned count ) +{ + C_ASSERT(sizeof(struct tls_array_control) == 2 * sizeof(void*)); + struct tls_array_control *ctrl; + + if ((ctrl = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, (2 + count) * sizeof(void*) ))) + { + ctrl->count = count; + ctrl = ctrl + 1; + } + return ctrl; +} + + +static void heap_free_tls_array( void *ptr ) +{ + RtlFreeHeap( GetProcessHeap(), 0, ((struct tls_array_control *)ptr) - 1); +} + + /************************************************************************* * alloc_tls_slot * @@ -1426,7 +1454,7 @@ static BOOL alloc_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) if (old_module_count < tls_module_count) { void **old = teb->ThreadLocalStoragePointer; - void **new = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, tls_module_count * sizeof(*new)); + void **new = heap_alloc_tls_array( tls_module_count ); if (!new) return FALSE; if (old) memcpy( new, old, old_module_count * sizeof(*new) ); @@ -1649,8 +1677,7 @@ static NTSTATUS alloc_thread_tls(void) void **pointers; UINT i; - if (!(pointers = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, - tls_module_count * sizeof(*pointers) ))) + if (!(pointers = heap_alloc_tls_array( tls_module_count ))) return STATUS_NO_MEMORY; for (i = 0; i < tls_module_count; i++) @@ -1662,7 +1689,7 @@ static NTSTATUS alloc_thread_tls(void) if (!(pointers[i] = alloc_thread_tls_slot( dir ))) { while (i) heap_free_aligned( pointers[--i] ); - RtlFreeHeap( GetProcessHeap(), 0, pointers ); + heap_free_tls_array( pointers ); return STATUS_NO_MEMORY; } TRACE( "slot %u: %u/%lu bytes at %p\n", i, (UINT)(dir->EndAddressOfRawData - dir->StartAddressOfRawData), dir->SizeOfZeroFill, pointers[i] ); @@ -3973,7 +4000,7 @@ void WINAPI LdrShutdownThread(void) { NtCurrentTeb()->ThreadLocalStoragePointer = NULL; for (i = 0; i < tls_module_count; i++) heap_free_aligned( pointers[i] ); - RtlFreeHeap( GetProcessHeap(), 0, pointers ); + heap_free_tls_array( pointers ); } RtlProcessFlsData( NtCurrentTeb()->FlsSlots, 2 ); NtCurrentTeb()->FlsSlots = NULL; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7251
rebased because of conflicting commit -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7251#note_100136
What's the status of this PR? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7251#note_109905
maybe @julliard could update on the status? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7251#note_110161
participants (4)
-
Alisa Sireneva (＠purplesyringa) -
Eric Pouech
-
eric pouech (＠epo)
-
Paul Gofman