Fix deadlock in IMC/IMV managers
authorTobias Brunner <tobias@strongswan.org>
Tue, 18 Dec 2012 14:50:08 +0000 (15:50 +0100)
committerTobias Brunner <tobias@strongswan.org>
Tue, 18 Dec 2012 14:59:29 +0000 (15:59 +0100)
Since reserve_id() might be called from e.g. notify_connection_change()
using a write lock will not work as this can't be acquired while holding
the read lock.

Also, with the previous code it was possible that two IMCs/IMVs added by
two threads at the same time would get the same ID assigned.

src/libcharon/plugins/tnc_imc/tnc_imc_manager.c
src/libcharon/plugins/tnc_imv/tnc_imv_manager.c

index bdb20d5..078f7bc 100644 (file)
@@ -22,6 +22,7 @@
 #include <daemon.h>
 #include <utils/debug.h>
 #include <threading/rwlock.h>
+#include <threading/mutex.h>
 #include <collections/linked_list.h>
 
 typedef struct private_tnc_imc_manager_t private_tnc_imc_manager_t;
@@ -50,15 +51,25 @@ struct private_tnc_imc_manager_t {
         * Next IMC ID to be assigned
         */
        TNC_IMCID next_imc_id;
+
+       /**
+        * Mutex to access next IMC ID
+        */
+       mutex_t *id_mutex;
 };
 
 METHOD(imc_manager_t, add, bool,
        private_tnc_imc_manager_t *this, imc_t *imc)
 {
        TNC_Version version;
+       TNC_IMCID imc_id;
 
-       imc->set_id(imc, this->next_imc_id);
-       if (imc->initialize(imc->get_id(imc), TNC_IFIMC_VERSION_1,
+       this->id_mutex->lock(this->id_mutex);
+       imc_id = this->next_imc_id++;
+       this->id_mutex->unlock(this->id_mutex);
+
+       imc->set_id(imc, imc_id);
+       if (imc->initialize(imc_id, TNC_IFIMC_VERSION_1,
                                                TNC_IFIMC_VERSION_1, &version) != TNC_RESULT_SUCCESS)
        {
                DBG1(DBG_TNC, "IMC \"%s\" failed to initialize", imc->get_name(imc));
@@ -66,7 +77,6 @@ METHOD(imc_manager_t, add, bool,
        }
        this->lock->write_lock(this->lock);
        this->imcs->insert_last(this->imcs, imc);
-       this->next_imc_id++;
        this->lock->unlock(this->lock);
 
        if (imc->provide_bind_function(imc->get_id(imc),
@@ -189,14 +199,16 @@ METHOD(imc_manager_t, reserve_id, bool,
        imc_t *imc;
        bool found = FALSE;
 
-       this->lock->write_lock(this->lock);
+       this->lock->read_lock(this->lock);
        enumerator = this->imcs->create_enumerator(this->imcs);
        while (enumerator->enumerate(enumerator, &imc))
        {
                if (id == imc->get_id(imc))
                {
                        found = TRUE;
+                       this->id_mutex->lock(this->id_mutex);
                        *new_id = this->next_imc_id++;
+                       this->id_mutex->unlock(this->id_mutex);
                        imc->add_id(imc, *new_id);
                        DBG2(DBG_TNC, "additional ID %u reserved for IMC with primary ID %u",
                                                  *new_id, id);
@@ -392,6 +404,7 @@ METHOD(imc_manager_t, destroy, void,
        }
        this->imcs->destroy(this->imcs);
        this->lock->destroy(this->lock);
+       this->id_mutex->destroy(this->id_mutex);
        free(this);
 }
 
@@ -421,6 +434,7 @@ imc_manager_t* tnc_imc_manager_create(void)
                },
                .imcs = linked_list_create(),
                .lock = rwlock_create(RWLOCK_TYPE_DEFAULT),
+               .id_mutex = mutex_create(MUTEX_TYPE_DEFAULT),
                .next_imc_id = 1,
        );
 
index 49f5149..b950e31 100644 (file)
@@ -31,6 +31,7 @@
 #include <daemon.h>
 #include <utils/debug.h>
 #include <threading/rwlock.h>
+#include <threading/mutex.h>
 #include <collections/linked_list.h>
 
 typedef struct private_tnc_imv_manager_t private_tnc_imv_manager_t;
@@ -61,6 +62,11 @@ struct private_tnc_imv_manager_t {
        TNC_IMVID next_imv_id;
 
        /**
+        * Mutex to access next IMV ID
+        */
+       mutex_t *id_mutex;
+
+       /**
         * Policy defining how to derive final recommendation from individual ones
         */
        recommendation_policy_t policy;
@@ -70,9 +76,14 @@ METHOD(imv_manager_t, add, bool,
        private_tnc_imv_manager_t *this, imv_t *imv)
 {
        TNC_Version version;
+       TNC_IMVID imv_id;
 
-       imv->set_id(imv, this->next_imv_id);
-       if (imv->initialize(imv->get_id(imv), TNC_IFIMV_VERSION_1,
+       this->id_mutex->lock(this->id_mutex);
+       imv_id = this->next_imv_id++;
+       this->id_mutex->unlock(this->id_mutex);
+
+       imv->set_id(imv, imv_id);
+       if (imv->initialize(imv_id, TNC_IFIMV_VERSION_1,
                                                TNC_IFIMV_VERSION_1, &version) != TNC_RESULT_SUCCESS)
        {
                DBG1(DBG_TNC, "IMV \"%s\" failed to initialize", imv->get_name(imv));
@@ -80,7 +91,6 @@ METHOD(imv_manager_t, add, bool,
        }
        this->lock->write_lock(this->lock);
        this->imvs->insert_last(this->imvs, imv);
-       this->next_imv_id++;
        this->lock->unlock(this->lock);
 
        if (imv->provide_bind_function(imv->get_id(imv),
@@ -203,14 +213,16 @@ METHOD(imv_manager_t, reserve_id, bool,
        imv_t *imv;
        bool found = FALSE;
 
-       this->lock->write_lock(this->lock);
+       this->lock->read_lock(this->lock);
        enumerator = this->imvs->create_enumerator(this->imvs);
        while (enumerator->enumerate(enumerator, &imv))
        {
                if (id == imv->get_id(imv))
                {
                        found = TRUE;
+                       this->id_mutex->lock(this->id_mutex);
                        *new_id = this->next_imv_id++;
+                       this->id_mutex->unlock(this->id_mutex);
                        imv->add_id(imv, *new_id);
                        DBG2(DBG_TNC, "additional ID %u reserved for IMV with primary ID %u",
                                                  *new_id, id);
@@ -469,6 +481,7 @@ METHOD(imv_manager_t, destroy, void,
        }
        this->imvs->destroy(this->imvs);
        this->lock->destroy(this->lock);
+       this->id_mutex->destroy(this->id_mutex);
        free(this);
 }
 
@@ -501,6 +514,7 @@ imv_manager_t* tnc_imv_manager_create(void)
                },
                .imvs = linked_list_create(),
                .lock = rwlock_create(RWLOCK_TYPE_DEFAULT),
+               .id_mutex = mutex_create(MUTEX_TYPE_DEFAULT),
                .next_imv_id = 1,
        );