From 386d29001571bd7c504a2a18a588afe997b77e11 Mon Sep 17 00:00:00 2001 From: Norbert Federa Date: Tue, 24 May 2016 13:35:11 +0200 Subject: [PATCH] winpr/synch: fix mutex implementation - Mutex is recursive on Windows; as a consequence we have to use the pthread PTHREAD_MUTEX_RECURSIVE type - Adapt MutexCloseHandle accordingly - ReleaseMutex returned TRUE even if pthread_mutex_unlock failed - Fixed and improved the TestSynchMutex ctest --- winpr/libwinpr/synch/mutex.c | 46 ++---- winpr/libwinpr/synch/test/TestSynchMutex.c | 245 ++++++++++++++++++++++++++--- 2 files changed, 236 insertions(+), 55 deletions(-) diff --git a/winpr/libwinpr/synch/mutex.c b/winpr/libwinpr/synch/mutex.c index a6de42f..6801206 100644 --- a/winpr/libwinpr/synch/mutex.c +++ b/winpr/libwinpr/synch/mutex.c @@ -70,16 +70,9 @@ BOOL MutexCloseHandle(HANDLE handle) if (!MutexIsHandled(handle)) return FALSE; - rc = pthread_mutex_trylock(&mutex->mutex); - switch(rc) + if ((rc = pthread_mutex_destroy(&mutex->mutex))) { - case 0: /* The mutex is now locked. */ - break; - /* If we already own the mutex consider it a success. */ - case EDEADLK: - case EBUSY: - break; - default: + WLog_ERR(TAG, "pthread_mutex_destroy failed with %s [%d]", strerror(rc), rc); #if defined(WITH_DEBUG_MUTEX) { size_t used = 0, i; @@ -98,22 +91,10 @@ BOOL MutexCloseHandle(HANDLE handle) winpr_backtrace_free(stack); } #endif - WLog_ERR(TAG, "pthread_mutex_trylock failed with %s [%d]", strerror(rc), rc); - return FALSE; - } - - rc = pthread_mutex_unlock(&mutex->mutex); - if (rc != 0) - { - WLog_ERR(TAG, "pthread_mutex_unlock failed with %s [%d]", strerror(rc), rc); - return FALSE; - } - - rc = pthread_mutex_destroy(&mutex->mutex); - if (rc != 0) - { - WLog_ERR(TAG, "pthread_mutex_destroy failed with %s [%d]", strerror(rc), rc); - return FALSE; + /** + * Note: unfortunately we may not return FALSE here since CloseHandle(hmutex) on + * Windows always seems to succeed independently of the mutex object locking state + */ } free(handle); @@ -138,7 +119,10 @@ HANDLE CreateMutexW(LPSECURITY_ATTRIBUTES lpMutexAttributes, BOOL bInitialOwner, if (mutex) { - pthread_mutex_init(&mutex->mutex, 0); + pthread_mutexattr_t attr; + pthread_mutexattr_init(&attr); + pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); + pthread_mutex_init(&mutex->mutex, &attr); WINPR_HANDLE_SET_TYPE_AND_MODE(mutex, HANDLE_TYPE_MUTEX, WINPR_FD_READ); mutex->ops = &ops; @@ -181,15 +165,19 @@ BOOL ReleaseMutex(HANDLE hMutex) { ULONG Type; WINPR_HANDLE* Object; - WINPR_MUTEX* mutex; if (!winpr_Handle_GetInfo(hMutex, &Type, &Object)) return FALSE; if (Type == HANDLE_TYPE_MUTEX) { - mutex = (WINPR_MUTEX*) Object; - pthread_mutex_unlock(&mutex->mutex); + WINPR_MUTEX* mutex = (WINPR_MUTEX*) Object; + int rc = pthread_mutex_unlock(&mutex->mutex); + if (rc) + { + WLog_ERR(TAG, "pthread_mutex_unlock failed with %s [%d]", strerror(rc), rc); + return FALSE; + } return TRUE; } diff --git a/winpr/libwinpr/synch/test/TestSynchMutex.c b/winpr/libwinpr/synch/test/TestSynchMutex.c index 22983a6..a7461ee 100644 --- a/winpr/libwinpr/synch/test/TestSynchMutex.c +++ b/winpr/libwinpr/synch/test/TestSynchMutex.c @@ -1,53 +1,246 @@ #include #include +#include -int TestSynchMutex(int argc, char* argv[]) + +BOOL test_mutex_basic() { - DWORD rc; HANDLE mutex; + DWORD rc; - mutex = CreateMutex(NULL, FALSE, NULL); - if (!mutex) + if (!(mutex = CreateMutex(NULL, FALSE, NULL))) { - printf("CreateMutex failure\n"); - return -1; + printf("%s: CreateMutex failed\n", __FUNCTION__); + return FALSE; } - /* Lock the mutex */ rc = WaitForSingleObject(mutex, INFINITE); - if (WAIT_OBJECT_0 != rc) + if (rc != WAIT_OBJECT_0) + { + printf("%s: WaitForSingleObject on mutex failed with %u\n", __FUNCTION__, rc); + return FALSE; + } + + if (!ReleaseMutex(mutex)) + { + printf("%s: ReleaseMutex failed\n", __FUNCTION__); + return FALSE; + } + + if (ReleaseMutex(mutex)) + { + printf("%s: ReleaseMutex unexpectedly succeeded on released mutex\n", __FUNCTION__); + return FALSE; + } + + if (!CloseHandle(mutex)) + { + printf("%s: CloseHandle on mutex failed\n", __FUNCTION__); + return FALSE; + } + return TRUE; +} + +BOOL test_mutex_recursive() +{ + HANDLE mutex; + DWORD rc, i, cnt = 50; + + if (!(mutex = CreateMutex(NULL, TRUE, NULL))) + { + printf("%s: CreateMutex failed\n", __FUNCTION__); + return FALSE; + } + + for (i = 0; i < cnt; i++) + { + rc = WaitForSingleObject(mutex, INFINITE); + if (rc != WAIT_OBJECT_0) + { + printf("%s: WaitForSingleObject #%u on mutex failed with %u\n", __FUNCTION__, i, rc); + return FALSE; + } + } + + for (i = 0; i < cnt; i++) { - printf("WaitForSingleObject on mutex failed with %d\n", rc); - return -2; + if (!ReleaseMutex(mutex)) + { + printf("%s: ReleaseMutex #%u failed\n", __FUNCTION__, i); + return FALSE; + } } - /* TryLock should now fail. */ - rc = WaitForSingleObject(mutex, 0); - if (WAIT_TIMEOUT != rc) + if (!ReleaseMutex(mutex)) { - printf("Timed WaitForSingleObject on locked mutex failed with %d\n", rc); - return -3; + /* Note: The mutex was initially owned ! */ + printf("%s: Final ReleaseMutex failed\n", __FUNCTION__); + return FALSE; } - /* Unlock the mutex */ - rc = ReleaseMutex(mutex); - if (!rc) + if (ReleaseMutex(mutex)) { - printf("ReleaseMutex failed.\n"); - return -4; + printf("%s: ReleaseMutex unexpectedly succeeded on released mutex\n", __FUNCTION__); + return FALSE; } - /* TryLock should now succeed. */ - rc = WaitForSingleObject(mutex, 0); - if (WAIT_OBJECT_0 != rc) + if (!CloseHandle(mutex)) { - printf("Timed WaitForSingleObject on free mutex failed with %d\n", rc); - return -5; + printf("%s: CloseHandle on mutex failed\n", __FUNCTION__); + return FALSE; } + return TRUE; +} - CloseHandle(mutex); + +HANDLE thread1_mutex1 = NULL; +HANDLE thread1_mutex2 = NULL; +BOOL thread1_failed = TRUE; + +DWORD WINAPI test_mutex_thread1(LPVOID lpParam) +{ + HANDLE hStartEvent = (HANDLE)lpParam; + DWORD rc = 0; + if (WaitForSingleObject(hStartEvent, INFINITE) != WAIT_OBJECT_0) + { + fprintf(stderr, "%s: failed to wait for start event\n", __FUNCTION__); + return 0; + } + + /** + * at this point: + * thread1_mutex1 is expected to be locked + * thread1_mutex2 is expected to be unlocked + * defined task: + * try to lock thread1_mutex1 (expected to fail) + * lock and unlock thread1_mutex2 (expected to work) + */ + + rc = WaitForSingleObject(thread1_mutex1, 10); + if (rc != WAIT_TIMEOUT) + { + fprintf(stderr, "%s: WaitForSingleObject on thread1_mutex1 unexpectedly returned %u instead of WAIT_TIMEOUT (%u)\n", + __FUNCTION__, rc, WAIT_TIMEOUT); + return 0; + } + + rc = WaitForSingleObject(thread1_mutex2, 10); + if (rc != WAIT_OBJECT_0) + { + fprintf(stderr, "%s: WaitForSingleObject on thread1_mutex2 unexpectedly returned %u instead of WAIT_OBJECT_0\n", + __FUNCTION__, rc); + return 0; + } + + if (!ReleaseMutex(thread1_mutex2)) { + fprintf(stderr, "%s: ReleaseMutex failed on thread1_mutex2\n", __FUNCTION__); + return 0; + } + + thread1_failed = FALSE; return 0; } +BOOL test_mutex_threading() +{ + HANDLE hThread = NULL; + HANDLE hStartEvent = NULL; + + if (!(thread1_mutex1 = CreateMutex(NULL, TRUE, NULL))) + { + printf("%s: CreateMutex thread1_mutex1 failed\n", __FUNCTION__); + goto fail; + } + + if (!(thread1_mutex2 = CreateMutex(NULL, FALSE, NULL))) + { + printf("%s: CreateMutex thread1_mutex2 failed\n", __FUNCTION__); + goto fail; + } + + if (!(hStartEvent = CreateEvent(NULL, TRUE, FALSE, NULL))) + { + fprintf(stderr, "%s: error creating start event\n", __FUNCTION__); + goto fail; + } + + thread1_failed = TRUE; + + if (!(hThread = CreateThread(NULL, 0, test_mutex_thread1, (LPVOID)hStartEvent, 0, NULL))) + { + fprintf(stderr, "%s: error creating test_mutex_thread_1\n", __FUNCTION__); + goto fail; + } + + Sleep(100); + + if (!thread1_failed) + { + fprintf(stderr, "%s: thread1 premature success\n", __FUNCTION__); + goto fail; + } + + SetEvent(hStartEvent); + + if (WaitForSingleObject(hThread, 2000) != WAIT_OBJECT_0) + { + fprintf(stderr, "%s: thread1 premature success\n", __FUNCTION__); + goto fail; + } + + if (thread1_failed) + { + fprintf(stderr, "%s: thread1 has not reported success\n", __FUNCTION__); + goto fail; + } + + /** + * - thread1 must not have succeeded to lock thread1_mutex1 + * - thread1 must have locked and unlocked thread1_mutex2 + */ + + if (!ReleaseMutex(thread1_mutex1)) + { + printf("%s: ReleaseMutex unexpectedly failed on thread1_mutex1\n", __FUNCTION__); + goto fail; + } + + if (ReleaseMutex(thread1_mutex2)) + { + printf("%s: ReleaseMutex unexpectedly succeeded on thread1_mutex2\n", __FUNCTION__); + goto fail; + } + + CloseHandle(hThread); + CloseHandle(hStartEvent); + CloseHandle(thread1_mutex1); + CloseHandle(thread1_mutex2); + + return TRUE; + +fail: + ReleaseMutex(thread1_mutex1); + ReleaseMutex(thread1_mutex2); + CloseHandle(thread1_mutex1); + CloseHandle(thread1_mutex2); + CloseHandle(hStartEvent); + CloseHandle(hThread); + return FALSE; +} + +int TestSynchMutex(int argc, char* argv[]) +{ + if (!test_mutex_basic()) + return 1; + + if (!test_mutex_recursive()) + return 2; + + if (!test_mutex_threading()) + return 3; + + printf("TestSynchMutex succeeded\n"); + return 0; +} -- 2.7.4