indicationmgr: fixed deadlock when re-starting the namager thread.

When the indication manager thread was stopped, it destroys its mutex and
creates new one. If pthread_mutex_destroy() fails because the mutex is
locked, the new mutex is not created -> the old one is used, but it's already
locked -> deadlock.

So let's make sure the thread does not hold the mutex when destroying the thread.

Rewrite of indication manager thread is needed to have the manage() function
interruptible when waiting for events, e.g. using a pipe and select(),
this pthread_cancel leads to memory leaks.

1 files changed, 10 insertions, 25 deletions

diff --git a/src/indmanager/ind_manager.c b/src/indmanager/ind_manager.c
index 7d6bc75..188139a 100644
--- a/src/indmanager/ind_manager.c
+++ b/src/indmanager/ind_manager.c
@@ -791,18 +791,6 @@ static bool remove_all_filters(IMManager *manager, IMError *err)
     return true;
 }
 
-static bool recreate_mutex(IMManager *manager, IMError *err, bool do_free)
-{
-    if (do_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,
@@ -840,10 +828,10 @@ IMManager* im_create_manager(IMInstGather gather, IMFilterChecker f_checker,
     DEBUG("Manager created");
 
     if (pthread_cond_init(&manager->_t_cond, NULL) ||
-        !recreate_mutex(manager, err, false)) {
-            *err = IM_ERR_THREAD;
-            free(manager);
-            return NULL;
+            pthread_mutex_init(&manager->_t_mutex, NULL)) {
+        *err = IM_ERR_THREAD;
+        free(manager);
+        return NULL;
     }
 
     return manager;
@@ -1217,11 +1205,16 @@ bool im_stop_ind(IMManager *manager, const CMPIContext *ctx, IMError *err)
     DEBUG("Stopping indications");
     manager->running = false;
 
-    /* No locking here due to potential deadlock */
+    /* We must lock the mutex so we are sure the thread does not hold it.
+     * If we destroy the thread with mutex locked, the mutex is locked forever.
+     */
+    pthread_mutex_lock(&manager->_t_mutex);
     if (pthread_cancel(manager->_t_manage)) {
         *err = IM_ERR_THREAD;
         return false;
     }
+    pthread_mutex_unlock(&manager->_t_mutex);
+
     if (pthread_join(manager->_t_manage, NULL)) {
         *err = IM_ERR_THREAD;
         return false;
@@ -1232,14 +1225,6 @@ bool im_stop_ind(IMManager *manager, const CMPIContext *ctx, IMError *err)
     manager->data = NULL;
     /* TODO: properly detach the thread using CBAttachThread(), needs to be called from the thread */
     manager->ctx_manage = NULL;
-    /* 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;
 }