summaryrefslogtreecommitdiff
path: root/src/irmd
diff options
context:
space:
mode:
authorDimitri Staessens <dimitri@ouroboros.rocks>2020-11-23 19:52:28 +0100
committerSander Vrijders <sander@ouroboros.rocks>2020-11-25 15:35:23 +0100
commit4c01338e4fb8aee6b28603e7e5f7459f59db9561 (patch)
treec7993a4490975888027b7273f511b5c4f59c241d /src/irmd
parent2796b90230c7a7642678dea2147ed19b43f9835d (diff)
downloadouroboros-4c01338e4fb8aee6b28603e7e5f7459f59db9561.tar.gz
ouroboros-4c01338e4fb8aee6b28603e7e5f7459f59db9561.zip
irmd: Fix data race in flow allocation
The flow information in the main loop is passed as a direct pointer to an irm_flow object in the flow database. This was (probably) not really an issue due to how the flow allocation operations work, but the thread sanitizer was barfing a lot of (correct) data race errors when running bigger tests, so now makes a safe copy of the data. Signed-off-by: Dimitri Staessens <dimitri@ouroboros.rocks> Signed-off-by: Sander Vrijders <sander@ouroboros.rocks>
Diffstat (limited to 'src/irmd')
-rw-r--r--src/irmd/main.c173
1 files changed, 100 insertions, 73 deletions
diff --git a/src/irmd/main.c b/src/irmd/main.c
index 3a0ad544..647468d7 100644
--- a/src/irmd/main.c
+++ b/src/irmd/main.c
@@ -1256,35 +1256,35 @@ static int proc_announce(pid_t pid,
return 0;
}
-static int flow_accept(pid_t pid,
- struct timespec * timeo,
- struct irm_flow ** fl,
- const void * data,
- size_t len)
+static int flow_accept(pid_t pid,
+ struct timespec * timeo,
+ struct irm_flow * f_out,
+ const void * data,
+ size_t len)
{
struct irm_flow * f = NULL;
- struct proc_entry * e = NULL;
+ struct proc_entry * pe = NULL;
struct reg_entry * re = NULL;
struct list_head * p = NULL;
- pid_t pid_n1;
pid_t pid_n;
+ pid_t pid_n_1;
int flow_id;
int ret;
pthread_rwlock_wrlock(&irmd.reg_lock);
- e = proc_table_get(&irmd.proc_table, pid);
- if (e == NULL) {
+ pe = proc_table_get(&irmd.proc_table, pid);
+ if (pe == NULL) {
pthread_rwlock_unlock(&irmd.reg_lock);
log_err("Unknown process %d calling accept.", pid);
return -EINVAL;
}
- log_dbg("New instance (%d) of %s added.", pid, e->prog);
+ log_dbg("New instance (%d) of %s added.", pid, pe->prog);
log_dbg("This process accepts flows for:");
- list_for_each(p, &e->names) {
+ list_for_each(p, &pe->names) {
struct str_el * s = list_entry(p, struct str_el, next);
log_dbg(" %s", s->str);
re = registry_get_entry(&irmd.registry, s->str);
@@ -1294,7 +1294,7 @@ static int flow_accept(pid_t pid,
pthread_rwlock_unlock(&irmd.reg_lock);
- ret = proc_entry_sleep(e, timeo);
+ ret = proc_entry_sleep(pe, timeo);
if (ret == -ETIMEDOUT)
return -ETIMEDOUT;
@@ -1314,20 +1314,20 @@ static int flow_accept(pid_t pid,
}
pid_n = f->n_pid;
- pid_n1 = f->n_1_pid;
+ pid_n_1 = f->n_1_pid;
flow_id = f->flow_id;
pthread_rwlock_unlock(&irmd.flows_lock);
pthread_rwlock_rdlock(&irmd.reg_lock);
- e = proc_table_get(&irmd.proc_table, pid);
- if (e == NULL) {
+ pe = proc_table_get(&irmd.proc_table, pid);
+ if (pe == NULL) {
pthread_rwlock_unlock(&irmd.reg_lock);
pthread_rwlock_wrlock(&irmd.flows_lock);
list_del(&f->next);
bmp_release(irmd.flow_ids, f->flow_id);
pthread_rwlock_unlock(&irmd.flows_lock);
- ipcp_flow_alloc_resp(pid_n1, flow_id, pid_n, -1, NULL, 0);
+ ipcp_flow_alloc_resp(pid_n_1, flow_id, pid_n, -1, NULL, 0);
clear_irm_flow(f);
irm_flow_set_state(f, FLOW_NULL);
irm_flow_destroy(f);
@@ -1335,11 +1335,11 @@ static int flow_accept(pid_t pid,
return -EPERM;
}
- pthread_mutex_lock(&e->lock);
+ pthread_mutex_lock(&pe->lock);
- re = e->re;
+ re = pe->re;
- pthread_mutex_unlock(&e->lock);
+ pthread_mutex_unlock(&pe->lock);
if (reg_entry_get_state(re) != REG_NAME_FLOW_ARRIVED) {
pthread_rwlock_unlock(&irmd.reg_lock);
@@ -1347,7 +1347,7 @@ static int flow_accept(pid_t pid,
list_del(&f->next);
bmp_release(irmd.flow_ids, f->flow_id);
pthread_rwlock_unlock(&irmd.flows_lock);
- ipcp_flow_alloc_resp(pid_n1, flow_id, pid_n, -1, NULL, 0);
+ ipcp_flow_alloc_resp(pid_n_1, flow_id, pid_n, -1, NULL, 0);
clear_irm_flow(f);
irm_flow_set_state(f, FLOW_NULL);
irm_flow_destroy(f);
@@ -1359,12 +1359,25 @@ static int flow_accept(pid_t pid,
pthread_rwlock_unlock(&irmd.reg_lock);
+ pthread_rwlock_wrlock(&irmd.flows_lock);
+
+ f_out->flow_id = f->flow_id;
+ f_out->n_pid = f->n_pid;
+ f_out->n_1_pid = f->n_1_pid;
+ f_out->data = f->data; /* pass owner */
+ f_out->len = f->len;
+ f_out->qs = f->qs;
+ f->data = NULL;
+ f->len = 0;
+
+ pthread_rwlock_unlock(&irmd.flows_lock);
+
if (f->qs.cypher_s == 0) { /* no crypto requested, don't send pubkey */
data = NULL;
len = 0;
}
- if (ipcp_flow_alloc_resp(pid_n1, flow_id, pid_n, 0, data, len)) {
+ if (ipcp_flow_alloc_resp(pid_n_1, flow_id, pid_n, 0, data, len)) {
pthread_rwlock_wrlock(&irmd.flows_lock);
list_del(&f->next);
pthread_rwlock_unlock(&irmd.flows_lock);
@@ -1379,8 +1392,6 @@ static int flow_accept(pid_t pid,
log_info("Flow on flow_id %d allocated.", f->flow_id);
- *fl = f;
-
return 0;
}
@@ -1388,7 +1399,7 @@ static int flow_alloc(pid_t pid,
const char * dst,
qosspec_t qs,
struct timespec * timeo,
- struct irm_flow ** e,
+ struct irm_flow * f_out,
bool join,
const void * data,
size_t len)
@@ -1407,6 +1418,7 @@ static int flow_alloc(pid_t pid,
}
pthread_rwlock_wrlock(&irmd.flows_lock);
+
flow_id = bmp_allocate(irmd.flow_ids);
if (!bmp_is_id_valid(irmd.flow_ids, flow_id)) {
pthread_rwlock_unlock(&irmd.flows_lock);
@@ -1466,9 +1478,19 @@ static int flow_alloc(pid_t pid,
return -EPIPE;
}
+ pthread_rwlock_wrlock(&irmd.flows_lock);
+
assert(irm_flow_get_state(f) == FLOW_ALLOCATED);
- *e = f;
+ f_out->flow_id = f->flow_id;
+ f_out->n_pid = f->n_pid;
+ f_out->n_1_pid = f->n_1_pid;
+ f_out->data = f->data; /* pass owner */
+ f_out->len = f->len;
+ f->data = NULL;
+ f->len = 0;
+
+ pthread_rwlock_unlock(&irmd.flows_lock);
log_info("Flow on flow_id %d allocated.", flow_id);
@@ -1553,21 +1575,22 @@ static pid_t auto_execute(char ** argv)
return pid;
}
-static struct irm_flow * flow_req_arr(pid_t pid,
- const uint8_t * hash,
- qosspec_t qs,
- const void * data,
- size_t len)
+static int flow_req_arr(pid_t pid,
+ struct irm_flow * f_out,
+ const uint8_t * hash,
+ qosspec_t qs,
+ const void * data,
+ size_t len)
{
struct reg_entry * re = NULL;
struct prog_entry * a = NULL;
- struct proc_entry * e = NULL;
+ struct proc_entry * pe = NULL;
struct irm_flow * f = NULL;
struct pid_el * c_pid;
struct ipcp_entry * ipcp;
- pid_t h_pid = -1;
- int flow_id = -1;
+ pid_t h_pid;
+ int flow_id;
struct timespec wt = {IRMD_REQ_ARR_TIMEOUT / 1000,
(IRMD_REQ_ARR_TIMEOUT % 1000) * MILLION};
@@ -1580,7 +1603,7 @@ static struct irm_flow * flow_req_arr(pid_t pid,
ipcp = get_ipcp_entry_by_pid(pid);
if (ipcp == NULL) {
log_err("IPCP died.");
- return NULL;
+ return -EIPCP;
}
re = registry_get_entry_by_hash(&irmd.registry, ipcp->dir_hash_algo,
@@ -1588,7 +1611,7 @@ static struct irm_flow * flow_req_arr(pid_t pid,
if (re == NULL) {
pthread_rwlock_unlock(&irmd.reg_lock);
log_err("Unknown hash: " HASH_FMT ".", HASH_VAL(hash));
- return NULL;
+ return -1;
}
log_info("Flow request arrived for %s.", re->name);
@@ -1598,7 +1621,7 @@ static struct irm_flow * flow_req_arr(pid_t pid,
/* Give the process a bit of slop time to call accept */
if (reg_entry_leave_state(re, REG_NAME_IDLE, &wt) == -1) {
log_err("No processes for " HASH_FMT ".", HASH_VAL(hash));
- return NULL;
+ return -1;
}
pthread_rwlock_wrlock(&irmd.reg_lock);
@@ -1607,12 +1630,12 @@ static struct irm_flow * flow_req_arr(pid_t pid,
case REG_NAME_IDLE:
pthread_rwlock_unlock(&irmd.reg_lock);
log_err("No processes for " HASH_FMT ".", HASH_VAL(hash));
- return NULL;
+ return -1;
case REG_NAME_AUTO_ACCEPT:
c_pid = malloc(sizeof(*c_pid));
if (c_pid == NULL) {
pthread_rwlock_unlock(&irmd.reg_lock);
- return NULL;
+ return -1;
}
reg_entry_set_state(re, REG_NAME_AUTO_EXEC);
@@ -1625,7 +1648,7 @@ static struct irm_flow * flow_req_arr(pid_t pid,
log_err("Could not start program for reg_entry %s.",
re->name);
free(c_pid);
- return NULL;
+ return -1;
}
list_add(&c_pid->next, &irmd.spawned_pids);
@@ -1633,7 +1656,7 @@ static struct irm_flow * flow_req_arr(pid_t pid,
pthread_rwlock_unlock(&irmd.reg_lock);
if (reg_entry_leave_state(re, REG_NAME_AUTO_EXEC, NULL))
- return NULL;
+ return -1;
pthread_rwlock_wrlock(&irmd.reg_lock);
/* FALLTHRU */
@@ -1642,14 +1665,14 @@ static struct irm_flow * flow_req_arr(pid_t pid,
if (h_pid == -1) {
pthread_rwlock_unlock(&irmd.reg_lock);
log_err("Invalid process id returned.");
- return NULL;
+ return -1;
}
break;
default:
pthread_rwlock_unlock(&irmd.reg_lock);
log_err("IRMd in wrong state.");
- return NULL;
+ return -1;
}
pthread_rwlock_unlock(&irmd.reg_lock);
@@ -1658,7 +1681,7 @@ static struct irm_flow * flow_req_arr(pid_t pid,
flow_id = bmp_allocate(irmd.flow_ids);
if (!bmp_is_id_valid(irmd.flow_ids, flow_id)) {
pthread_rwlock_unlock(&irmd.flows_lock);
- return NULL;
+ return -1;
}
f = irm_flow_create(h_pid, pid, flow_id, qs);
@@ -1666,7 +1689,7 @@ static struct irm_flow * flow_req_arr(pid_t pid,
bmp_release(irmd.flow_ids, flow_id);
pthread_rwlock_unlock(&irmd.flows_lock);
log_err("Could not allocate flow_id.");
- return NULL;
+ return -1;
}
if (len != 0) {
@@ -1676,7 +1699,7 @@ static struct irm_flow * flow_req_arr(pid_t pid,
bmp_release(irmd.flow_ids, flow_id);
pthread_rwlock_unlock(&irmd.flows_lock);
log_err("Could not piggyback data.");
- return NULL;
+ return -1;
}
f->len = len;
@@ -1691,8 +1714,8 @@ static struct irm_flow * flow_req_arr(pid_t pid,
reg_entry_set_state(re, REG_NAME_FLOW_ARRIVED);
- e = proc_table_get(&irmd.proc_table, h_pid);
- if (e == NULL) {
+ pe = proc_table_get(&irmd.proc_table, h_pid);
+ if (pe == NULL) {
pthread_rwlock_unlock(&irmd.reg_lock);
pthread_rwlock_wrlock(&irmd.flows_lock);
clear_irm_flow(f);
@@ -1703,16 +1726,19 @@ static struct irm_flow * flow_req_arr(pid_t pid,
free(f->data);
f->len = 0;
irm_flow_destroy(f);
- return NULL;
+ return -1;
}
- proc_entry_wake(e, re);
+ proc_entry_wake(pe, re);
pthread_rwlock_unlock(&irmd.reg_lock);
reg_entry_leave_state(re, REG_NAME_FLOW_ARRIVED, NULL);
- return f;
+ f_out->flow_id = flow_id;
+ f_out->n_pid = h_pid;
+
+ return 0;
}
static int flow_alloc_reply(int flow_id,
@@ -1722,7 +1748,7 @@ static int flow_alloc_reply(int flow_id,
{
struct irm_flow * f;
- pthread_rwlock_rdlock(&irmd.flows_lock);
+ pthread_rwlock_wrlock(&irmd.flows_lock);
f = get_irm_flow(flow_id);
if (f == NULL) {
@@ -1740,6 +1766,7 @@ static int flow_alloc_reply(int flow_id,
pthread_rwlock_unlock(&irmd.flows_lock);
return -1;
}
+
memcpy(f->data, data, len);
f->len = len;
@@ -2027,12 +2054,14 @@ static void * mainloop(void * o)
while (true) {
irm_msg_t * ret_msg;
- struct irm_flow * e = NULL;
+ struct irm_flow e;
struct timespec * timeo = NULL;
struct timespec ts = {0, 0};
struct cmd * cmd;
int result;
+ memset(&e, 0, sizeof(e));
+
ret_msg = malloc(sizeof(*ret_msg));
if (ret_msg == NULL)
return (void *) -1;
@@ -2150,15 +2179,14 @@ static void * mainloop(void * o)
if (result == 0) {
qosspec_msg_t qs_msg;
ret_msg->has_flow_id = true;
- ret_msg->flow_id = e->flow_id;
+ ret_msg->flow_id = e.flow_id;
ret_msg->has_pid = true;
- ret_msg->pid = e->n_1_pid;
- qs_msg = spec_to_msg(&e->qs);
+ ret_msg->pid = e.n_1_pid;
+ qs_msg = spec_to_msg(&e.qs);
ret_msg->qosspec = &qs_msg;
ret_msg->has_pk = true;
- ret_msg->pk.data = e->data;
- ret_msg->pk.len = e->len;
- e->len = 0; /* Data is free'd with ret_msg */
+ ret_msg->pk.data = e.data;
+ ret_msg->pk.len = e.len;
}
break;
case IRM_MSG_CODE__IRM_FLOW_ALLOC:
@@ -2170,13 +2198,12 @@ static void * mainloop(void * o)
msg->pk.len);
if (result == 0) {
ret_msg->has_flow_id = true;
- ret_msg->flow_id = e->flow_id;
+ ret_msg->flow_id = e.flow_id;
ret_msg->has_pid = true;
- ret_msg->pid = e->n_1_pid;
+ ret_msg->pid = e.n_1_pid;
ret_msg->has_pk = true;
- ret_msg->pk.data = e->data;
- ret_msg->pk.len = e->len;
- e->len = 0; /* Data is free'd with ret_msg */
+ ret_msg->pk.data = e.data;
+ ret_msg->pk.len = e.len;
}
break;
case IRM_MSG_CODE__IRM_FLOW_JOIN:
@@ -2186,9 +2213,9 @@ static void * mainloop(void * o)
timeo, &e, true, NULL, 0);
if (result == 0) {
ret_msg->has_flow_id = true;
- ret_msg->flow_id = e->flow_id;
+ ret_msg->flow_id = e.flow_id;
ret_msg->has_pid = true;
- ret_msg->pid = e->n_1_pid;
+ ret_msg->pid = e.n_1_pid;
}
break;
case IRM_MSG_CODE__IRM_FLOW_DEALLOC:
@@ -2199,17 +2226,17 @@ static void * mainloop(void * o)
case IRM_MSG_CODE__IPCP_FLOW_REQ_ARR:
assert(msg->pk.len > 0 ? msg->pk.data != NULL
: msg->pk.data == NULL);
- e = flow_req_arr(msg->pid,
- msg->hash.data,
- msg_to_spec(msg->qosspec),
- msg->pk.data,
- msg->pk.len);
- result = (e == NULL ? -1 : 0);
+ result = flow_req_arr(msg->pid,
+ &e,
+ msg->hash.data,
+ msg_to_spec(msg->qosspec),
+ msg->pk.data,
+ msg->pk.len);
if (result == 0) {
ret_msg->has_flow_id = true;
- ret_msg->flow_id = e->flow_id;
+ ret_msg->flow_id = e.flow_id;
ret_msg->has_pid = true;
- ret_msg->pid = e->n_pid;
+ ret_msg->pid = e.n_pid;
}
break;
case IRM_MSG_CODE__IPCP_FLOW_ALLOC_REPLY: