From: Alberto Leiva Popper Date: Mon, 6 Jun 2022 22:16:12 +0000 (-0500) Subject: Refactors resulting from the issue #83 review X-Git-Tag: 1.5.4~20 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=0f579ac34c01fb46294ed4e2aa78d51de60ebabc;p=thirdparty%2FFORT-validator.git Refactors resulting from the issue #83 review Mostly quality of life improvements. On the other hand, it looks like the notfatal hash table API was being used incorrectly. HASH_ADD_KEYPTR can OOM, but `errno` wasn't being catched. Fixing this is nontrivial, however, because strange `reqs_error` functions are in the way, and that's a spaggetti I decided to avoid. Instead, I converted HASH_ADD_KEYPTR usage to the fatal hash table API. That's the future according to #40, anyway. I don't think this has anything to do with #83, though. --- diff --git a/src/data_structure/uthash_nonfatal.h b/src/data_structure/uthash_nonfatal.h index 76fbef6b..39206396 100644 --- a/src/data_structure/uthash_nonfatal.h +++ b/src/data_structure/uthash_nonfatal.h @@ -16,6 +16,26 @@ * this is the caller's responsibility. * * TODO I think most of the code is not checking this. + * + * Functions that can OOM: + * + * HASH_ADD_TO_TABLE + * HASH_ADD_KEYPTR_BYHASHVALUE_INORDER + * HASH_REPLACE_BYHASHVALUE_INORDER + * HASH_REPLACE_INORDER + * HASH_ADD_KEYPTR_INORDER + * HASH_ADD_INORDER + * HASH_ADD_BYHASHVALUE_INORDER + * HASH_ADD_KEYPTR_BYHASHVALUE + * HASH_REPLACE_BYHASHVALUE + * HASH_REPLACE (*) + * HASH_ADD_KEYPTR (**) + * HASH_ADD + * HASH_ADD_BYHASHVALUE + * HASH_SELECT + * + * (*) Used by Fort + * (**) Used by Fort, but in its fatal uthash form. */ #define HASH_NONFATAL_OOM 1 #define uthash_nonfatal_oom(obj) \ diff --git a/src/object/tal.c b/src/object/tal.c index b63ca3af..470e39c4 100644 --- a/src/object/tal.c +++ b/src/object/tal.c @@ -69,7 +69,7 @@ SLIST_HEAD(threads_list, validation_thread); struct tal_param { struct thread_pool *pool; struct db_table *db; - struct threads_list *threads; + struct threads_list threads; }; static int @@ -497,7 +497,8 @@ tal_get_spki(struct tal *tal, unsigned char const **buffer, size_t *len) * have been extracted from a TAL. */ static int -handle_tal_uri(struct tal *tal, struct rpki_uri *uri, void *arg) +handle_tal_uri(struct tal *tal, struct rpki_uri *uri, + struct validation_thread *thread) { /* * Because of the way the foreach iterates, this function must return @@ -513,7 +514,6 @@ handle_tal_uri(struct tal *tal, struct rpki_uri *uri, void *arg) * A "hard error" is any other error. */ - struct validation_thread *thread_arg = arg; struct validation_handler validation_handler; struct validation *state; struct cert_stack *certstack; @@ -523,13 +523,13 @@ handle_tal_uri(struct tal *tal, struct rpki_uri *uri, void *arg) validation_handler.handle_roa_v4 = handle_roa_v4; validation_handler.handle_roa_v6 = handle_roa_v6; validation_handler.handle_router_key = handle_router_key; - validation_handler.arg = thread_arg->arg; + validation_handler.arg = thread->arg; error = validation_prepare(&state, tal, &validation_handler); if (error) return ENSURE_NEGATIVE(error); - if (thread_arg->sync_files) { + if (thread->sync_files) { if (uri_is_rsync(uri)) { if (!config_get_rsync_enabled()) { validation_destroy(state); @@ -563,8 +563,8 @@ handle_tal_uri(struct tal *tal, struct rpki_uri *uri, void *arg) } /* At least one URI was sync'd */ - thread_arg->retry_local = false; - if (thread_arg->sync_files && working_repo_peek() != NULL) + thread->retry_local = false; + if (thread->sync_files && working_repo_peek() != NULL) reqs_errors_rem_uri(working_repo_peek()); working_repo_pop(); @@ -675,7 +675,7 @@ do_file_validation(void *thread_arg) if (error) goto destroy_tal; - error = foreach_uri(tal, __handle_tal_uri_sync, thread_arg); + error = foreach_uri(tal, __handle_tal_uri_sync, thread); if (error > 0) { error = 0; goto destroy_tal; @@ -692,7 +692,7 @@ do_file_validation(void *thread_arg) thread->sync_files = false; pr_val_warn("Looking for the TA certificate at the local files."); - error = foreach_uri(tal, __handle_tal_uri_local, thread_arg); + error = foreach_uri(tal, __handle_tal_uri_local, thread); if (error > 0) error = 0; else if (error == 0) @@ -749,7 +749,7 @@ __do_file_validation(char const *tal_file, void *arg) goto free_tal_file; } - SLIST_INSERT_HEAD(t_param->threads, thread, next); + SLIST_INSERT_HEAD(&t_param->threads, thread, next); return 0; free_tal_file: @@ -764,64 +764,52 @@ free_db_rrdp: int perform_standalone_validation(struct thread_pool *pool, struct db_table *table) { - struct tal_param *param; - struct threads_list threads; + struct tal_param param; struct validation_thread *thread; - int error, t_error; - - param = malloc(sizeof(struct tal_param)); - if (param == NULL) - return pr_enomem(); + int error; /* Set existent tal RRDP info to non visited */ db_rrdp_reset_visited_tals(); - SLIST_INIT(&threads); - - param->pool = pool; - param->db = table; - param->threads = &threads; + param.pool = pool; + param.db = table; + SLIST_INIT(¶m.threads); error = process_file_or_dir(config_get_tal(), TAL_FILE_EXTENSION, true, - __do_file_validation, param); + __do_file_validation, ¶m); if (error) { /* End all thread data */ - while (!SLIST_EMPTY(&threads)) { - thread = threads.slh_first; - SLIST_REMOVE_HEAD(&threads, next); + while (!SLIST_EMPTY(¶m.threads)) { + thread = SLIST_FIRST(¶m.threads); + SLIST_REMOVE_HEAD(¶m.threads, next); thread_destroy(thread); } - free(param); return error; } /* Wait for all */ thread_pool_wait(pool); - t_error = 0; - while (!SLIST_EMPTY(&threads)) { - thread = threads.slh_first; - SLIST_REMOVE_HEAD(&threads, next); + while (!SLIST_EMPTY(¶m.threads)) { + thread = SLIST_FIRST(¶m.threads); + SLIST_REMOVE_HEAD(¶m.threads, next); if (thread->exit_status) { - t_error = thread->exit_status; + error = thread->exit_status; pr_op_warn("Validation from TAL '%s' yielded error, discarding any other validation results.", thread->tal_file); } thread_destroy(thread); } - /* The parameter isn't needed anymore */ - free(param); - /* Log the error'd URIs summary */ reqs_errors_log_summary(); /* One thread has errors, validation can't keep the resulting table */ - if (t_error) - return t_error; + if (error) + return error; /* Remove non-visited rrdps URIS by tal */ db_rrdp_rem_nonvisited_tals(); - return error; + return 0; } diff --git a/src/output_printer.c b/src/output_printer.c index 34edcf86..570ef2f2 100644 --- a/src/output_printer.c +++ b/src/output_printer.c @@ -157,7 +157,7 @@ open_file(char const *loc, FILE **out, bool *fopen) } static void -print_roas(struct db_table *db) +print_roas(struct db_table const *db) { FILE *out; JSON_OUT json_out; @@ -188,7 +188,7 @@ print_roas(struct db_table *db) } static void -print_router_keys(struct db_table *db) +print_router_keys(struct db_table const *db) { FILE *out; JSON_OUT json_out; @@ -222,7 +222,7 @@ print_router_keys(struct db_table *db) } void -output_print_data(struct db_table *db) +output_print_data(struct db_table const *db) { print_roas(db); print_router_keys(db); diff --git a/src/output_printer.h b/src/output_printer.h index 4bb089e2..92b5b6a1 100644 --- a/src/output_printer.h +++ b/src/output_printer.h @@ -3,6 +3,6 @@ #include "rtr/db/db_table.h" -void output_print_data(struct db_table *); +void output_print_data(struct db_table const *); #endif /* SRC_OUTPUT_PRINTER_H_ */ diff --git a/src/reqs_errors.c b/src/reqs_errors.c index 951b93e0..03c3157f 100644 --- a/src/reqs_errors.c +++ b/src/reqs_errors.c @@ -4,7 +4,7 @@ #include #include -#include "data_structure/uthash_nonfatal.h" +#include "data_structure/uthash.h" #include "common.h" #include "config.h" #include "log.h" diff --git a/src/rrdp/db/db_rrdp_uris.c b/src/rrdp/db/db_rrdp_uris.c index 106c4b1e..ea9e1533 100644 --- a/src/rrdp/db/db_rrdp_uris.c +++ b/src/rrdp/db/db_rrdp_uris.c @@ -3,7 +3,7 @@ #include #include #include -#include "data_structure/uthash_nonfatal.h" +#include "data_structure/uthash.h" #include "common.h" #include "log.h" #include "thread_var.h" @@ -94,6 +94,10 @@ add_rrdp_uri(struct db_rrdp_uri *uris, struct uris_table *new_uri) { struct uris_table *old_uri; + /* + * TODO (fine) this should use HASH_REPLACE instead of HASH_ADD_KEYPTR + */ + old_uri = find_rrdp_uri(uris, new_uri->uri); if (old_uri != NULL) { HASH_DELETE(hh, uris->table, old_uri); diff --git a/src/rtr/db/db_table.c b/src/rtr/db/db_table.c index 11eab2cb..f18ee15b 100644 --- a/src/rtr/db/db_table.c +++ b/src/rtr/db/db_table.c @@ -57,7 +57,7 @@ db_table_destroy(struct db_table *table) } int -db_table_foreach_roa(struct db_table *table, vrp_foreach_cb cb, void *arg) +db_table_foreach_roa(struct db_table const *table, vrp_foreach_cb cb, void *arg) { struct hashable_roa *node, *tmp; int error; @@ -72,8 +72,8 @@ db_table_foreach_roa(struct db_table *table, vrp_foreach_cb cb, void *arg) } int -db_table_foreach_router_key(struct db_table *table, router_key_foreach_cb cb, - void *arg) +db_table_foreach_router_key(struct db_table const *table, + router_key_foreach_cb cb, void *arg) { struct hashable_key *node, *tmp; int error; diff --git a/src/rtr/db/db_table.h b/src/rtr/db/db_table.h index 603bd702..8562567a 100644 --- a/src/rtr/db/db_table.h +++ b/src/rtr/db/db_table.h @@ -13,10 +13,10 @@ void db_table_destroy(struct db_table *); unsigned int db_table_roa_count(struct db_table *); unsigned int db_table_router_key_count(struct db_table *); -int db_table_foreach_roa(struct db_table *, vrp_foreach_cb, void *); +int db_table_foreach_roa(struct db_table const *, vrp_foreach_cb, void *); void db_table_remove_roa(struct db_table *, struct vrp const *); -int db_table_foreach_router_key(struct db_table *, router_key_foreach_cb, +int db_table_foreach_router_key(struct db_table const *, router_key_foreach_cb, void *); void db_table_remove_router_key(struct db_table *, struct router_key const *); diff --git a/src/rtr/db/vrps.c b/src/rtr/db/vrps.c index 40a3468b..59a9875d 100644 --- a/src/rtr/db/vrps.c +++ b/src/rtr/db/vrps.c @@ -168,35 +168,32 @@ vrps_destroy(void) db_table_destroy(state.base); } -#define WLOCK_HANDLER(lock, cb) \ +#define WLOCK_HANDLER(cb) \ int error; \ - rwlock_write_lock(lock); \ + rwlock_write_lock(&table_lock); \ error = cb; \ - rwlock_unlock(lock); \ + rwlock_unlock(&table_lock); \ return error; int handle_roa_v4(uint32_t as, struct ipv4_prefix const *prefix, uint8_t max_length, void *arg) { - WLOCK_HANDLER(&table_lock, - rtrhandler_handle_roa_v4(arg, as, prefix, max_length)) + WLOCK_HANDLER(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) { - WLOCK_HANDLER(&table_lock, - rtrhandler_handle_roa_v6(arg, as, prefix, max_length)) + WLOCK_HANDLER(rtrhandler_handle_roa_v6(arg, as, prefix, max_length)) } int handle_router_key(unsigned char const *ski, uint32_t as, unsigned char const *spk, void *arg) { - WLOCK_HANDLER(&table_lock, - rtrhandler_handle_router_key(arg, ski, as, spk)) + WLOCK_HANDLER(rtrhandler_handle_router_key(arg, ski, as, spk)) } static int diff --git a/src/validation_handler.c b/src/validation_handler.c index fd58b72e..364ee1c5 100644 --- a/src/validation_handler.c +++ b/src/validation_handler.c @@ -4,21 +4,20 @@ #include "log.h" #include "thread_var.h" -static int -get_current_threads_handler(struct validation_handler const **result) +static struct validation_handler const * +get_current_threads_handler(void) { struct validation *state; struct validation_handler const *handler; state = state_retrieve(); if (state == NULL) - return -EINVAL; + return NULL; handler = validation_get_validation_handler(state); if (handler == NULL) pr_crit("This thread lacks a validation handler."); - *result = handler; - return 0; + return handler; } int @@ -26,11 +25,10 @@ vhandler_handle_roa_v4(uint32_t as, struct ipv4_prefix const *prefix, uint8_t max_length) { struct validation_handler const *handler; - int error; - error = get_current_threads_handler(&handler); - if (error) - return error; + handler = get_current_threads_handler(); + if (handler == NULL) + return -EINVAL; return (handler->handle_roa_v4 != NULL) ? handler->handle_roa_v4(as, prefix, max_length, handler->arg) @@ -42,11 +40,10 @@ vhandler_handle_roa_v6(uint32_t as, struct ipv6_prefix const *prefix, uint8_t max_length) { struct validation_handler const *handler; - int error; - error = get_current_threads_handler(&handler); - if (error) - return error; + handler = get_current_threads_handler(); + if (handler == NULL) + return -EINVAL; return (handler->handle_roa_v6 != NULL) ? handler->handle_roa_v6(as, prefix, max_length, handler->arg) @@ -58,11 +55,10 @@ vhandler_handle_router_key(unsigned char const *ski, uint32_t as, unsigned char const *spk) { struct validation_handler const *handler; - int error; - error = get_current_threads_handler(&handler); - if (error) - return error; + handler = get_current_threads_handler(); + if (handler == NULL) + return -EINVAL; return (handler->handle_router_key != NULL) ? handler->handle_router_key(ski, as, spk, handler->arg) diff --git a/src/visited_uris.c b/src/visited_uris.c index 987c7357..be9fdc8d 100644 --- a/src/visited_uris.c +++ b/src/visited_uris.c @@ -6,7 +6,7 @@ #include "log.h" #include "delete_dir_daemon.h" #include "data_structure/array_list.h" -#include "data_structure/uthash_nonfatal.h" +#include "data_structure/uthash.h" struct visited_elem { /* key */