From: Alberto Leiva Popper Date: Thu, 2 May 2019 23:14:20 +0000 (-0500) Subject: Patch several memory leaks X-Git-Tag: v0.0.2~41^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=880261e97f1f0a3bec8e06b89333efe3d9975af2;p=thirdparty%2FFORT-validator.git Patch several memory leaks --- diff --git a/src/notify.c b/src/notify.c index a57da60c..40e7485a 100644 --- a/src/notify.c +++ b/src/notify.c @@ -1,7 +1,9 @@ #include "notify.h" #include +#include #include "clients.h" +#include "log.h" #include "rtr/pdu_sender.h" #include "rtr/db/vrps.h" diff --git a/src/rtr/db/roa_table.c b/src/rtr/db/roa_table.c index b166b20d..52a0b42d 100644 --- a/src/rtr/db/roa_table.c +++ b/src/rtr/db/roa_table.c @@ -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 diff --git a/src/rtr/db/roa_table.h b/src/rtr/db/roa_table.h index f9a3122d..8bd0da2d 100644 --- a/src/rtr/db/roa_table.h +++ b/src/rtr/db/roa_table.h @@ -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 *); diff --git a/src/rtr/db/vrp.h b/src/rtr/db/vrp.h index 9a520eca..3ffc0403 100644 --- a/src/rtr/db/vrp.h +++ b/src/rtr/db/vrp.h @@ -1,6 +1,9 @@ #ifndef SRC_RTR_DB_VRP_H_ #define SRC_RTR_DB_VRP_H_ +#include +#include + #define FLAG_WITHDRAWAL 0 #define FLAG_ANNOUNCEMENT 1 diff --git a/src/rtr/db/vrps.c b/src/rtr/db/vrps.c index 1b262684..d6160ab7 100644 --- a/src/rtr/db/vrps.c +++ b/src/rtr/db/vrps.c @@ -4,7 +4,10 @@ #include #include #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); diff --git a/src/rtr/db/vrps.h b/src/rtr/db/vrps.h index 77fe1223..173cd37f 100644 --- a/src/rtr/db/vrps.h +++ b/src/rtr/db/vrps.h @@ -1,11 +1,8 @@ #ifndef SRC_VRPS_H_ #define SRC_VRPS_H_ -#include -#include - -#include "rtr/db/delta.h" -#include "rtr/db/roa_table.h" +#include +#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 *); diff --git a/src/rtr/pdu_handler.c b/src/rtr/pdu_handler.c index 6053a1a2..c7fd219e 100644 --- a/src/rtr/pdu_handler.c +++ b/src/rtr/pdu_handler.c @@ -5,6 +5,7 @@ #include #include "err_pdu.h" +#include "log.h" #include "pdu.h" #include "pdu_sender.h" #include "rtr/db/vrps.h" diff --git a/src/rtr/rtr.c b/src/rtr/rtr.c index 4662da9a..0f4b44eb 100644 --- a/src/rtr/rtr.c +++ b/src/rtr/rtr.c @@ -25,8 +25,18 @@ /* 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(¶m, 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, ¶m.client_addr, + err = clients_add(param->client_fd, ¶m->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; diff --git a/src/updates_daemon.c b/src/updates_daemon.c index 2dea6b40..4572d90e 100644 --- a/src/updates_daemon.c +++ b/src/updates_daemon.c @@ -6,106 +6,36 @@ #include #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); }