From: Paul Gofman pgofman@codeweavers.com
--- dlls/kernel32/tests/module.c | 48 ++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)
diff --git a/dlls/kernel32/tests/module.c b/dlls/kernel32/tests/module.c index 35c7c552dd5..23c99d0e084 100644 --- a/dlls/kernel32/tests/module.c +++ b/dlls/kernel32/tests/module.c @@ -1627,6 +1627,53 @@ static void test_ddag_node(void) ok( se == node->Dependencies.Tail, "Expected end of the list.\n" ); }
+static HANDLE test_tls_links_started, test_tls_links_done; + +static DWORD WINAPI test_tls_links_thread(void* tlsidx_v) +{ + SetEvent(test_tls_links_started); + WaitForSingleObject(test_tls_links_done, INFINITE); + return 0; +} + +static void test_tls_links(void) +{ + TEB *teb = NtCurrentTeb(), *thread_teb; + THREAD_BASIC_INFORMATION tbi; + NTSTATUS status; + HANDLE thread; + + todo_wine ok(!!teb->ThreadLocalStoragePointer, "got NULL.\n"); + + test_tls_links_started = CreateEventW(NULL, FALSE, FALSE, NULL); + test_tls_links_done = CreateEventW(NULL, FALSE, FALSE, NULL); + + thread = CreateThread(NULL, 0, test_tls_links_thread, NULL, CREATE_SUSPENDED, NULL); + do + { + /* workaround currently present Wine bug when thread teb may be not available immediately + * after creating a thread before it is initialized on the Unix side. */ + Sleep(1); + status = NtQueryInformationThread(thread, ThreadBasicInformation, &tbi, sizeof(tbi), NULL); + ok(!status, "got %#lx.\n", status ); + } while (!(thread_teb = tbi.TebBaseAddress)); + ok(!thread_teb->ThreadLocalStoragePointer, "got %p.\n", thread_teb->ThreadLocalStoragePointer); + ResumeThread(thread); + WaitForSingleObject(test_tls_links_started, INFINITE); + + todo_wine ok(!!thread_teb->ThreadLocalStoragePointer, "got NULL.\n"); + todo_wine ok(!teb->TlsLinks.Flink, "got %p.\n", teb->TlsLinks.Flink); + todo_wine ok(!teb->TlsLinks.Blink, "got %p.\n", teb->TlsLinks.Blink); + todo_wine ok(!thread_teb->TlsLinks.Flink, "got %p.\n", thread_teb->TlsLinks.Flink); + todo_wine ok(!thread_teb->TlsLinks.Blink, "got %p.\n", thread_teb->TlsLinks.Blink); + SetEvent(test_tls_links_done); + WaitForSingleObject(thread, INFINITE); + + CloseHandle(thread); + CloseHandle(test_tls_links_started); + CloseHandle(test_tls_links_done); +} + START_TEST(module) { WCHAR filenameW[MAX_PATH]; @@ -1663,4 +1710,5 @@ START_TEST(module) test_LdrGetDllFullName(); test_apisets(); test_ddag_node(); + test_tls_links(); }
From: Paul Gofman pgofman@codeweavers.com
--- dlls/kernel32/tests/module.c | 4 ++-- dlls/ntdll/loader.c | 16 +++++++--------- 2 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/dlls/kernel32/tests/module.c b/dlls/kernel32/tests/module.c index 23c99d0e084..f707522772f 100644 --- a/dlls/kernel32/tests/module.c +++ b/dlls/kernel32/tests/module.c @@ -1643,7 +1643,7 @@ static void test_tls_links(void) NTSTATUS status; HANDLE thread;
- todo_wine ok(!!teb->ThreadLocalStoragePointer, "got NULL.\n"); + ok(!!teb->ThreadLocalStoragePointer, "got NULL.\n");
test_tls_links_started = CreateEventW(NULL, FALSE, FALSE, NULL); test_tls_links_done = CreateEventW(NULL, FALSE, FALSE, NULL); @@ -1661,7 +1661,7 @@ static void test_tls_links(void) ResumeThread(thread); WaitForSingleObject(test_tls_links_started, INFINITE);
- todo_wine ok(!!thread_teb->ThreadLocalStoragePointer, "got NULL.\n"); + ok(!!thread_teb->ThreadLocalStoragePointer, "got NULL.\n"); todo_wine ok(!teb->TlsLinks.Flink, "got %p.\n", teb->TlsLinks.Flink); todo_wine ok(!teb->TlsLinks.Blink, "got %p.\n", teb->TlsLinks.Blink); todo_wine ok(!thread_teb->TlsLinks.Flink, "got %p.\n", thread_teb->TlsLinks.Flink); diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 21378102ebd..ebdb08569c0 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -137,7 +137,7 @@ typedef struct _wine_modref BOOL system; } WINE_MODREF;
-static UINT tls_module_count; /* number of modules with TLS directory */ +static UINT tls_module_count = 32; /* number of modules with TLS directory */ static IMAGE_TLS_DIRECTORY *tls_dirs; /* array of TLS directories */ static LIST_ENTRY tls_links = { &tls_links, &tls_links };
@@ -1314,13 +1314,10 @@ static BOOL alloc_tls_slot( LDR_DATA_TABLE_ENTRY *mod )
if (i == tls_module_count) { - UINT new_count = max( 32, tls_module_count * 2 ); + UINT new_count = tls_module_count * 2;
- if (!tls_dirs) - new_ptr = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, new_count * sizeof(*tls_dirs) ); - else - new_ptr = RtlReAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, tls_dirs, - new_count * sizeof(*tls_dirs) ); + new_ptr = RtlReAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, tls_dirs, + new_count * sizeof(*tls_dirs) ); if (!new_ptr) return FALSE;
/* resize the pointer block in all running threads */ @@ -1568,8 +1565,6 @@ static NTSTATUS alloc_thread_tls(void) void **pointers; UINT i, size;
- if (!tls_module_count) return STATUS_SUCCESS; - if (!(pointers = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, tls_module_count * sizeof(*pointers) ))) return STATUS_NO_MEMORY; @@ -4342,6 +4337,9 @@ void loader_init( CONTEXT *context, void **entry ) RtlSetBits( peb->TlsBitmap, 0, NtCurrentTeb()->WowTebOffset ? WOW64_TLS_MAX_NUMBER : 1 ); RtlSetBits( peb->TlsBitmap, NTDLL_TLS_ERRNO, 1 );
+ if (!(tls_dirs = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, tls_module_count * sizeof(*tls_dirs) ))) + NtTerminateProcess( GetCurrentProcess(), STATUS_NO_MEMORY ); + init_user_process_params(); load_global_options(); version_init();
From: Paul Gofman pgofman@codeweavers.com
--- dlls/ntdll/loader.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index ebdb08569c0..5d0f03a76da 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -1294,6 +1294,7 @@ static BOOL alloc_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) ULONG i, size; void *new_ptr; LIST_ENTRY *entry; + UINT old_module_count = tls_module_count;
if (!(dir = RtlImageDirectoryEntryToData( mod->DllBase, TRUE, IMAGE_DIRECTORY_ENTRY_TLS, &size ))) return FALSE; @@ -1319,16 +1320,22 @@ static BOOL alloc_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) new_ptr = RtlReAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, tls_dirs, new_count * sizeof(*tls_dirs) ); if (!new_ptr) return FALSE; + tls_dirs = new_ptr; + tls_module_count = new_count; + }
- /* resize the pointer block in all running threads */ - for (entry = tls_links.Flink; entry != &tls_links; entry = entry->Flink) + /* allocate the data block in all running threads */ + for (entry = tls_links.Flink; entry != &tls_links; entry = entry->Flink) + { + TEB *teb = CONTAINING_RECORD( entry, TEB, TlsLinks ); + + if (old_module_count < tls_module_count) { - TEB *teb = CONTAINING_RECORD( entry, TEB, TlsLinks ); void **old = teb->ThreadLocalStoragePointer; - void **new = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, new_count * sizeof(*new)); + void **new = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, tls_module_count * sizeof(*new));
if (!new) return FALSE; - if (old) memcpy( new, old, tls_module_count * sizeof(*new) ); + if (old) memcpy( new, old, old_module_count * sizeof(*new) ); teb->ThreadLocalStoragePointer = new; #ifdef __x86_64__ /* macOS-specific hack */ if (teb->Instrumentation[0]) ((TEB *)teb->Instrumentation[0])->ThreadLocalStoragePointer = new; @@ -1337,15 +1344,6 @@ static BOOL alloc_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) /* FIXME: can't free old block here, should be freed at thread exit */ }
- tls_dirs = new_ptr; - tls_module_count = new_count; - } - - /* allocate the data block in all running threads */ - for (entry = tls_links.Flink; entry != &tls_links; entry = entry->Flink) - { - TEB *teb = CONTAINING_RECORD( entry, TEB, TlsLinks ); - 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 );
From: Paul Gofman pgofman@codeweavers.com
--- dlls/kernel32/tests/module.c | 8 ++++---- dlls/ntdll/loader.c | 38 ++++++++++++++++++++++++------------ 2 files changed, 29 insertions(+), 17 deletions(-)
diff --git a/dlls/kernel32/tests/module.c b/dlls/kernel32/tests/module.c index f707522772f..6b576ea25fd 100644 --- a/dlls/kernel32/tests/module.c +++ b/dlls/kernel32/tests/module.c @@ -1662,10 +1662,10 @@ static void test_tls_links(void) WaitForSingleObject(test_tls_links_started, INFINITE);
ok(!!thread_teb->ThreadLocalStoragePointer, "got NULL.\n"); - todo_wine ok(!teb->TlsLinks.Flink, "got %p.\n", teb->TlsLinks.Flink); - todo_wine ok(!teb->TlsLinks.Blink, "got %p.\n", teb->TlsLinks.Blink); - todo_wine ok(!thread_teb->TlsLinks.Flink, "got %p.\n", thread_teb->TlsLinks.Flink); - todo_wine ok(!thread_teb->TlsLinks.Blink, "got %p.\n", thread_teb->TlsLinks.Blink); + ok(!teb->TlsLinks.Flink, "got %p.\n", teb->TlsLinks.Flink); + ok(!teb->TlsLinks.Blink, "got %p.\n", teb->TlsLinks.Blink); + ok(!thread_teb->TlsLinks.Flink, "got %p.\n", thread_teb->TlsLinks.Flink); + ok(!thread_teb->TlsLinks.Blink, "got %p.\n", thread_teb->TlsLinks.Blink); SetEvent(test_tls_links_done); WaitForSingleObject(thread, INFINITE);
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 5d0f03a76da..5f84ca7e23b 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -139,7 +139,6 @@ typedef struct _wine_modref
static UINT tls_module_count = 32; /* number of modules with TLS directory */ static IMAGE_TLS_DIRECTORY *tls_dirs; /* array of TLS directories */ -static LIST_ENTRY tls_links = { &tls_links, &tls_links };
static RTL_CRITICAL_SECTION loader_section; static RTL_CRITICAL_SECTION_DEBUG critsect_debug = @@ -1293,8 +1292,8 @@ static BOOL alloc_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) const IMAGE_TLS_DIRECTORY *dir; ULONG i, size; void *new_ptr; - LIST_ENTRY *entry; UINT old_module_count = tls_module_count; + HANDLE thread = NULL, next;
if (!(dir = RtlImageDirectoryEntryToData( mod->DllBase, TRUE, IMAGE_DIRECTORY_ENTRY_TLS, &size ))) return FALSE; @@ -1325,9 +1324,25 @@ static BOOL alloc_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) }
/* allocate the data block in all running threads */ - for (entry = tls_links.Flink; entry != &tls_links; entry = entry->Flink) + while (!NtGetNextThread( GetCurrentProcess(), thread, THREAD_QUERY_LIMITED_INFORMATION, 0, 0, &next )) { - TEB *teb = CONTAINING_RECORD( entry, TEB, TlsLinks ); + THREAD_BASIC_INFORMATION tbi; + TEB *teb; + + if (thread) NtClose( thread ); + thread = next; + if (NtQueryInformationThread( thread, ThreadBasicInformation, &tbi, sizeof(tbi), NULL ) || !tbi.TebBaseAddress) + { + ERR( "NtQueryInformationThread failed.\n" ); + continue; + } + teb = tbi.TebBaseAddress; + if (!teb->ThreadLocalStoragePointer) + { + /* Thread is not initialized by loader yet or already teared down. */ + TRACE( "thread %04lx NULL tls block.\n", HandleToULong(tbi.ClientId.UniqueThread) ); + continue; + }
if (old_module_count < tls_module_count) { @@ -1354,6 +1369,7 @@ static BOOL alloc_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) RtlFreeHeap( GetProcessHeap(), 0, InterlockedExchangePointer( (void **)teb->ThreadLocalStoragePointer + i, new_ptr )); } + if (thread) NtClose( thread );
*(DWORD *)dir->AddressOfIndex = i; tls_dirs[i] = *dir; @@ -3864,9 +3880,13 @@ void WINAPI LdrShutdownThread(void) if (wm->ldr.TlsIndex == -1) call_tls_callbacks( wm->ldr.DllBase, DLL_THREAD_DETACH );
RtlAcquirePebLock(); - if (NtCurrentTeb()->TlsLinks.Flink) RemoveEntryList( &NtCurrentTeb()->TlsLinks ); if ((pointers = NtCurrentTeb()->ThreadLocalStoragePointer)) { + NtCurrentTeb()->ThreadLocalStoragePointer = NULL; +#ifdef __x86_64__ /* macOS-specific hack */ + if (NtCurrentTeb()->Instrumentation[0]) + ((TEB *)NtCurrentTeb()->Instrumentation[0])->ThreadLocalStoragePointer = NULL; +#endif for (i = 0; i < tls_module_count; i++) RtlFreeHeap( GetProcessHeap(), 0, pointers[i] ); RtlFreeHeap( GetProcessHeap(), 0, pointers ); } @@ -4218,10 +4238,6 @@ static void init_wow64( CONTEXT *context ) imports_fixup_done = TRUE; }
- RtlAcquirePebLock(); - InsertHeadList( &tls_links, &NtCurrentTeb()->TlsLinks ); - RtlReleasePebLock(); - RtlLeaveCriticalSection( &loader_section ); pWow64LdrpInitialize( context ); } @@ -4393,10 +4409,6 @@ void loader_init( CONTEXT *context, void **entry ) wm = get_modref( NtCurrentTeb()->Peb->ImageBaseAddress ); }
- RtlAcquirePebLock(); - InsertHeadList( &tls_links, &NtCurrentTeb()->TlsLinks ); - RtlReleasePebLock(); - NtCurrentTeb()->FlsSlots = fls_alloc_data();
if (!attach_done) /* first time around */
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=148020
Your paranoid android.
=== debian11b (64 bit WoW report) ===
user32: msg.c:6935: Test failed: SetFocus(hwnd) on a button: 3: the winevent_hook 0x8005 was expected, but got hook 0x0005 instead msg.c:6935: Test failed: SetFocus(hwnd) on a button: 3: the winevent_hook 0x8005 was expected, but got winevent_hook 0x0003 instead msg.c:6935: Test failed: SetFocus(hwnd) on a button: 3: the winevent_hook 0x8005 was expected, but got msg 0x030f instead msg.c:6935: Test failed: SetFocus(hwnd) on a button: 3: the winevent_hook 0x8005 was expected, but got msg 0x001c instead msg.c:6935: Test failed: SetFocus(hwnd) on a button: 3: the winevent_hook 0x8005 was expected, but got msg 0x0086 instead msg.c:6935: Test failed: SetFocus(hwnd) on a button: 3: the winevent_hook 0x8005 was expected, but got msg 0x0006 instead msg.c:6935: Test failed: SetFocus(hwnd) on a button: 3: the winevent_hook 0x8005 was expected, but got hook 0x0009 instead
Resolves one of the issues with CoD Black Ops Coldwar.
The problem happens when a thread calls TerminateThread() on self. When it is called on another thread the TEB is leaked. alloc_tls_slot() loops through dead threads' TEBs but that doesn't cause obvious problems. However, when TerminateThread() is called on self, loader thread detach is not executed but the TEB is freed in exit_thread. So if some new thread is reusing the same teb it gets linked into TlsLinks list for a second time and then traversing the list in consequent alloc_tls_slot() results in infinite loop.
-
Marvin -
Paul Gofman -
Paul Gofman (＠gofman)