From a8ec5017e0a4452f520c90f68dc049d28a922690 Mon Sep 17 00:00:00 2001 From: dimitri staessens Date: Sun, 24 Sep 2017 21:43:05 +0200 Subject: ipcpd: Fix deadlock in DHT The lookup_update was waiting for the LU_INIT state to resolve under dht->lock which prevented that update. This PR fixes this bug, but leaves a (very rare) bug when the lookup_destroy is called while the lookup_update is waiting for the LU_INIT state to resolve. The solution also is a (harmless) lock inversion, but this is also not the best. Fixes #51 Fixes #52 --- src/ipcpd/normal/dht.c | 68 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 27 deletions(-) diff --git a/src/ipcpd/normal/dht.c b/src/ipcpd/normal/dht.c index 93fd4e4e..4d0cdb02 100644 --- a/src/ipcpd/normal/dht.c +++ b/src/ipcpd/normal/dht.c @@ -698,7 +698,7 @@ static void lookup_destroy(struct lookup * lu) return; case LU_PENDING: lu->state = LU_DESTROY; - pthread_cond_signal(&lu->cond); + pthread_cond_broadcast(&lu->cond); break; case LU_INIT: case LU_DONE: @@ -727,7 +727,6 @@ static void lookup_destroy(struct lookup * lu) pthread_mutex_unlock(&lu->lock); - pthread_cond_destroy(&lu->cond); pthread_mutex_destroy(&lu->lock); free(lu); @@ -761,13 +760,22 @@ static void lookup_update(struct dht * dht, lu->n_addrs = msg->n_addrs; } lu->state = LU_COMPLETE; - pthread_cond_signal(&lu->cond); + pthread_cond_broadcast(&lu->cond); pthread_mutex_unlock(&lu->lock); return; } - while (lu->state == LU_INIT) + while (lu->state == LU_INIT) { + pthread_rwlock_unlock(&dht->lock); pthread_cond_wait(&lu->cond, &lu->lock); + pthread_rwlock_rdlock(&dht->lock); + } + + /* BUG: this should not be allowed since it's use-after-free. */ + if (lu->state == LU_DESTROY || lu->state == LU_NULL) { + log_warn("Use-after-free. Update aborted to avoid worse."); + return; + } for (n = 0; n < msg->n_contacts; ++n) { c = contact_create(msg->contacts[n]->id.data, @@ -775,6 +783,8 @@ static void lookup_update(struct dht * dht, if (c == NULL) continue; + pos = 0; + list_for_each(p, &lu->contacts) { struct contact * e; e = list_entry(p, struct contact, next); @@ -786,6 +796,7 @@ static void lookup_update(struct dht * dht, if (dist(c->id, lu->key) > dist(e->id, lu->key)) break; + pos++; } @@ -798,7 +809,6 @@ static void lookup_update(struct dht * dht, mod = true; } else if (pos == dht->k) { contact_destroy(c); - continue; } else { struct contact * d; list_add_tail(&c->next, p); @@ -815,7 +825,7 @@ static void lookup_update(struct dht * dht, else lu->state = LU_UPDATE; - pthread_cond_signal(&lu->cond); + pthread_cond_broadcast(&lu->cond); pthread_mutex_unlock(&lu->lock); return; } @@ -862,7 +872,7 @@ static ssize_t lookup_contact_addrs(struct lookup * lu, } static void lookup_new_addrs(struct lookup * lu, - uint64_t * addrs) + uint64_t * addrs) { struct list_head * p; size_t n = 0; @@ -889,8 +899,10 @@ static void lookup_new_addrs(struct lookup * lu, addrs[n] = 0; - if (n == 0) + if (n == 0) { lu->state = LU_DONE; + pthread_cond_signal(&lu->cond); + } pthread_mutex_unlock(&lu->lock); } @@ -901,7 +913,7 @@ static void lookup_set_state(struct lookup * lu, pthread_mutex_lock(&lu->lock); lu->state = state; - pthread_cond_signal(&lu->cond); + pthread_cond_broadcast(&lu->cond); pthread_mutex_unlock(&lu->lock); } @@ -927,8 +939,10 @@ static enum lookup_state lookup_wait(struct lookup * lu) pthread_mutex_lock(&lu->lock); - lu->state = LU_PENDING; - pthread_cond_signal(&lu->cond); + if (lu->state == LU_INIT) { + lu->state = LU_PENDING; + pthread_cond_signal(&lu->cond); + } pthread_cleanup_push(cleanup_wait, lu); @@ -1582,6 +1596,16 @@ static ssize_t kad_find(struct dht * dht, return sent; } +static void lookup_detach(struct dht * dht, + struct lookup * lu) +{ + pthread_rwlock_wrlock(&dht->lock); + + list_del(&lu->next); + + pthread_rwlock_unlock(&dht->lock); +} + static struct lookup * kad_lookup(struct dht * dht, const uint8_t * id, enum kad_code code) @@ -1598,18 +1622,14 @@ static struct lookup * kad_lookup(struct dht * dht, lookup_new_addrs(lu, addrs); if (addrs[0] == 0) { - pthread_rwlock_wrlock(&dht->lock); - list_del(&lu->next); - pthread_rwlock_unlock(&dht->lock); + lookup_detach(dht, lu); lookup_destroy(lu); return NULL; } out = kad_find(dht, id, addrs, code); if (out == 0) { - pthread_rwlock_wrlock(&dht->lock); - list_del(&lu->next); - pthread_rwlock_unlock(&dht->lock); + lookup_detach(dht, lu); return lu; } @@ -1620,9 +1640,7 @@ static struct lookup * kad_lookup(struct dht * dht, case LU_UPDATE: lookup_new_addrs(lu, addrs); if (addrs[0] == 0) { - pthread_rwlock_wrlock(&dht->lock); - list_del(&lu->next); - pthread_rwlock_unlock(&dht->lock); + lookup_detach(dht, lu); lookup_set_state(lu, LU_COMPLETE); return lu; } @@ -1631,21 +1649,17 @@ static struct lookup * kad_lookup(struct dht * dht, lookup_add_out(lu, out); break; case LU_DESTROY: - pthread_rwlock_wrlock(&dht->lock); - list_del(&lu->next); - pthread_rwlock_unlock(&dht->lock); + lookup_detach(dht, lu); lookup_set_state(lu, LU_NULL); return NULL; default: break; - }; + } } assert(state = LU_COMPLETE); - pthread_rwlock_wrlock(&dht->lock); - list_del(&lu->next); - pthread_rwlock_unlock(&dht->lock); + lookup_detach(dht, lu); return lu; } -- cgit v1.2.3