From 9d2eff668454d6ef3b09064af73c6fa5186fd049 Mon Sep 17 00:00:00 2001 From: Corinna Vinschen Date: Fri, 28 Nov 2014 12:10:12 +0000 Subject: [PATCH] * cygheap.cc (init_cygheap::find_tls): Add comment. * cygtls.cc (well_known_dlls): Rephrase comment. (bloda_detect): New function. (_cygtls::call2): Call init_thread and bloda_detect for non-pthread threads only. (_cygtls::remove): Move remove_tls and remove_wq calls up to run first. * miscfuncs.cc (struct pthread_wrapper_arg): Rename from struct thread_wrapper_arg. (pthread_wrapper): Rename from thread_wrapper and drop "static". Fix comment. Drop call to _cygtls::remove. Call api_fatal rather than ExitThread. Explain why. * miscfuncs.h (pthread_wrapper): Declare pthread_wrapper. * thread.cc (pthread::exit): Add a FIXME comment. Call _cygtls::remove before calling ExitThread. --- winsup/cygwin/ChangeLog | 17 +++++++ winsup/cygwin/cygheap.cc | 1 + winsup/cygwin/cygtls.cc | 98 +++++++++++++++++++++----------------- winsup/cygwin/miscfuncs.cc | 24 +++++----- winsup/cygwin/miscfuncs.h | 1 + winsup/cygwin/thread.cc | 4 +- 6 files changed, 88 insertions(+), 57 deletions(-) diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog index edafd57fa..3eb802f39 100644 --- a/winsup/cygwin/ChangeLog +++ b/winsup/cygwin/ChangeLog @@ -1,3 +1,20 @@ +2014-11-28 Corinna Vinschen + + * cygheap.cc (init_cygheap::find_tls): Add comment. + * cygtls.cc (well_known_dlls): Rephrase comment. + (bloda_detect): New function. + (_cygtls::call2): Call init_thread and bloda_detect for non-pthread + threads only. + (_cygtls::remove): Move remove_tls and remove_wq calls up to run first. + * miscfuncs.cc (struct pthread_wrapper_arg): Rename from struct + thread_wrapper_arg. + (pthread_wrapper): Rename from thread_wrapper and drop "static". Fix + comment. Drop call to _cygtls::remove. Call api_fatal rather than + ExitThread. Explain why. + * miscfuncs.h (pthread_wrapper): Declare pthread_wrapper. + * thread.cc (pthread::exit): Add a FIXME comment. Call _cygtls::remove + before calling ExitThread. + 2014-11-27 Corinna Vinschen * mount.cc (mount_info::init): Take bool argument and allow to diff --git a/winsup/cygwin/cygheap.cc b/winsup/cygwin/cygheap.cc index 9ac303eed..594f53c9c 100644 --- a/winsup/cygwin/cygheap.cc +++ b/winsup/cygwin/cygheap.cc @@ -673,6 +673,7 @@ init_cygheap::find_tls (int sig, bool& issig_wait) { __try { + /* Only pthreads have tid set to non-0. */ if (!threadlist[ix]->tid) continue; else if (sigismember (&(threadlist[ix]->sigwait_mask), sig)) diff --git a/winsup/cygwin/cygtls.cc b/winsup/cygwin/cygtls.cc index 2eaae9437..83a62f1ae 100644 --- a/winsup/cygwin/cygtls.cc +++ b/winsup/cygwin/cygtls.cc @@ -1,7 +1,7 @@ /* cygtls.cc Copyright 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012, - 2013 Red Hat, Inc. + 2013, 2014 Red Hat, Inc. This software is a copyrighted work licensed under the terms of the Cygwin license. Please consult the file "CYGWIN_LICENSE" for @@ -38,10 +38,9 @@ dll_cmp (const void *a, const void *b) /* Keep sorted! This is a list of well-known core system DLLs which contain code - whiuch is started in its own thread by the system. Kernel32.dll, - for instance, contains the thread called on every Ctrl-C keypress - in a console window. The DLLs in this list are not recognized as - BLODAs. */ + started in its own thread by the system. Kernel32.dll, for instance, + contains the thread called on every Ctrl-C keypress in a console window. + The DLLs in this list are not recognized as BLODAs. */ const wchar_t *well_known_dlls[] = { L"advapi32.dll", @@ -55,46 +54,58 @@ const wchar_t *well_known_dlls[] = L"ws2_32.dll", }; +/* Optional BLODA detection. The idea is that the function address is supposed + to be within Cygwin itself. This is also true for pthreads, since pthreads + are always calling pthread::thread_init_wrapper() in thread.cc. Therefore, + every function call to a function outside of the Cygwin DLL is potentially + a thread injected into the Cygwin process by some BLODA. + + But that's too simple. Assuming the application itself calls CreateThread, + then this is a bad idea, but not really invalid. So we shouldn't print a + BLODA message if the address is within the loaded image of the application. + Also, ntdll.dll starts threads into the application which */ +static void +bloda_detect (DWORD (*func) (void *, void *)) +{ + PIMAGE_DOS_HEADER img_start = (PIMAGE_DOS_HEADER) + GetModuleHandle (NULL); + PIMAGE_NT_HEADERS32 ntheader = (PIMAGE_NT_HEADERS32) + ((PBYTE) img_start + img_start->e_lfanew); + void *img_end = (void *) ((PBYTE) img_start + + ntheader->OptionalHeader.SizeOfImage); + if (((void *) func < (void *) cygwin_hmodule + || (void *) func > (void *) cygheap) + && ((void *) func < (void *) img_start || (void *) func >= img_end)) + { + MEMORY_BASIC_INFORMATION mbi; + wchar_t modname[PATH_MAX]; + + VirtualQuery ((PVOID) func, &mbi, sizeof mbi); + GetModuleFileNameW ((HMODULE) mbi.AllocationBase, modname, + PATH_MAX); + /* Fetch basename and check against list of above system DLLs. */ + const wchar_t *modbasename = wcsrchr (modname, L'\\') + 1; + if (!bsearch (modbasename, well_known_dlls, + sizeof well_known_dlls / sizeof well_known_dlls[0], + sizeof well_known_dlls[0], dll_cmp)) + small_printf ("\n\nPotential BLODA detected! Thread function " + "called outside of Cygwin DLL:\n %W\n\n", + modname); + } +} + void _cygtls::call2 (DWORD (*func) (void *, void *), void *arg, void *buf) { - init_thread (buf, func); - - /* Optional BLODA detection. The idea is that the function address is - supposed to be within Cygwin itself. This is also true for pthreads, - since pthreads are always calling thread_wrapper in miscfuncs.cc. - Therefore, every function call to a function outside of the Cygwin DLL - is potentially a thread injected into the Cygwin process by some BLODA. - - But that's a bit too simple. Assuming the application itself calls - CreateThread, then this is a bad idea, but not really invalid. So we - shouldn't print a BLODA message if the address is within the loaded - image of the application. Also, ntdll.dll starts threads into the - application which */ - if (detect_bloda) + /* If func is pthread_wrapper, the final stack hasn't been set up yet. + This only happens in pthread_wrapper itself. Thus it doesn't make + sense to call init_thread or perform BLODA detection. pthread_wrapper + eventually calls init_thread by itself. */ + if ((void *) func != (void *) pthread_wrapper) { - PIMAGE_DOS_HEADER img_start = (PIMAGE_DOS_HEADER) GetModuleHandle (NULL); - PIMAGE_NT_HEADERS32 ntheader = (PIMAGE_NT_HEADERS32) - ((PBYTE) img_start + img_start->e_lfanew); - void *img_end = (void *) ((PBYTE) img_start - + ntheader->OptionalHeader.SizeOfImage); - if (((void *) func < (void *) cygwin_hmodule - || (void *) func > (void *) cygheap) - && ((void *) func < (void *) img_start || (void *) func >= img_end)) - { - MEMORY_BASIC_INFORMATION mbi; - wchar_t modname[PATH_MAX]; - - VirtualQuery ((PVOID) func, &mbi, sizeof mbi); - GetModuleFileNameW ((HMODULE) mbi.AllocationBase, modname, PATH_MAX); - /* Fetch basename and check against list of above system DLLs. */ - const wchar_t *modbasename = wcsrchr (modname, L'\\') + 1; - if (!bsearch (modbasename, well_known_dlls, - sizeof well_known_dlls / sizeof well_known_dlls[0], - sizeof well_known_dlls[0], dll_cmp)) - small_printf ("\n\nPotential BLODA detected! Thread function " - "called outside of Cygwin DLL:\n %W\n\n", modname); - } + init_thread (buf, func); + if (detect_bloda) + bloda_detect (func); } DWORD res = func (arg, buf); @@ -169,6 +180,9 @@ _cygtls::remove (DWORD wait) debug_printf ("wait %u", wait); + cygheap->remove_tls (this, INFINITE); + remove_wq (wait); + /* FIXME: Need some sort of atthreadexit function to allow things like select to control this themselves. */ @@ -197,8 +211,6 @@ _cygtls::remove (DWORD wait) /* Close timer handle. */ if (locals.cw_timer) NtClose (locals.cw_timer); - cygheap->remove_tls (this, wait); - remove_wq (wait); } #ifdef __x86_64__ diff --git a/winsup/cygwin/miscfuncs.cc b/winsup/cygwin/miscfuncs.cc index eee47a53a..e6ea5cc2d 100644 --- a/winsup/cygwin/miscfuncs.cc +++ b/winsup/cygwin/miscfuncs.cc @@ -540,7 +540,7 @@ __import_address (void *imp) parameters we don't use and instead to add parameters we need to make the function pthreads compatible. */ -struct thread_wrapper_arg +struct pthread_wrapper_arg { LPTHREAD_START_ROUTINE func; PVOID arg; @@ -549,23 +549,20 @@ struct thread_wrapper_arg PBYTE stacklimit; }; -static DWORD WINAPI -thread_wrapper (PVOID arg) +DWORD WINAPI +pthread_wrapper (PVOID arg) { /* Just plain paranoia. */ if (!arg) return ERROR_INVALID_PARAMETER; - /* The process is now threaded. Note the fact for later usage. */ + /* The process is now threaded. Note for later usage by arc4random. */ __isthreaded = 1; /* Fetch thread wrapper info and free from cygheap. */ - thread_wrapper_arg wrapper_arg = *(thread_wrapper_arg *) arg; + pthread_wrapper_arg wrapper_arg = *(pthread_wrapper_arg *) arg; cfree (arg); - /* Remove _cygtls from this stack since it won't be used anymore. */ - _my_tls.remove (0); - /* Set stack values in TEB */ PTEB teb = NtCurrentTeb (); teb->Tib.StackBase = wrapper_arg.stackbase; @@ -676,8 +673,9 @@ thread_wrapper (PVOID arg) : : [WRAPPER_ARG] "r" (&wrapper_arg), [CYGTLS] "i" (CYGTLS_PADSIZE)); #endif - /* Never return from here. */ - ExitThread (0); + /* pthread::thread_init_wrapper calls pthread::exit, which + in turn calls ExitThread, so we should never arrive here. */ + api_fatal ("Dumb thinko in pthread handling. Whip the developer."); } #ifdef __x86_64__ @@ -751,10 +749,10 @@ CygwinCreateThread (LPTHREAD_START_ROUTINE thread_func, PVOID thread_arg, PVOID real_stackaddr = NULL; ULONG real_stacksize = 0; ULONG real_guardsize = 0; - thread_wrapper_arg *wrapper_arg; + pthread_wrapper_arg *wrapper_arg; HANDLE thread = NULL; - wrapper_arg = (thread_wrapper_arg *) ccalloc (HEAP_STR, 1, + wrapper_arg = (pthread_wrapper_arg *) ccalloc (HEAP_STR, 1, sizeof *wrapper_arg); if (!wrapper_arg) { @@ -846,7 +844,7 @@ CygwinCreateThread (LPTHREAD_START_ROUTINE thread_func, PVOID thread_arg, reserve a 256K stack, not 64K, otherwise the thread creation might crash the process due to a stack overflow. */ thread = CreateThread (&sec_none_nih, 4 * PTHREAD_STACK_MIN, - thread_wrapper, wrapper_arg, + pthread_wrapper, wrapper_arg, creation_flags | STACK_SIZE_PARAM_IS_A_RESERVATION, thread_id); diff --git a/winsup/cygwin/miscfuncs.h b/winsup/cygwin/miscfuncs.h index e42940c1a..c53a520c7 100644 --- a/winsup/cygwin/miscfuncs.h +++ b/winsup/cygwin/miscfuncs.h @@ -66,6 +66,7 @@ ssize_t __reg3 check_iovec (const struct iovec *, int, bool); #define check_iovec_for_read(a, b) check_iovec ((a), (b), false) #define check_iovec_for_write(a, b) check_iovec ((a), (b), true) +extern "C" DWORD WINAPI pthread_wrapper (PVOID arg); extern "C" HANDLE WINAPI CygwinCreateThread (LPTHREAD_START_ROUTINE thread_func, PVOID thread_arg, PVOID stackaddr, ULONG stacksize, ULONG guardsize, diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc index f801d5962..a2e2aeb27 100644 --- a/winsup/cygwin/thread.cc +++ b/winsup/cygwin/thread.cc @@ -543,11 +543,13 @@ pthread::exit (void *value_ptr) { if (is_main_tls) { + /* FIXME: Needs locking. */ _cygtls *dummy = (_cygtls *) malloc (sizeof (_cygtls)); *dummy = *_main_tls; _main_tls = dummy; - _main_tls->initialized = false; + _main_tls->initialized = 0; } + cygtls->remove (INFINITE); ExitThread (0); } }