From: Tobias Brunner Date: Tue, 18 Dec 2012 14:50:08 +0000 (+0100) Subject: Fix deadlock in IMC/IMV managers X-Git-Tag: 5.0.2dr4~96 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0080daa78789bb7012bc0f80dd6b4ff0d47e41a3;p=thirdparty%2Fstrongswan.git Fix deadlock in IMC/IMV managers 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. --- diff --git a/src/libcharon/plugins/tnc_imc/tnc_imc_manager.c b/src/libcharon/plugins/tnc_imc/tnc_imc_manager.c index bdb20d5560..078f7bc344 100644 --- a/src/libcharon/plugins/tnc_imc/tnc_imc_manager.c +++ b/src/libcharon/plugins/tnc_imc/tnc_imc_manager.c @@ -22,6 +22,7 @@ #include #include #include +#include #include 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, ); diff --git a/src/libcharon/plugins/tnc_imv/tnc_imv_manager.c b/src/libcharon/plugins/tnc_imv/tnc_imv_manager.c index 49f51493a6..b950e3119b 100644 --- a/src/libcharon/plugins/tnc_imv/tnc_imv_manager.c +++ b/src/libcharon/plugins/tnc_imv/tnc_imv_manager.c @@ -31,6 +31,7 @@ #include #include #include +#include #include typedef struct private_tnc_imv_manager_t private_tnc_imv_manager_t; @@ -60,6 +61,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 */ @@ -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, );