From f7777922edd673e43fd670d3d28f052fde2ecc0a Mon Sep 17 00:00:00 2001 From: dimitri staessens Date: Thu, 25 Aug 2016 15:54:02 +0200 Subject: ipcpd, normal: Safe handling of CDAP requests Destruction of the object in the reply stage was unsafe. --- src/ipcpd/normal/CMakeLists.txt | 1 + src/ipcpd/normal/cdap_request.c | 156 ++++++++++++++++++++++++++++++++++++++++ src/ipcpd/normal/cdap_request.h | 77 ++++++++++++++++++++ src/ipcpd/normal/fmgr.c | 6 +- src/ipcpd/normal/main.c | 22 +++--- src/ipcpd/normal/ribmgr.c | 97 +++++++++---------------- 6 files changed, 282 insertions(+), 77 deletions(-) create mode 100644 src/ipcpd/normal/cdap_request.c create mode 100644 src/ipcpd/normal/cdap_request.h (limited to 'src') 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 + * Dimitri Staessens + * + * 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 +#include +#include + +#include "cdap_request.h" + +#include + +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 + * Dimitri Staessens + * + * 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 +#include +#include + +#include + +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); -- cgit v1.2.3