summaryrefslogtreecommitdiff
path: root/src/ipcpd
diff options
context:
space:
mode:
authordimitri staessens <dimitri.staessens@ugent.be>2017-09-24 21:43:05 +0200
committerdimitri staessens <dimitri.staessens@ugent.be>2017-09-24 22:00:20 +0200
commita8ec5017e0a4452f520c90f68dc049d28a922690 (patch)
treef15df5de41e270829f3cbca9e876d15e39fd1781 /src/ipcpd
parentff5063ad0e7902ce59864a466bd9d8d606d788e4 (diff)
downloadouroboros-a8ec5017e0a4452f520c90f68dc049d28a922690.tar.gz
ouroboros-a8ec5017e0a4452f520c90f68dc049d28a922690.zip
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
Diffstat (limited to 'src/ipcpd')
-rw-r--r--src/ipcpd/normal/dht.c68
1 files 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;
}