summaryrefslogtreecommitdiff
path: root/src/ipcpd/normal
diff options
context:
space:
mode:
authordimitri staessens <dimitri.staessens@intec.ugent.be>2016-08-25 15:54:02 +0200
committerdimitri staessens <dimitri.staessens@intec.ugent.be>2016-08-25 16:16:32 +0200
commitf7777922edd673e43fd670d3d28f052fde2ecc0a (patch)
tree32443ecbb20927b33a6becd50dc6720e4be5b722 /src/ipcpd/normal
parent6da1fa9716e508d5d15d2953a2c358cfd24c1570 (diff)
downloadouroboros-f7777922edd673e43fd670d3d28f052fde2ecc0a.tar.gz
ouroboros-f7777922edd673e43fd670d3d28f052fde2ecc0a.zip
ipcpd, normal: Safe handling of CDAP requests
Destruction of the object in the reply stage was unsafe.
Diffstat (limited to 'src/ipcpd/normal')
-rw-r--r--src/ipcpd/normal/CMakeLists.txt1
-rw-r--r--src/ipcpd/normal/cdap_request.c156
-rw-r--r--src/ipcpd/normal/cdap_request.h77
-rw-r--r--src/ipcpd/normal/fmgr.c6
-rw-r--r--src/ipcpd/normal/main.c22
-rw-r--r--src/ipcpd/normal/ribmgr.c97
6 files changed, 282 insertions, 77 deletions
diff --git a/src/ipcpd/normal/CMakeLists.txt b/src/ipcpd/normal/CMakeLists.txt
index 555260f1..68dca4b3 100644
--- a/src/ipcpd/normal/CMakeLists.txt
+++ b/src/ipcpd/normal/CMakeLists.txt
@@ -25,6 +25,7 @@ set(SOURCE_FILES
main.c
fmgr.c
frct.c
+ cdap_request.c
ribmgr.c
rmt.c
)
diff --git a/src/ipcpd/normal/cdap_request.c b/src/ipcpd/normal/cdap_request.c
new file mode 100644
index 00000000..cdbbb833
--- /dev/null
+++ b/src/ipcpd/normal/cdap_request.c
@@ -0,0 +1,156 @@
+/*
+ * Ouroboros - Copyright (C) 2016
+ *
+ * Normal IPCP - RIB Manager - CDAP request
+ *
+ * Sander Vrijders <sander.vrijders@intec.ugent.be>
+ * Dimitri Staessens <dimitri.staessens@intec.ugent.be>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <ouroboros/config.h>
+#include <ouroboros/time_utils.h>
+#include <ouroboros/errno.h>
+
+#include "cdap_request.h"
+
+#include <stdlib.h>
+
+struct cdap_request * cdap_request_create(enum cdap_opcode code,
+ char * name,
+ int invoke_id,
+ struct cdap * instance)
+{
+ struct cdap_request * creq = malloc(sizeof(*creq));
+ pthread_condattr_t cattr;
+
+ if (creq == NULL)
+ return NULL;
+
+ creq->code = code;
+ creq->name = name;
+ creq->invoke_id = invoke_id;
+ creq->instance = instance;
+ creq->state = REQ_INIT;
+ creq->result = -1;
+
+ pthread_condattr_init(&cattr);
+ pthread_condattr_setclock(&cattr, PTHREAD_COND_CLOCK);
+ pthread_cond_init(&creq->cond, &cattr);
+ pthread_mutex_init(&creq->lock, NULL);
+
+ INIT_LIST_HEAD(&creq->next);
+
+ return creq;
+}
+
+void cdap_request_destroy(struct cdap_request * creq)
+{
+ if (creq == NULL)
+ return;
+
+ pthread_mutex_lock(&creq->lock);
+
+ if (creq->state == REQ_DESTROY) {
+ pthread_mutex_unlock(&creq->lock);
+ return;
+ }
+
+ if (creq->state == REQ_INIT)
+ creq->state = REQ_DONE;
+
+ if (creq->state == REQ_PENDING) {
+ creq->state = REQ_DESTROY;
+ pthread_cond_broadcast(&creq->cond);
+ }
+
+ while (creq->state != REQ_DONE)
+ pthread_cond_wait(&creq->cond, &creq->lock);
+
+ pthread_mutex_unlock(&creq->lock);
+
+ pthread_cond_destroy(&creq->cond);
+ pthread_mutex_destroy(&creq->lock);
+
+ if (creq->name != NULL)
+ free(creq->name);
+
+ free(creq);
+}
+
+int cdap_request_wait(struct cdap_request * creq)
+{
+ struct timespec timeout = {(CDAP_REPLY_TIMEOUT / 1000),
+ (CDAP_REPLY_TIMEOUT % 1000) * MILLION};
+ struct timespec abstime;
+ int ret = -1;
+
+ if (creq == NULL)
+ return -EINVAL;
+
+ clock_gettime(CLOCK_REALTIME, &abstime);
+ ts_add(&abstime, &timeout, &abstime);
+
+ pthread_mutex_lock(&creq->lock);
+
+ if (creq->state != REQ_INIT) {
+ pthread_mutex_unlock(&creq->lock);
+ return -EINVAL;
+ }
+
+ creq->state = REQ_PENDING;
+
+ while (creq->state == REQ_PENDING) {
+ if ((ret = -pthread_cond_timedwait(&creq->cond,
+ &creq->lock,
+ &abstime)) == -ETIMEDOUT) {
+ break;
+ }
+ }
+
+ if (creq->state == REQ_DESTROY)
+ ret = -1;
+
+ creq->state = REQ_DONE;
+ pthread_cond_broadcast(&creq->cond);
+
+ pthread_mutex_unlock(&creq->lock);
+
+ return ret;
+}
+
+void cdap_request_respond(struct cdap_request * creq,
+ int response)
+{
+ if (creq == NULL)
+ return;
+
+ pthread_mutex_lock(&creq->lock);
+
+ if (creq->state != REQ_PENDING) {
+ pthread_mutex_unlock(&creq->lock);
+ return;
+ }
+
+ creq->state = REQ_RESPONSE;
+ creq->result = response;
+ pthread_cond_broadcast(&creq->cond);
+
+ while (creq->state == REQ_RESPONSE)
+ pthread_cond_wait(&creq->cond, &creq->lock);
+
+ pthread_mutex_unlock(&creq->lock);
+}
diff --git a/src/ipcpd/normal/cdap_request.h b/src/ipcpd/normal/cdap_request.h
new file mode 100644
index 00000000..b3be44f8
--- /dev/null
+++ b/src/ipcpd/normal/cdap_request.h
@@ -0,0 +1,77 @@
+/*
+ * Ouroboros - Copyright (C) 2016
+ *
+ * Normal IPCP - RIB Manager - CDAP request
+ *
+ * Sander Vrijders <sander.vrijders@intec.ugent.be>
+ * Dimitri Staessens <dimitri.staessens@intec.ugent.be>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#ifndef OUROBOROS_IPCPD_NORMAL_CDAP_REQUEST_H
+#define OUROBOROS_IPCPD_NORMAL_CDAP_REQUEST_H
+
+#include <ouroboros/config.h>
+#include <ouroboros/cdap.h>
+#include <ouroboros/list.h>
+
+#include <pthread.h>
+
+enum cdap_opcode {
+ READ = 0,
+ WRITE,
+ START,
+ STOP,
+ CREATE,
+ DELETE
+};
+
+enum creq_state {
+ REQ_INIT = 0,
+ REQ_PENDING,
+ REQ_RESPONSE,
+ REQ_DONE,
+ REQ_DESTROY
+};
+
+struct cdap_request {
+ struct list_head next;
+
+ enum cdap_opcode code;
+ char * name;
+ int invoke_id;
+ struct cdap * instance;
+
+ int result;
+
+ enum creq_state state;
+ pthread_cond_t cond;
+ pthread_mutex_t lock;
+};
+
+struct cdap_request * cdap_request_create(enum cdap_opcode code,
+ char * name,
+ int invoke_id,
+ struct cdap * instance);
+
+void cdap_request_destroy(struct cdap_request * creq);
+
+int cdap_request_wait(struct cdap_request * creq);
+
+void cdap_request_respond(struct cdap_request * creq,
+ int response);
+
+#endif /* OUROBOROS_IPCPD_NORMAL_CDAP_REQUEST_H */
diff --git a/src/ipcpd/normal/fmgr.c b/src/ipcpd/normal/fmgr.c
index 2e73d71f..3056b46d 100644
--- a/src/ipcpd/normal/fmgr.c
+++ b/src/ipcpd/normal/fmgr.c
@@ -182,8 +182,7 @@ int fmgr_fini()
pthread_cancel(fmgr->listen_thread);
- pthread_join(fmgr->listen_thread,
- NULL);
+ pthread_join(fmgr->listen_thread, NULL);
list_for_each(pos, &fmgr->n_1_flows) {
struct n_1_flow * e =
@@ -194,6 +193,9 @@ int fmgr_fini()
LOG_ERR("Failed to remove management flow.");
}
+ pthread_mutex_destroy(&fmgr->n_1_flows_lock);
+ pthread_mutex_destroy(&fmgr->n_flows_lock);
+
free(fmgr);
return 0;
diff --git a/src/ipcpd/normal/main.c b/src/ipcpd/normal/main.c
index a067fb83..f0fc0681 100644
--- a/src/ipcpd/normal/main.c
+++ b/src/ipcpd/normal/main.c
@@ -80,15 +80,6 @@ void ipcp_sig_handler(int sig, siginfo_t * info, void * c)
ipcp_set_state(_ipcp, IPCP_SHUTDOWN);
pthread_rwlock_unlock(&_ipcp->state_lock);
-
- if (fmgr_fini())
- LOG_ERR("Failed to finalize flow manager.");
-
- if (ribmgr_fini())
- LOG_ERR("Failed to finalize RIB manager.");
-
- if (frct_fini())
- LOG_ERR("Failed to finalize FRCT.");
}
default:
return;
@@ -247,9 +238,9 @@ void normal_ipcp_data_destroy()
if (normal_data(_ipcp)->rb != NULL)
shm_ap_rbuff_close(normal_data(_ipcp)->rb);
- pthread_rwlock_unlock(&_ipcp->state_lock);
-
ipcp_data_destroy(_ipcp->data);
+
+ pthread_rwlock_unlock(&_ipcp->state_lock);
}
int main(int argc, char * argv[])
@@ -339,6 +330,15 @@ int main(int argc, char * argv[])
pthread_join(normal_data(_ipcp)->mainloop, NULL);
+ if (fmgr_fini())
+ LOG_ERR("Failed to finalize flow manager.");
+
+ if (ribmgr_fini())
+ LOG_ERR("Failed to finalize RIB manager.");
+
+ if (frct_fini())
+ LOG_ERR("Failed to finalize FRCT.");
+
normal_ipcp_data_destroy();
free(_ipcp);
close_logfile();
diff --git a/src/ipcpd/normal/ribmgr.c b/src/ipcpd/normal/ribmgr.c
index 9b61e180..007361fd 100644
--- a/src/ipcpd/normal/ribmgr.c
+++ b/src/ipcpd/normal/ribmgr.c
@@ -37,6 +37,7 @@
#include "dt_const.h"
#include "frct.h"
#include "ipcp.h"
+#include "cdap_request.h"
#include "static_info.pb-c.h"
typedef StaticInfoMsg static_info_msg_t;
@@ -46,27 +47,6 @@ typedef StaticInfoMsg static_info_msg_t;
extern struct ipcp * _ipcp;
-enum cdap_opcode {
- READ = 0,
- WRITE,
- START,
- STOP,
- CREATE,
- DELETE
-};
-
-struct cdap_request {
- enum cdap_opcode code;
- char * name;
- int invoke_id;
- struct cdap * instance;
- int result;
- bool replied;
- pthread_cond_t cond;
-
- struct list_head next;
-};
-
struct mgmt_flow {
struct cdap * instance;
int fd;
@@ -86,53 +66,48 @@ struct rib {
} * rib = NULL;
/* Call while holding cdap_reqs_lock */
+/* FIXME: better not to call blocking functions under any lock */
int cdap_result_wait(struct cdap * instance,
enum cdap_opcode code,
char * name,
int invoke_id)
{
struct cdap_request * req;
- pthread_condattr_t cattr;
- struct timespec timeout = {(CDAP_REPLY_TIMEOUT / 1000),
- (CDAP_REPLY_TIMEOUT % 1000) * MILLION};
- struct timespec abstime;
-
- clock_gettime(PTHREAD_COND_CLOCK, &abstime);
- ts_add(&abstime, &timeout, &abstime);
+ int ret;
+ char * name_dup = strdup(name);
+ if (name_dup == NULL)
+ return -1;
- req = malloc(sizeof(*req));
- if (req == NULL)
+ req = cdap_request_create(code, name_dup, invoke_id, instance);
+ if (req == NULL) {
+ free(name_dup);
return -1;
+ }
- req->code = code;
- req->invoke_id = invoke_id;
- req->instance = instance;
- req->result = -1;
- req->replied = false;
+ list_add(&req->next, &rib->cdap_reqs);
- pthread_condattr_init(&cattr);
- pthread_condattr_setclock(&cattr, PTHREAD_COND_CLOCK);
- pthread_cond_init(&req->cond, &cattr);
+ pthread_mutex_unlock(&rib->cdap_reqs_lock);
- req->name = strdup(name);
- if (req->name == NULL) {
- free(req);
- return -1;
- }
+ ret = cdap_request_wait(req);
- INIT_LIST_HEAD(&req->next);
+ pthread_mutex_lock(&rib->cdap_reqs_lock);
- list_add(&req->next, &rib->cdap_reqs);
+ if (ret == -1) /* should only be on ipcp shutdown */
+ LOG_DBG("Waiting CDAP request destroyed.");
- while (req->replied == false)
- if (pthread_cond_timedwait(&req->cond,
- &rib->cdap_reqs_lock,
- &abstime) == ETIMEDOUT) {
- LOG_ERR("Didn't receive a CDAP reply in time.");
- return -1;
- }
+ if (ret == -ETIMEDOUT)
+ LOG_ERR("CDAP Request timed out.");
+
+ if (ret)
+ LOG_DBG("Unknown error code: %d.", ret);
- return req->result;
+ if (!ret)
+ ret = req->result;
+
+ list_del(&req->next);
+ cdap_request_destroy(req);
+
+ return ret;
}
int ribmgr_init()
@@ -200,11 +175,13 @@ int ribmgr_cdap_reply(struct cdap * instance,
struct list_head * pos, * n = NULL;
pthread_mutex_lock(&rib->cdap_reqs_lock);
+
list_for_each_safe(pos, n, &rib->cdap_reqs) {
struct cdap_request * req =
list_entry(pos, struct cdap_request, next);
if (req->instance == instance &&
- req->invoke_id == invoke_id) {
+ req->invoke_id == invoke_id &&
+ req->state == REQ_PENDING) {
if (result != 0)
LOG_ERR("CDAP command with code %d and name %s "
"failed with error %d",
@@ -214,20 +191,12 @@ int ribmgr_cdap_reply(struct cdap * instance,
"executed succesfully",
req->code, req->name);
- /* FIXME: In case of a read, update values here */
-
- req->replied = true;
- req->result = result;
- pthread_cond_broadcast(&req->cond);
pthread_mutex_unlock(&rib->cdap_reqs_lock);
- sched_yield();
+ /* FIXME: In case of a read, update values here */
+ cdap_request_respond(req, result);
pthread_mutex_lock(&rib->cdap_reqs_lock);
- free(req->name);
- list_del(&req->next);
- free(req);
- break;
}
}
pthread_mutex_unlock(&rib->cdap_reqs_lock);