From: Yifu Wang Subject: [PATCH] msvcr120: Fixed a race condition in Concurrency::critical_section. Message-Id: Date: Tue, 30 Dec 2014 23:45:11 +0000 Fixed a race condition which could corrupt the critical_section data structure and causes critical_section_unlock() to hang. See comment in patch for detail. Added a simple testcase for critical_section. --- dlls/msvcr120/tests/msvcr120.c | 133 +++++++++++++++++++++++++++++++++++++--- dlls/msvcrt/lock.c | 22 ++++++- 2 files changed, 145 insertions(+), 10 deletions(-) From 235231eb0f8ff224992ca2e9eb653c4c10242240 Mon Sep 17 00:00:00 2001 From: Yifu Wang Date: Tue, 30 Dec 2014 15:03:36 -0800 Subject: [PATCH] msvcr120: Fixed a race condition in Concurrency::critical_section. --- dlls/msvcr120/tests/msvcr120.c | 133 +++++++++++++++++++++++++++++++++++++--- dlls/msvcrt/lock.c | 22 ++++++- 2 files changed, 145 insertions(+), 10 deletions(-) diff --git a/dlls/msvcr120/tests/msvcr120.c b/dlls/msvcr120/tests/msvcr120.c index c0a582f..69a24e0 100644 --- a/dlls/msvcr120/tests/msvcr120.c +++ b/dlls/msvcr120/tests/msvcr120.c @@ -16,17 +16,53 @@ * 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 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; +} + +#define call_func1(func,_this) call_thiscall_func1(func,_this) + +#else + +#define init_thiscall_thunk() +#define call_func1(func,_this) func(_this) + +#endif /* __i386__ */ + +#undef __thiscall +#ifdef __i386__ +#define __thiscall __stdcall +#else +#define __thiscall __cdecl +#endif struct MSVCRT_lconv { @@ -58,9 +94,29 @@ struct MSVCRT_lconv wchar_t* _W_negative_sign; }; +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 category, const char* locale); 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 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 BOOL init(void) { @@ -76,6 +132,30 @@ 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"); + } + 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"); +#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"); +#endif + } + init_thiscall_thunk(); + return TRUE; } @@ -148,8 +228,43 @@ static void test_lconv(void) test_lconv_helper(locstrs[i]); } +critical_section critsect; + +#define GET_CS_ITER 100000 +#define GET_CS_THRDS 20 +#define GET_CS_TIMEOUT 120000 + +static DWORD WINAPI get_critical_section(LPVOID arg) { + int i; + for(i = 0; i < GET_CS_ITER; ++ i) { + call_func1(p_critical_section_lock, &critsect); + call_func1(p_critical_section_unlock, &critsect); + } + return 0; +} + +static void test_critical_section(void) { + int i; + DWORD res; + HANDLE thrds[GET_CS_THRDS]; + + call_func1(p_critical_section_ctor, &critsect); + + for(i = 0; i < GET_CS_THRDS; ++ i) + thrds[i] = CreateThread(0, 0, get_critical_section, 0, 0, 0); + + res = WaitForMultipleObjects(GET_CS_THRDS, thrds, TRUE, GET_CS_TIMEOUT); + ok(res == WAIT_OBJECT_0, "execution timed out, critical section probably not functioning correctly\n"); + + for(i = 0; i < GET_CS_THRDS; ++ i) + CloseHandle(thrds[i]); + + 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..30818b5 100644 --- a/dlls/msvcrt/lock.c +++ b/dlls/msvcrt/lock.c @@ -356,6 +356,19 @@ static inline void spin_wait_for_next_cs(cs_queue *q) SpinWait_dtor(&sw); } +static inline void spin_wait_for_next_null(cs_queue *q) +{ + SpinWait sw; + + if(!q->next) return; + + SpinWait_ctor(&sw, &spin_wait_yield); + SpinWait__Reset(&sw); + while(q->next) + SpinWait__SpinOnce(&sw); + SpinWait_dtor(&sw); +} + static inline void cs_set_head(critical_section *cs, cs_queue *q) { cs->unk_thread_id = GetCurrentThreadId(); @@ -380,11 +393,18 @@ void __thiscall critical_section_lock(critical_section *this) memset(&q, 0, sizeof(q)); last = InterlockedExchangePointer(&this->tail, &q); if(last) { + /* If last == &this->unk_active, the current thread might be racing setting + * unk_active.next with another thread calling cs_set_head(). If the current + * thread sets unk_active.next first, the data structure would be left in an + * inconsistent state and causes hang. Spin waiting until unk_active.next + * becomes NULL solves the race condition. + */ + if(last == &this->unk_active) + spin_wait_for_next_null(&this->unk_active); last->next = &q; 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); -- 1.7.1