indmanager: Forcefully recreate mutexes on indication stop

Creating mutexes with the PTHREAD_MUTEX_ERRORCHECK attribute proved to be
a wrong way. It turned out locking was not working properly as any attempt
to lock already locked mutex failed with a return value that we didn't check.
The only reason we were using this special attribute was to prevent crash on
forcefully unlocking mutex in an unknown state. That didn't work either as long
as EPERM was returned when trying to unlock the mutex that has been locked from
other thread... that was forcefully canceled. That only led to deadlocks
fortunately hard to hit, unfortunately equally hard to debug.

2 files changed, 29 insertions, 14 deletions

diff --git a/src/indmanager/ind_manager.c b/src/indmanager/ind_manager.c
index 186f4da..ccb6a2e 100644
--- a/src/indmanager/ind_manager.c
+++ b/src/indmanager/ind_manager.c
@@ -791,6 +791,18 @@ static bool remove_all_filters(IMManager *manager, IMError *err)
     return true;
 }
 
+static bool recreate_mutex(IMManager *manager, IMError *err, bool free)
+{
+    if (!free) {
+        if (pthread_mutex_destroy(&manager->_t_mutex))
+            return false;
+    }
+    if (pthread_mutex_init(&manager->_t_mutex, NULL))
+        return false;
+
+    return true;
+}
+
 IMManager* im_create_manager(IMInstGather gather, IMFilterChecker f_checker,
                              bool polling, IMEventWatcher watcher,
                              IMIndType type, const CMPIBroker *broker,
@@ -827,10 +839,8 @@ IMManager* im_create_manager(IMInstGather gather, IMFilterChecker f_checker,
     manager->data = NULL;
     DEBUG("Manager created");
 
-    if (pthread_mutexattr_init(&manager->_t_mutex_attr) ||
-        pthread_mutexattr_settype(&manager->_t_mutex_attr, PTHREAD_MUTEX_ERRORCHECK) ||
-        pthread_mutex_init(&manager->_t_mutex, &manager->_t_mutex_attr) ||
-        pthread_cond_init(&manager->_t_cond, NULL)) {
+    if (pthread_cond_init(&manager->_t_cond, NULL) ||
+        !recreate_mutex(manager, err, false)) {
             *err = IM_ERR_THREAD;
             free(manager);
             return NULL;
@@ -850,16 +860,14 @@ bool im_destroy_manager(IMManager *manager, const CMPIContext *ctx,
         *err = IM_ERR_CONTEXT;
         return false;
     }
-    im_stop_ind(manager, ctx, err);
-    pthread_mutex_lock(&manager->_t_mutex);
-    if (!remove_all_filters(manager, err)) {
-        pthread_mutex_unlock(&manager->_t_mutex);
+    if (manager->running)
+        im_stop_ind(manager, ctx, err);
+    /* No need for locking as all running threads should be stopped at this point */
+    if (!remove_all_filters(manager, err))
         return false;
-    }
-    pthread_mutex_unlock(&manager->_t_mutex);
     DEBUG("Destroying manager");
     if (pthread_mutex_destroy(&manager->_t_mutex) ||
-        pthread_mutexattr_destroy(&manager->_t_mutex_attr)) {
+        pthread_cond_destroy(&manager->_t_mutex)) {
         *err = IM_ERR_THREAD;
         return false;
     }
@@ -1188,6 +1196,7 @@ bool im_start_ind(IMManager *manager, const CMPIContext *ctx, IMError *err)
     }
     DEBUG("Starting indications");
     manager->running = true;
+
     pthread_cond_signal(&manager->_t_cond);
     return true;
 }
@@ -1220,9 +1229,16 @@ bool im_stop_ind(IMManager *manager, const CMPIContext *ctx, IMError *err)
     /* Cleanup */
     remove_all_enums(manager, err);
     manager->data = NULL;
+    /* TODO: properly detach the thread using CBAttachThread(), needs to be called from the thread */
     manager->ctx_manage = NULL;
-    /* Mutex could have been in locked state when the thread was canceled */
-    pthread_mutex_unlock(&manager->_t_mutex);
+    /* No other thread should be accessing the mutex now and we need to forcefully recreate it
+     * as long as it could have been left in a locked state when the thread was canceled.
+     * Can't use pthread_mutex_unlock() since it returns EPERM when called from a thread
+     * that doesn't own the lock. */
+    if (!recreate_mutex(manager, err, true)) {
+        *err = IM_ERR_THREAD;
+        return false;
+    }
 
     return true;
 }
diff --git a/src/indmanager/ind_manager.h b/src/indmanager/ind_manager.h
index 383f4b6..733cf7a 100644
--- a/src/indmanager/ind_manager.h
+++ b/src/indmanager/ind_manager.h
@@ -143,7 +143,6 @@ struct _IMManager {
     // threading
     pthread_t _t_manage;
     pthread_mutex_t _t_mutex;
-    pthread_mutexattr_t _t_mutex_attr;
     pthread_cond_t _t_cond;
     // passed data, used for communication between gather/watcher/etc.
     void *data;