]> git.ipfire.org Git - thirdparty/FORT-validator.git/commitdiff
Patch several memory leaks
authorAlberto Leiva Popper <ydahhrk@gmail.com>
Thu, 2 May 2019 23:14:20 +0000 (18:14 -0500)
committerAlberto Leiva Popper <ydahhrk@gmail.com>
Thu, 2 May 2019 23:14:20 +0000 (18:14 -0500)
src/notify.c
src/rtr/db/roa_table.c
src/rtr/db/roa_table.h
src/rtr/db/vrp.h
src/rtr/db/vrps.c
src/rtr/db/vrps.h
src/rtr/pdu_handler.c
src/rtr/rtr.c
src/updates_daemon.c

index a57da60c2f8c9af074baa1d4cbb302be15dbf6f6..40e7485abd3fd30be1e86ccda3e2f5edeba2b430 100644 (file)
@@ -1,7 +1,9 @@
 #include "notify.h"
 
 #include <err.h>
+#include <stddef.h>
 #include "clients.h"
+#include "log.h"
 #include "rtr/pdu_sender.h"
 #include "rtr/db/vrps.h"
 
index b166b20d8ac746bfeaa9d80d6f5674e7b58e8c5c..52a0b42d179a451d439b523bef654ba31af794e6 100644 (file)
@@ -14,7 +14,6 @@ struct hashable_roa {
 
 struct roa_table {
        struct hashable_roa *roas;
-       unsigned int references;
 };
 
 struct roa_table *
@@ -27,7 +26,6 @@ roa_table_create(void)
                return NULL;
 
        table->roas = NULL;
-       table->references = 1;
        return table;
 }
 
@@ -44,19 +42,10 @@ roa_table_cleanup(struct roa_table *table)
 }
 
 void
-roa_table_get(struct roa_table *table)
+roa_table_destroy(struct roa_table *table)
 {
-       table->references++;
-}
-
-void
-roa_table_put(struct roa_table *table)
-{
-       table->references--;
-       if (table->references == 0) {
-               roa_table_cleanup(table);
-               free(table);
-       }
+       roa_table_cleanup(table);
+       free(table);
 }
 
 int
index f9a3122de30e36c2cd731c4a894c13c32b2daca9..8bd0da2d9bb2422fbc2d0fbbff19075caca51ade 100644 (file)
@@ -6,11 +6,8 @@
 
 struct roa_table;
 
-/* Constructor */
 struct roa_table *roa_table_create(void);
-/* Reference counting */
-void roa_table_get(struct roa_table *);
-void roa_table_put(struct roa_table *);
+void roa_table_destroy(struct roa_table *);
 
 int roa_table_foreach_roa(struct roa_table *, vrp_foreach_cb, void *);
 
index 9a520eca266ae4d464d16cca93501c9d14173b32..3ffc040335c9d97216067e796a2b0d9829455af4 100644 (file)
@@ -1,6 +1,9 @@
 #ifndef SRC_RTR_DB_VRP_H_
 #define SRC_RTR_DB_VRP_H_
 
+#include <stdint.h>
+#include <netinet/in.h>
+
 #define FLAG_WITHDRAWAL                0
 #define FLAG_ANNOUNCEMENT      1
 
index 1b2626843cc8fef8fb9e890c6fa6474c21625af4..d6160ab762b8d14577f2cd0493d49481fbbf568b 100644 (file)
@@ -4,7 +4,10 @@
 #include <stdbool.h>
 #include <string.h>
 #include "common.h"
+#include "validation_handler.h"
 #include "data_structure/array_list.h"
+#include "object/tal.h"
+#include "rtr/db/roa_table.h"
 
 /*
  * Storage of VRPs (term taken from RFC 6811 "Validated ROA Payload") and
@@ -21,8 +24,16 @@ struct delta {
 ARRAY_LIST(deltas_db, struct delta)
 
 struct state {
-       struct roa_table *base; /** All the current valid ROAs */
-       struct deltas_db deltas; /** ROA changes to @base over time */
+       /**
+        * All the current valid ROAs.
+        *
+        * Can be NULL, so handle gracefully.
+        * (We use this to know we're supposed to generate a @deltas entry
+        * during the current iteration.)
+        */
+       struct roa_table *base;
+       /** ROA changes to @base over time. */
+       struct deltas_db deltas;
 
        uint32_t current_serial;
        uint16_t v0_session_id;
