ipcpd, normal: Safe handling of CDAP requests

Destruction of the object in the reply stage was unsafe.

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   <[email protected]>
+ *    Dimitri Staessens <[email protected]>
+ *
+ * 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   <[email protected]>
+ *    Dimitri Staessens <[email protected]>
+ *
+ * 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);