From 13321d02e9bf48bcf81758e20d5cee2a82735426 Mon Sep 17 00:00:00 2001 From: Alberto Leiva Popper Date: Fri, 11 Jun 2021 14:59:29 -0500 Subject: [PATCH] Log: Revert ad841d5024bea7f7d9243a0aae5fdecc40afcd3b ad841d50 was a mistake. It was never agreed in #40 that Fort should shotgun blast its own face on the first ENOMEM, and even if it was, the idea is preposterous. Memory allocation failures are neither programming errors nor an excuse to leave all the routers hanging. While there's some truth to the notion that a Fort memory leak (which has been exhausting memory over time) could be temporarily amended by killing Fort (and letting the OS clean it up), the argument completely misses the reality that memory allocation failures could happen regardless of the existence of a memory leak. A memory leak is a valid reason to throw away the results of a current validation run (as long as the admin is warned), but an existing validation result and the RTR server must remain afloat. Also includes a pr_enomem() caller review. --- src/asn1/decode.c | 3 --- src/common.c | 16 ---------------- src/common.h | 3 --- src/config.c | 2 +- src/log.c | 23 +++++++++++++++++++++-- src/main.c | 10 +++++++--- src/rrdp/rrdp_objects.c | 2 +- src/rsync/rsync.c | 20 ++++++++++---------- src/rtr/pdu_handler.c | 2 +- src/rtr/primitive_reader.c | 2 +- src/slurm/db_slurm.c | 16 ++++------------ src/uri.c | 4 ++-- 12 files changed, 48 insertions(+), 55 deletions(-) diff --git a/src/asn1/decode.c b/src/asn1/decode.c index 4e9b2437..f4703d1d 100644 --- a/src/asn1/decode.c +++ b/src/asn1/decode.c @@ -96,9 +96,6 @@ asn1_decode(const void *buffer, size_t buffer_size, rval = ber_decode(&s_codec_ctx, descriptor, result, buffer, buffer_size); if (rval.code != RC_OK) { - if (rval.code == RC_FAIL && errno == ENOMEM) - pr_enomem(); - /* Must free partial object according to API contracts. */ ASN_STRUCT_FREE(*descriptor, *result); /* We expect the data to be complete; RC_WMORE is an error. */ diff --git a/src/common.c b/src/common.c index 83b6fe67..f49bb744 100644 --- a/src/common.c +++ b/src/common.c @@ -68,22 +68,6 @@ rwlock_unlock(pthread_rwlock_t *lock) error); } -void -close_thread(pthread_t thread, char const *what) -{ - int error; - - error = pthread_cancel(thread); - if (error && error != ESRCH) - pr_crit("pthread_cancel() threw %d on the '%s' thread.", - error, what); - - error = pthread_join(thread, NULL); - if (error) - pr_crit("pthread_join() threw %d on the '%s' thread.", - error, what); -} - static int process_file(char const *dir_name, char const *file_name, char const *file_ext, int *fcount, process_file_cb cb, void *arg) diff --git a/src/common.h b/src/common.h index 24fd976a..1c92c41e 100644 --- a/src/common.h +++ b/src/common.h @@ -47,9 +47,6 @@ int rwlock_read_lock(pthread_rwlock_t *); void rwlock_write_lock(pthread_rwlock_t *); void rwlock_unlock(pthread_rwlock_t *); -/** Also boilerplate. */ -void close_thread(pthread_t thread, char const *); - typedef int (*process_file_cb)(char const *, void *); int process_file_or_dir(char const *, char const *, bool, process_file_cb, void *); diff --git a/src/config.c b/src/config.c index 5194e288..adbe8618 100644 --- a/src/config.c +++ b/src/config.c @@ -964,7 +964,7 @@ set_default_values(void) error = string_array_init(&rpki_config.server.address, NULL, 0); if (error) - return pr_enomem(); + return error; rpki_config.server.port = strdup("323"); if (rpki_config.server.port == NULL) { diff --git a/src/log.c b/src/log.c index d2c3d243..9e8a65be 100644 --- a/src/log.c +++ b/src/log.c @@ -546,8 +546,27 @@ val_crypto_err(const char *format, ...) int pr_enomem(void) { - /* TODO this being a critical is not acceptable */ - pr_crit("Out of memory."); + /* + * I'm not using PR_SIMPLE and friends, because those allocate. + * We want to minimize allocations after a memory allocation failure. + */ + + if (LOG_ERR > op_config.level) + return -ENOMEM; + + if (op_config.syslog_enabled) { + lock_mutex(); + syslog(LOG_ERR | op_config.facility, "Out of memory."); + unlock_mutex(); + } + + if (op_config.fprintf_enabled) { + lock_mutex(); + fprintf(stderr, "Out of memory.\n"); + unlock_mutex(); + } + + return -ENOMEM; } __dead void diff --git a/src/main.c b/src/main.c index c85c7bb6..0d845d5d 100644 --- a/src/main.c +++ b/src/main.c @@ -34,12 +34,15 @@ main(int argc, char **argv) { int error; - printf("Fort 1.5.0.1\n"); /* Initializations */ error = log_setup(); if (error) - return error; + goto just_quit; + + /* TODO (issue49) don't forget to improve this. */ + pr_op_info("Fort 1.5.0.2"); + error = thvar_init(); if (error) goto revert_log; @@ -115,5 +118,6 @@ revert_config: revert_log: log_teardown(); PR_DEBUG_MSG("Main thread returning."); - return error; +just_quit: + return abs(error); } diff --git a/src/rrdp/rrdp_objects.c b/src/rrdp/rrdp_objects.c index 24807ba2..b8d1c056 100644 --- a/src/rrdp/rrdp_objects.c +++ b/src/rrdp/rrdp_objects.c @@ -246,7 +246,7 @@ update_notification_create(struct update_notification **file) error = deltas_head_create(&list); if (error) { free(tmp); - return pr_enomem(); + return error; } tmp->deltas_list = list; tmp->uri = NULL; diff --git a/src/rsync/rsync.c b/src/rsync/rsync.c index 45b9ea16..f5129a61 100644 --- a/src/rsync/rsync.c +++ b/src/rsync/rsync.c @@ -248,7 +248,7 @@ prepare_rsync(struct rpki_uri *uri, bool is_ta, char ***args, size_t *args_len) return 0; } -static void +__dead static void handle_child_thread(char **args, int fds[2][2]) { /* THIS FUNCTION MUST NEVER RETURN!!! */ @@ -280,17 +280,16 @@ create_pipes(int fds[2][2]) return 0; } -static void +static int log_buffer(char const *buffer, ssize_t read, int type, bool log_operation) { #define PRE_RSYNC "[RSYNC exec]: " char *cpy, *cur, *tmp; cpy = malloc(read + 1); - if (cpy == NULL) { - pr_enomem(); - return; - } + if (cpy == NULL) + return pr_enomem(); + strncpy(cpy, buffer, read); cpy[read] = '\0'; @@ -312,6 +311,7 @@ log_buffer(char const *buffer, ssize_t read, int type, bool log_operation) cur = tmp + 1; } free(cpy); + return 0; #undef PRE_RSYNC } @@ -320,6 +320,7 @@ read_pipe(int fd_pipe[2][2], int type, bool log_operation) { char buffer[4096]; ssize_t count; + int error; while (1) { count = read(fd_pipe[type][0], buffer, sizeof(buffer)); @@ -332,7 +333,9 @@ read_pipe(int fd_pipe[2][2], int type, bool log_operation) if (count == 0) break; - log_buffer(buffer, count, type, log_operation); + error = log_buffer(buffer, count, type, log_operation); + if (error) + return error; } close(fd_pipe[type][0]); /* Close read end */ return 0; @@ -466,9 +469,6 @@ do_rsync(struct rpki_uri *uri, bool is_ta, bool log_operation) TA_DEBUG_MSG("%d", error); pr_val_debug("Child terminated with error code %d.", error); - if (error == -ENOMEM) - pr_enomem(); - if (!error) goto release_args; diff --git a/src/rtr/pdu_handler.c b/src/rtr/pdu_handler.c index 6c8bbd4a..1ed8532b 100644 --- a/src/rtr/pdu_handler.c +++ b/src/rtr/pdu_handler.c @@ -67,7 +67,7 @@ handle_serial_query_pdu(int fd, struct rtr_request const *request) error = send_cache_reset_pdu(fd, version); goto end; case -ENOMEM: /* Memory allocation failure */ - pr_enomem(); + error = pr_enomem(); goto end; case EAGAIN: /* Too many threads */ /* diff --git a/src/rtr/primitive_reader.c b/src/rtr/primitive_reader.c index 60569bf1..00943f35 100644 --- a/src/rtr/primitive_reader.c +++ b/src/rtr/primitive_reader.c @@ -227,7 +227,7 @@ read_string(struct pdu_reader *reader, uint32_t string_len, rtr_char **result) */ string = malloc(string_len + 1); /* Include NULL chara. */ - if (!string) + if (string == NULL) return pr_enomem(); memcpy(string, reader->buffer, string_len); diff --git a/src/slurm/db_slurm.c b/src/slurm/db_slurm.c index 2a962c0e..564e46c0 100644 --- a/src/slurm/db_slurm.c +++ b/src/slurm/db_slurm.c @@ -386,25 +386,17 @@ db_slurm_vrp_is_filtered(struct db_slurm *db, struct vrp const *vrp) bool db_slurm_bgpsec_is_filtered(struct db_slurm *db, struct router_key const *key) { + unsigned char ski[RK_SKI_LEN]; struct slurm_bgpsec slurm_bgpsec; - unsigned char *tmp; - bool result; - tmp = malloc(RK_SKI_LEN); - if (tmp == NULL) { - pr_enomem(); - return false; - } slurm_bgpsec.data_flag = SLURM_COM_FLAG_ASN | SLURM_BGPS_FLAG_SKI; slurm_bgpsec.asn = key->as; - memcpy(tmp, key->ski, RK_SKI_LEN); - slurm_bgpsec.ski = tmp; + memcpy(ski, key->ski, RK_SKI_LEN); + slurm_bgpsec.ski = ski; /* Router public key isn't used at filters */ slurm_bgpsec.router_public_key = NULL; - result = bgpsec_filtered(db, &slurm_bgpsec); - free(tmp); - return result; + return bgpsec_filtered(db, &slurm_bgpsec); } #define ITERATE_LIST_FUNC(type, object, db_list) \ diff --git a/src/uri.c b/src/uri.c index 392515ba..d640aa49 100644 --- a/src/uri.c +++ b/src/uri.c @@ -168,7 +168,7 @@ ia5str2global(struct rpki_uri *uri, char const *mft, IA5String_t *ia5) slash_pos = strrchr(mft, '/'); if (slash_pos == NULL) { joined = malloc(ia5->size + 1); - if (!joined) + if (joined == NULL) return pr_enomem(); strncpy(joined, (char *) ia5->buf, ia5->size); joined[ia5->size] = '\0'; @@ -178,7 +178,7 @@ ia5str2global(struct rpki_uri *uri, char const *mft, IA5String_t *ia5) dir_len = (slash_pos + 1) - mft; joined = malloc(dir_len + ia5->size + 1); - if (!joined) + if (joined == NULL) return pr_enomem(); strncpy(joined, mft, dir_len); -- 2.47.2