@@ -44,9 +55,7 @@ vrps_init(void)
 {
        int error;
 
-       state.base = roa_table_create();
-       if (state.base == NULL)
-               return pr_enomem();
+       state.base = NULL;
 
        deltas_db_init(&state.deltas);
 
@@ -67,7 +76,6 @@ vrps_init(void)
        error = pthread_rwlock_init(&lock, NULL);
        if (error) {
                deltas_db_cleanup(&state.deltas, delta_destroy);
-               roa_table_put(state.base);
                return pr_errno(error, "pthread_rwlock_init() errored");
        }
 
@@ -77,38 +85,120 @@ vrps_init(void)
 void
 vrps_destroy(void)
 {
-       roa_table_put(state.base);
+       if (state.base != NULL)
+               roa_table_destroy(state.base);
        deltas_db_cleanup(&state.deltas, delta_destroy);
        pthread_rwlock_destroy(&lock); /* Nothing to do with error code */
 }
 
-/*
- * @new_deltas can be NULL, @new_tree cannot.
- */
+static int
+__reset(void *arg)
+{
+       return rtrhandler_reset(arg);
+}
+
+int
+__handle_roa_v4(uint32_t as, struct ipv4_prefix const *prefix,
+    uint8_t max_length, void *arg)
+{
+       return rtrhandler_handle_roa_v4(arg, as, prefix, max_length);
+}
+
 int
-vrps_update(struct roa_table *new_roas, struct deltas *new_deltas)
+__handle_roa_v6(uint32_t as, struct ipv6_prefix const * prefix,
+    uint8_t max_length, void *arg)
 {
-       struct delta new_delta;
-       int error = 0;
+       return rtrhandler_handle_roa_v6(arg, as, prefix, max_length);
+}
+
+static int
+__perform_standalone_validation(struct roa_table **result)
+{
+       struct roa_table *roas;
+       struct validation_handler validation_handler;
+       int error;
+
+       roas = roa_table_create();
+       if (roas == NULL)
+               return pr_enomem();
+
+       validation_handler.reset = __reset;
+       validation_handler.traverse_down = NULL;
+       validation_handler.traverse_up = NULL;
+       validation_handler.handle_roa_v4 = __handle_roa_v4;
+       validation_handler.handle_roa_v6 = __handle_roa_v6;
+       validation_handler.arg = roas;
+
+       error = perform_standalone_validation(&validation_handler);
+       if (error) {
+               roa_table_destroy(roas);
+               return error;
+       }
+
+       *result = roas;
+       return 0;
+}
+
+int
+vrps_update(bool *changed)
+{
+       struct roa_table *old_base;
+       struct roa_table *new_base;
+       struct deltas *deltas; /* Deltas in raw form */
+       struct delta deltas_node; /* Deltas in database node form */
+       int error;
+
+       *changed = false;
+       old_base = NULL;
+       new_base = NULL;
+
+       error = __perform_standalone_validation(&new_base);
+       if (error)
+               return error;
 
        rwlock_write_lock(&lock);
 
-       if (new_deltas != NULL) {
-               new_delta.serial = state.current_serial + 1;
-               new_delta.deltas = new_deltas;
-               error = deltas_db_add(&state.deltas, &new_delta);
-               if (error)
-                       goto end;
+       if (state.base != NULL) {
+               error = compute_deltas(state.base, new_base, &deltas);
+               if (error) {
+                       rwlock_unlock(&lock);
+                       goto revert_base;
+               }
+
+               if (deltas_is_empty(deltas)) {
+                       rwlock_unlock(&lock);
+                       goto revert_deltas; /* error == 0 is good */
+               }
+
+               deltas_node.serial = state.current_serial + 1;
+               deltas_node.deltas = deltas;
+               error = deltas_db_add(&state.deltas, &deltas_node);
+               if (error) {
+                       rwlock_unlock(&lock);
+                       goto revert_deltas;
+               }
+
+               /*
+                * Postpone destruction of the old database,
+                * to release the lock ASAP.
+                */
+               old_base = state.base;
        }
 
-       if (state.base != NULL)
-               roa_table_put(state.base);
-       state.base = new_roas;
-       roa_table_get(new_roas);
+       *changed = true;
+       state.base = new_base;
        state.current_serial++;
 
-end:
        rwlock_unlock(&lock);
+
+       if (old_base != NULL)
+               roa_table_destroy(old_base);
+       return 0;
+
+revert_deltas:
+       deltas_destroy(deltas);
+revert_base:
+       roa_table_destroy(new_base);
        return error;
 }
 
@@ -182,7 +272,8 @@ vrps_foreach_base_roa(vrp_foreach_cb cb, void *arg)
        if (error)
                return error;
 
-       error = roa_table_foreach_roa(state.base, cb, arg);
+       if (state.base != NULL)
+               error = roa_table_foreach_roa(state.base, cb, arg);
 
        rwlock_unlock(&lock);
 
index 77fe1223d58c219e787181078e57a53a41307491..173cd37f0246217e43a9bcb16a52489181373242 100644 (file)
@@ -1,11 +1,8 @@
 #ifndef SRC_VRPS_H_
 #define SRC_VRPS_H_
 
-#include <time.h>
-#include <netinet/ip.h>
-
-#include "rtr/db/delta.h"
-#include "rtr/db/roa_table.h"
+#include <stdbool.h>
+#include "rtr/db/vrp.h"
 
 enum delta_status {
        /** There's no data at the DB */
@@ -21,7 +18,7 @@ enum delta_status {
 int vrps_init(void);
 void vrps_destroy(void);
 
-int vrps_update(struct roa_table *, struct deltas *);
+int vrps_update(bool *);
 int deltas_db_status(uint32_t *, enum delta_status *);
 
 int vrps_foreach_base_roa(vrp_foreach_cb, void *);
index 6053a1a288ecbc141c0cceb2e8f53b06844e871b..c7fd219ec8725772aee803fc09069b63dd96b44e 100644 (file)
@@ -5,6 +5,7 @@
 #include <unistd.h>
 
 #include "err_pdu.h"
+#include "log.h"
 #include "pdu.h"
 #include "pdu_sender.h"
 #include "rtr/db/vrps.h"
index 4662da9ac04ebc1e7a90fddfa1898d6502dd9c28..0f4b44eb7d192cdb8463f5d00417ac15abf90feb 100644 (file)
 /* TODO (urgent) this needs to be atomic */
 volatile bool loop;
 
+/*
+ * Arguments that the server socket thread will send to the client
+ * socket threads whenever it creates them.
+ */
+struct thread_param {
+       int client_fd;
+       struct sockaddr_storage client_addr;
+};
+
 struct thread_node {
        pthread_t tid;
+       struct thread_param params;
        SLIST_ENTRY(thread_node) next;
 };
 
@@ -105,15 +115,6 @@ create_server_socket(int *result)
        return pr_err("None of the addrinfo candidates could be bound.");
 }
 
-/*
- * Arguments that the server socket thread will send to the client socket
- * threads whenever it creates them.
- */
-struct thread_param {
-       int     client_fd;
-       struct sockaddr_storage client_addr;
-};
-
 enum verdict {
        /* No errors; continue happily. */
        VERDICT_SUCCESS,
@@ -170,52 +171,46 @@ end_client(int client_fd, const struct pdu_metadata *meta, void *pdu)
 
 /*
  * The client socket threads' entry routine.
- *
- * Please remember that this function needs to always release @param_void
- * before returning.
  */
 static void *
 client_thread_cb(void *param_void)
 {
-       struct thread_param param;
+       struct thread_param *param = param_void;
        struct pdu_metadata const *meta;
        void *pdu;
        int err;
        uint8_t rtr_version;
 
-       memcpy(&param, param_void, sizeof(param));
-       free(param_void);
-
        while (loop) { /* For each PDU... */
-               err = pdu_load(param.client_fd, &pdu, &meta, &rtr_version);
+               err = pdu_load(param->client_fd, &pdu, &meta, &rtr_version);
                if (err)
-                       return end_client(param.client_fd, NULL, NULL);
+                       return end_client(param->client_fd, NULL, NULL);
 
                /* Protocol Version Negotiation */
                if (rtr_version != RTR_VERSION_SUPPORTED) {
-                       err_pdu_send(param.client_fd, RTR_VERSION_SUPPORTED,
+                       err_pdu_send(param->client_fd, RTR_VERSION_SUPPORTED,
                            ERR_PDU_UNSUP_PROTO_VERSION,
                            (struct pdu_header *) pdu, NULL);
-                       return end_client(param.client_fd, meta, pdu);
+                       return end_client(param->client_fd, meta, pdu);
                }
                /* RTR Version ready, now update client */
-               err = clients_add(param.client_fd, &param.client_addr,
+               err = clients_add(param->client_fd, &param->client_addr,
                    rtr_version);
                if (err) {
                        if (err == -ERTR_VERSION_MISMATCH) {
-                               err_pdu_send(param.client_fd, rtr_version,
+                               err_pdu_send(param->client_fd, rtr_version,
                                    (rtr_version == RTR_V0
                                    ? ERR_PDU_UNSUP_PROTO_VERSION
                                    : ERR_PDU_UNEXPECTED_PROTO_VERSION),
                                    (struct pdu_header *) pdu, NULL);
                        }
-                       return end_client(param.client_fd, meta, pdu);
+                       return end_client(param->client_fd, meta, pdu);
                }
 
-               err = meta->handle(param.client_fd, pdu);
+               err = meta->handle(param->client_fd, pdu);
                meta->destructor(pdu);
                if (err)
-                       return end_client(param.client_fd, NULL, NULL);
+                       return end_client(param->client_fd, NULL, NULL);
        }
 
        return NULL; /* Unreachable. */
@@ -228,10 +223,8 @@ static int
 handle_client_connections(int server_fd)
 {
        struct sockaddr_storage client_addr;
-       struct thread_param *arg;
        struct thread_node *new_thread;
        socklen_t sizeof_client_addr;
-       pthread_attr_t attr;
        int client_fd;
 
        listen(server_fd, config_get_server_queue());
@@ -264,29 +257,13 @@ handle_client_connections(int server_fd)
                        continue;
                }
 
-               arg = malloc(sizeof(struct thread_param));
-               if (arg == NULL) {
-                       pr_err("Couldn't create thread_param struct");
-                       free(new_thread);
-                       close(client_fd);
-                       continue;
-               }
-               arg->client_fd = client_fd;
-               arg->client_addr = client_addr;
-
-               pthread_attr_init(&attr);
-               pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE);
-               errno = pthread_create(&new_thread->tid, &attr,
-                   client_thread_cb, arg);
-               pthread_attr_destroy(&attr);
+               new_thread->params.client_fd = client_fd;
+               new_thread->params.client_addr = client_addr;
+
+               errno = pthread_create(&new_thread->tid, NULL,
+                   client_thread_cb, &new_thread->params);
                if (errno) {
                        pr_errno(errno, "Could not spawn the client's thread");
-                       /*
-                        * It is not clear to me whether @arg should be freed
-                        * here. We're supposed to have transferred its
-                        * ownership to the thread.
-                        * Maybe we should store it in @new_thread instead.
-                        */
                        free(new_thread);
                        close(client_fd);
                        continue;
index 2dea6b4097572e8f317638206d9386d39ab259c2..4572d90ef1503bc059e89d678c5f8838fc3e50b5 100644 (file)
 #include <unistd.h>
 
 #include "config.h"
+#include "log.h" /* TODO delete me probably */
 #include "notify.h"
 #include "object/tal.h"
 #include "rtr/db/vrps.h"
 
 static pthread_t thread;
 
-static int
-__reset(void *arg)
-{
-       return rtrhandler_reset(arg);
-}
-
-int
-__handle_roa_v4(uint32_t as, struct ipv4_prefix const *prefix,
-    uint8_t max_length, void *arg)
-{
-       return rtrhandler_handle_roa_v4(arg, as, prefix, max_length);
-}
-
-int
-__handle_roa_v6(uint32_t as, struct ipv6_prefix const * prefix,
-    uint8_t max_length, void *arg)
-{
-       return rtrhandler_handle_roa_v6(arg, as, prefix, max_length);
-}
-
 static void *
 check_vrps_updates(void *param_void)
 {
-       struct validation_handler validation_handler;
-       struct roa_table *old_roas;
-       struct deltas *deltas;
+       bool changed;
        int error;
 
-       validation_handler.reset = __reset;
-       validation_handler.traverse_down = NULL;
-       validation_handler.traverse_up = NULL;
-       validation_handler.handle_roa_v4 = __handle_roa_v4;
-       validation_handler.handle_roa_v6 = __handle_roa_v6;
-       old_roas = NULL;
-
        do {
-               validation_handler.arg = roa_table_create();
-               if (validation_handler.arg == NULL) {
-                       pr_err("Memory allocation failed. Cannot validate. Sleeping...");
-                       goto sleep;
-               }
-
-               error = perform_standalone_validation(&validation_handler);
+               error = vrps_update(&changed);
                if (error) {
-                       roa_table_put(validation_handler.arg);
-                       pr_err("Validation failed (error code %d). Cannot udpate the ROA database. Sleeping...",
+                       pr_err("Error code %d while trying to update the ROA database. Sleeping...",
                            error);
                        goto sleep;
                }
 
-               if (old_roas == NULL) {
-                       error = vrps_update(validation_handler.arg, NULL);
-                       if (error) {
-                               roa_table_put(validation_handler.arg);
-                               pr_err("Error code %d while trying to update the ROA database. Sleeping...",
+               if (changed) {
+                       error = notify_clients();
+                       if (error)
+                               pr_debug("Could not notify clients of the new VRPs. (Error code %d.) Sleeping...",
                                    error);
-                       } else {
-                               old_roas = validation_handler.arg;
-                       }
-                       goto sleep;
+                       else
+                               pr_debug("Database updated successfully. Sleeping...");
                }
 
-               error = compute_deltas(old_roas, validation_handler.arg, &deltas);
-               if (error) {
-                       roa_table_put(validation_handler.arg);
-                       pr_err("Something went wrong while trying to compute the deltas. (error code %d.) Cannot update the ROA database. Sleeping...",
-                           error);
-                       goto sleep;
-               }
-
-               if (deltas_is_empty(deltas)) {
-                       roa_table_put(validation_handler.arg);
-                       deltas_destroy(deltas);
-                       pr_debug("No changes. Sleeping...");
-                       goto sleep;
-               }
-
-               error = vrps_update(validation_handler.arg, deltas);
-               if (error) {
-                       roa_table_put(validation_handler.arg);
-                       deltas_destroy(deltas);
-                       pr_err("Error code %d while trying to store the deltas in the database. Cannot update the ROA database. Sleeping...",
-                           error);
-                       goto sleep;
-               }
-
-               old_roas = validation_handler.arg;
-               error = notify_clients();
-               if (error)
-                       pr_debug("Could not notify clients of the new VRPs. (Error code %d.) Sleeping...",
-                           error);
-               else
-                       pr_debug("Database updated successfully. Sleeping...");
-
 sleep:
                sleep(config_get_validation_interval());
        } while (true);
@@ -116,12 +46,7 @@ sleep:
 int
 updates_daemon_start(void)
 {
-       pthread_attr_t attr;
-
-       pthread_attr_init(&attr);
-       pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE);
        errno = pthread_create(&thread, NULL, check_vrps_updates, NULL);
-       pthread_attr_destroy(&attr);
        if (errno)
                return -pr_errno(errno,
                    "Could not spawn the update daemon thread");
@@ -132,7 +57,6 @@ updates_daemon_start(void)
 void
 updates_daemon_destroy(void)
 {
-       void *ptr = NULL;
        pthread_cancel(thread);
-       pthread_join(thread, &ptr);
+       pthread_join(thread, NULL);
 }