From: Yifu Wang Subject: [PATCH] msvcr120: Fixed bugs in Concurrency::critical_section. (resend) Message-Id: Date: Sat, 3 Jan 2015 01:22:04 +0000 critical_section_lock/try_lock/try_lock_for(): - Fixed a race condition which would corrupt the critical_section data structure and cause critical_section_unlock() to hang. critical_section_try_lock_for(): - Fixed a bug which would hang the program in the event where timer expires. Critical_section_unlock(): - Fixed a leak. --- dlls/msvcr120/tests/msvcr120.c | 203 +++++++++++++++++++++++++++++++++++++-- dlls/msvcrt/lock.c | 29 ++++-- 2 files changed, 210 insertions(+), 22 deletions(-) From 835a92db3f4359e0fd83eae23aca1a4fa54ccb84 Mon Sep 17 00:00:00 2001 From: Yifu Wang Date: Tue, 30 Dec 2014 15:03:36 -0800 Subject: [PATCH] msvcr120: Fixed bugs in Concurrency::critical_section. --- dlls/msvcr120/tests/msvcr120.c | 203 +++++++++++++++++++++++++++++++++++++-- dlls/msvcrt/lock.c | 29 ++++-- 2 files changed, 210 insertions(+), 22 deletions(-) diff --git a/dlls/msvcr120/tests/msvcr120.c b/dlls/msvcr120/tests/msvcr120.c index c0a582f..9d6006e 100644 --- a/dlls/msvcr120/tests/msvcr120.c +++ b/dlls/msvcr120/tests/msvcr120.c @@ -16,17 +16,59 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA */ -#include -#include -#include -#include -#include - -#include -#include #include "wine/test.h" -#include +#include "locale.h" + +/* Emulate a __thiscall */ +#ifdef __i386__ + +#include "pshpack1.h" +struct thiscall_thunk +{ + BYTE pop_eax; /* popl %eax (ret addr) */ + BYTE pop_edx; /* popl %edx (func) */ + BYTE pop_ecx; /* popl %ecx (this) */ + BYTE push_eax; /* pushl %eax */ + WORD jmp_edx; /* jmp *%edx */ +}; +#include "poppack.h" + +static ULONG_PTR (WINAPI *call_thiscall_func1)( void *func, void *this ); +static ULONG_PTR (WINAPI *call_thiscall_func2_uint)( void *func, void *this, unsigned int a); + +static void init_thiscall_thunk(void) +{ + struct thiscall_thunk *thunk = VirtualAlloc( NULL, sizeof(*thunk), + MEM_COMMIT, PAGE_EXECUTE_READWRITE ); + thunk->pop_eax = 0x58; /* popl %eax */ + thunk->pop_edx = 0x5a; /* popl %edx */ + thunk->pop_ecx = 0x59; /* popl %ecx */ + thunk->push_eax = 0x50; /* pushl %eax */ + thunk->jmp_edx = 0xe2ff; /* jmp *%edx */ + call_thiscall_func1 = (void *)thunk; + call_thiscall_func2_uint = (void *)thunk; +} + +#define call_func1(func,_this) call_thiscall_func1(func,_this) +#define call_func2_uint(func,_this,a) call_thiscall_func2_uint(func,_this,a) + +#else + +#define init_thiscall_thunk() +#define call_func1(func,_this) func(_this) +#define call_func2_uint(func,_this,a) func(_this,a) + +#endif /* __i386__ */ + +#undef __thiscall +#ifdef __i386__ +#define __thiscall __stdcall +#else +#define __thiscall __cdecl +#endif + +typedef unsigned char MSVCRT_bool; struct MSVCRT_lconv { @@ -58,9 +100,31 @@ struct MSVCRT_lconv wchar_t* _W_negative_sign; }; -static char* (CDECL *p_setlocale)(int category, const char* locale); +typedef struct cs_queue +{ + struct cs_queue *next; + BOOL free; + int unknown; +} cs_queue; + +typedef struct +{ + ULONG_PTR unk_thread_id; + cs_queue unk_active; + void *unknown[2]; + cs_queue *head; + void *tail; +} critical_section; + +static char* (CDECL *p_setlocale)(int, const char*); static struct MSVCRT_lconv* (CDECL *p_localeconv)(void); -static size_t (CDECL *p_wcstombs_s)(size_t *ret, char* dest, size_t sz, const wchar_t* src, size_t max); +static size_t (CDECL *p_wcstombs_s)(size_t*, char*, size_t, const wchar_t*, size_t); +static void (__thiscall *p_critical_section_ctor)(critical_section*); +static void (__thiscall *p_critical_section_dtor)(critical_section*); +static void (__thiscall *p_critical_section_lock)(critical_section*); +static void (__thiscall *p_critical_section_unlock)(critical_section*); +static MSVCRT_bool (__thiscall *p_critical_section_try_lock)(critical_section*); +static MSVCRT_bool (__thiscall *p_critical_section_try_lock_for)(critical_section*, unsigned int); static BOOL init(void) { @@ -76,6 +140,36 @@ static BOOL init(void) p_setlocale = (void*)GetProcAddress(module, "setlocale"); p_localeconv = (void*)GetProcAddress(module, "localeconv"); p_wcstombs_s = (void*)GetProcAddress(module, "wcstombs_s"); + + if(sizeof(void*) == 8) + { + p_critical_section_ctor = (void*)GetProcAddress(module, "??0critical_section@Concurrency@@QEAA@XZ"); + p_critical_section_dtor = (void*)GetProcAddress(module, "??1critical_section@Concurrency@@QEAA@XZ"); + p_critical_section_lock = (void*)GetProcAddress(module, "?lock@critical_section@Concurrency@@QEAAXXZ"); + p_critical_section_unlock = (void*)GetProcAddress(module, "?unlock@critical_section@Concurrency@@QEAAXXZ"); + p_critical_section_try_lock = (void*)GetProcAddress(module, "?try_lock@critical_section@Concurrency@@QEAA_NXZ"); + p_critical_section_try_lock_for = (void*)GetProcAddress(module, "?try_lock_for@critical_section@Concurrency@@QEAA_NI@Z"); + } + else + { +#ifdef __arm__ + p_critical_section_ctor = (void*)GetProcAddress(module, "??0critical_section@Concurrency@@QAA@XZ"); + p_critical_section_dtor = (void*)GetProcAddress(module, "??1critical_section@Concurrency@@QAA@XZ"); + p_critical_section_lock = (void*)GetProcAddress(module, "?lock@critical_section@Concurrency@@QAAXXZ"); + p_critical_section_unlock = (void*)GetProcAddress(module, "?unlock@critical_section@Concurrency@@QAAXXZ"); + p_critical_section_try_lock = (void*)GetProcAddress(module, "?try_lock@critical_section@Concurrency@@QAA_NXZ"); + p_critical_section_try_lock_for = (void*)GetProcAddress(module, "?try_lock_for@critical_section@Concurrency@@QAA_NI@Z"); +#else + p_critical_section_ctor = (void*)GetProcAddress(module, "??0critical_section@Concurrency@@QAE@XZ"); + p_critical_section_dtor = (void*)GetProcAddress(module, "??1critical_section@Concurrency@@QAE@XZ"); + p_critical_section_lock = (void*)GetProcAddress(module, "?lock@critical_section@Concurrency@@QAEXXZ"); + p_critical_section_unlock = (void*)GetProcAddress(module, "?unlock@critical_section@Concurrency@@QAEXXZ"); + p_critical_section_try_lock = (void*)GetProcAddress(module, "?try_lock@critical_section@Concurrency@@QAE_NXZ"); + p_critical_section_try_lock_for = (void*)GetProcAddress(module, "?try_lock_for@critical_section@Concurrency@@QAE_NI@Z"); +#endif + } + init_thiscall_thunk(); + return TRUE; } @@ -148,8 +242,95 @@ static void test_lconv(void) test_lconv_helper(locstrs[i]); } +critical_section critsect; + +#define CS_ITER 20000 +#define CS_NUM_THRDS 4 + +static int cs_counter = 0; + +static DWORD WINAPI lock_unlock_1(LPVOID arg) { + int i; + for(i = 0; i < CS_ITER; ++ i) { + call_func1(p_critical_section_lock, &critsect); + ++ cs_counter; + call_func1(p_critical_section_unlock, &critsect); + } + return 0; +} + +static DWORD WINAPI lock_unlock_2(LPVOID arg) { + int i = 0; + MSVCRT_bool res; + while(i < CS_ITER) { + res = (MSVCRT_bool)call_func1(p_critical_section_try_lock, &critsect); + if(res) { + ++ cs_counter; + call_func1(p_critical_section_unlock, &critsect); + ++ i; + } + } + return 0; +} + +static DWORD WINAPI lock_unlock_3(LPVOID arg) { + int i = 0; + MSVCRT_bool res; + while(i < CS_ITER) { + res = (MSVCRT_bool)call_func2_uint(p_critical_section_try_lock_for, &critsect, 2); + if(res) { + ++ cs_counter; + call_func1(p_critical_section_unlock, &critsect); + ++ i; + } + } + return 0; +} + +static DWORD WINAPI test_try_lock(LPVOID arg) { + MSVCRT_bool res; + + res = (MSVCRT_bool)call_func1(p_critical_section_try_lock, &critsect); + ok(res == FALSE, "critical_section_try_lock() not functioning correctly\n"); + + res = (MSVCRT_bool)call_func2_uint(p_critical_section_try_lock_for, &critsect, 200); + ok(res == FALSE, "critical_section_try_lock_for() not functioning correctly\n"); + + return 0; +} + +static void test_critical_section(void) { + int i; + HANDLE thrds[CS_NUM_THRDS]; + + call_func1(p_critical_section_ctor, &critsect); + + for(i = 0; i < CS_NUM_THRDS / 3; ++ i) + thrds[i] = CreateThread(0, 0, lock_unlock_1, 0, 0, 0); + + for(i = CS_NUM_THRDS / 3; i < CS_NUM_THRDS / 3 * 2; ++ i) + thrds[i] = CreateThread(0, 0, lock_unlock_2, 0, 0, 0); + + for(i = CS_NUM_THRDS / 3 * 2; i < CS_NUM_THRDS; ++ i) + thrds[i] = CreateThread(0, 0, lock_unlock_3, 0, 0, 0); + + WaitForMultipleObjects(CS_NUM_THRDS, thrds, TRUE, INFINITE); + ok(cs_counter == CS_NUM_THRDS * CS_ITER, + "incorrect counter value, critical section not functioning correctly\n"); + + for(i = 0; i < CS_NUM_THRDS; ++ i) + CloseHandle(thrds[i]); + + call_func1(p_critical_section_lock, &critsect); + WaitForSingleObject(CreateThread(0, 0, test_try_lock, 0, 0, 0), INFINITE); + call_func1(p_critical_section_unlock, &critsect); + + call_func1(p_critical_section_dtor, &critsect); +} + START_TEST(msvcr120) { if (!init()) return; test_lconv(); + test_critical_section(); } diff --git a/dlls/msvcrt/lock.c b/dlls/msvcrt/lock.c index 328b5b5..6d903dc 100644 --- a/dlls/msvcrt/lock.c +++ b/dlls/msvcrt/lock.c @@ -384,10 +384,11 @@ void __thiscall critical_section_lock(critical_section *this) NtWaitForKeyedEvent(keyed_event, &q, 0, NULL); } - this->unk_active.next = NULL; - if(InterlockedCompareExchangePointer(&this->tail, &this->unk_active, &q) != &q) - spin_wait_for_next_cs(&q); cs_set_head(this, &q); + if(InterlockedCompareExchangePointer(&this->tail, &this->unk_active, &q) != &q) { + spin_wait_for_next_cs(&q); + this->unk_active.next = q.next; + } } /* ?try_lock@critical_section@Concurrency@@QAE_NXZ */ @@ -406,11 +407,11 @@ MSVCRT_bool __thiscall critical_section_try_lock(critical_section *this) memset(&q, 0, sizeof(q)); if(!InterlockedCompareExchangePointer(&this->tail, &q, NULL)) { - this->unk_active.next = NULL; - if(InterlockedCompareExchangePointer(&this->tail, &this->unk_active, &q) != &q) - spin_wait_for_next_cs(&q); - cs_set_head(this, &q); + if(InterlockedCompareExchangePointer(&this->tail, &this->unk_active, &q) != &q) { + spin_wait_for_next_cs(&q); + this->unk_active.next = q.next; + } return TRUE; } return FALSE; @@ -437,8 +438,10 @@ void __thiscall critical_section_unlock(critical_section *this) break; next = this->unk_active.next; - if(InterlockedCompareExchangePointer(&this->tail, NULL, next) == next) + if(InterlockedCompareExchangePointer(&this->tail, NULL, next) == next) { + HeapFree(GetProcessHeap(), 0, next); return; + } spin_wait_for_next_cs(next); this->unk_active.next = next->next; @@ -491,14 +494,18 @@ MSVCRT_bool __thiscall critical_section_try_lock_for( if(status == STATUS_TIMEOUT) { if(!InterlockedExchange(&q->free, TRUE)) return FALSE; + /* A thread has signaled the event and is block waiting. */ + /* We need to catch the event to wake the thread. */ + NtWaitForKeyedEvent(keyed_event, q, 0, NULL); } } - this->unk_active.next = NULL; - if(InterlockedCompareExchangePointer(&this->tail, &this->unk_active, q) != q) + cs_set_head(this, q); + if(InterlockedCompareExchangePointer(&this->tail, &this->unk_active, q) != q) { spin_wait_for_next_cs(q); + this->unk_active.next = q->next; + } - cs_set_head(this, q); HeapFree(GetProcessHeap(), 0, q); return TRUE; } -- 1.7.1