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

diff --git a/winpr/libwinpr/synch/mutex.c b/winpr/libwinpr/synch/mutex.c
index a6de42f..6801206 100644 (file)
--- 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 (file)
--- a/winpr/libwinpr/synch/test/TestSynchMutex.c
+++ b/winpr/libwinpr/synch/test/TestSynchMutex.c
@@ -1,53 +1,246 @@
 
 #include <winpr/crt.h>
 #include <winpr/synch.h>
+#include <winpr/thread.h>
 
-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;
+}