From 061b26496e68f8e6959128856702e827a68d103c Mon Sep 17 00:00:00 2001 From: Quentin Armitage Date: Wed, 23 Oct 2024 14:25:48 +0100 Subject: [PATCH 1/3] all: change checking process name at reload to include not NULL checks The code was using the reload variable as an indicator that prev_global_data was not NULL, and this was causing some static code analysers to to flag up NULL pointer dereferences. The patch explicitly checks whether prev_global_data is NULL or not, since this is synonymous with testing the reload variable. Signed-off-by: Quentin Armitage --- keepalived/bfd/bfd_daemon.c | 5 +++-- keepalived/check/check_daemon.c | 5 +++-- keepalived/vrrp/vrrp_daemon.c | 5 +++-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/keepalived/bfd/bfd_daemon.c b/keepalived/bfd/bfd_daemon.c index 5f908662d..113f86cba 100644 --- a/keepalived/bfd/bfd_daemon.c +++ b/keepalived/bfd/bfd_daemon.c @@ -159,8 +159,9 @@ start_bfd(__attribute__((unused)) data_t *prev_global_data) init_global_data(global_data, prev_global_data, true); /* Update process name if necessary */ - if ((!reload && global_data->bfd_process_name) || - (reload && + if ((!prev_global_data && // startup + global_data->bfd_process_name) || + (prev_global_data && // reload (!global_data->bfd_process_name != !prev_global_data->bfd_process_name || (global_data->bfd_process_name && strcmp(global_data->bfd_process_name, prev_global_data->bfd_process_name))))) set_process_name(global_data->bfd_process_name); diff --git a/keepalived/check/check_daemon.c b/keepalived/check/check_daemon.c index d98411e18..d5bbae4fb 100644 --- a/keepalived/check/check_daemon.c +++ b/keepalived/check/check_daemon.c @@ -326,8 +326,9 @@ start_check(data_t *prev_global_data) init_global_data(global_data, prev_global_data, true); /* Update process name if necessary */ - if ((!reload && global_data->lvs_process_name) || - (reload && + if ((!prev_global_data && // startup + global_data->lvs_process_name) || + (prev_global_data && // reload (!global_data->lvs_process_name != !prev_global_data->lvs_process_name || (global_data->lvs_process_name && strcmp(global_data->lvs_process_name, prev_global_data->lvs_process_name))))) set_process_name(global_data->lvs_process_name); diff --git a/keepalived/vrrp/vrrp_daemon.c b/keepalived/vrrp/vrrp_daemon.c index 6201aa7e8..09c3557f6 100644 --- a/keepalived/vrrp/vrrp_daemon.c +++ b/keepalived/vrrp/vrrp_daemon.c @@ -526,8 +526,9 @@ start_vrrp(data_t *prev_global_data) init_data(conf_file, vrrp_init_keywords, false); /* Update process name if necessary */ - if ((!reload && global_data->vrrp_process_name) || - (reload && + if ((!prev_global_data && // startup + global_data->vrrp_process_name) || + (prev_global_data && // reload (!global_data->vrrp_process_name != !prev_global_data->vrrp_process_name || (global_data->vrrp_process_name && strcmp(global_data->vrrp_process_name, prev_global_data->vrrp_process_name))))) set_process_name(global_data->vrrp_process_name); From 43889d17d6788fa3fe457de3819f2891abd1f4f4 Mon Sep 17 00:00:00 2001 From: Quentin Armitage Date: Wed, 23 Oct 2024 14:30:33 +0100 Subject: [PATCH 2/3] vrrp: Only use dbus_{in,out}_pipe[0] to indicate pipe is closed The code was, in some places, also setting dbus_{in,out}_pipe[1] to -1 when it closed the pipes, which is unnecessary. In other places it set dbus_{in,out}_pipe[0] to -1 twice, and did not set the [1] equivalents at all, which was just wrong. Signed-off-by: Quentin Armitage --- keepalived/vrrp/vrrp_dbus.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/keepalived/vrrp/vrrp_dbus.c b/keepalived/vrrp/vrrp_dbus.c index 8f30c803e..dab0fd87f 100644 --- a/keepalived/vrrp/vrrp_dbus.c +++ b/keepalived/vrrp/vrrp_dbus.c @@ -136,8 +136,8 @@ static GMainLoop *loop; /* Data passing between main vrrp thread and dbus thread */ dbus_queue_ent_t *ent_ptr; -static int dbus_in_pipe[2] = {-1, -1}; -static int dbus_out_pipe[2] = {-1, -1}; +static int dbus_in_pipe[2] = { -1 }; // [0] == -1 indicates pipe is closed +static int dbus_out_pipe[2] = { -1 }; // Ditto static sem_t thread_end; /* The only characters that are valid in a dbus path are A-Z, a-z, 0-9, _ */ @@ -942,14 +942,12 @@ dbus_start_error(dbus_files_t *files) close(dbus_in_pipe[0]); close(dbus_in_pipe[1]); dbus_in_pipe[0] = -1; - dbus_in_pipe[1] = -1; } if (dbus_out_pipe[0] != -1) { close(dbus_out_pipe[0]); close(dbus_out_pipe[1]); dbus_out_pipe[0] = -1; - dbus_out_pipe[1] = -1; } return false; @@ -1074,11 +1072,9 @@ dbus_stop(void) close(dbus_in_pipe[0]); close(dbus_in_pipe[1]); dbus_in_pipe[0] = -1; - dbus_in_pipe[0] = -1; close(dbus_out_pipe[0]); close(dbus_out_pipe[1]); dbus_out_pipe[0] = -1; - dbus_out_pipe[0] = -1; } #ifdef THREAD_DUMP From 3f67cc6d2f3b65e4b83f610bca3dc4b8d56405dc Mon Sep 17 00:00:00 2001 From: Quentin Armitage Date: Wed, 23 Oct 2024 14:36:13 +0100 Subject: [PATCH 3/3] all: clear pointers to old data structures freed after reload This means that if that if there is a subsequent reference to the old data via thoe old_global_data, or old{bfd,check,vrrp} pointers, it should cause a segfault rather than undefined behaviour. It will also make it more straightford to debug any problem should it occur. Signed-off-by: Quentin Armitage --- keepalived/bfd/bfd_daemon.c | 8 ++++---- keepalived/bfd/bfd_data.c | 6 +++++- keepalived/check/check_daemon.c | 8 ++++---- keepalived/check/check_data.c | 6 +++++- keepalived/core/global_data.c | 6 +++++- keepalived/core/main.c | 6 +++--- keepalived/include/bfd_data.h | 2 +- keepalived/include/check_data.h | 2 +- keepalived/include/global_data.h | 2 +- keepalived/include/vrrp_data.h | 2 +- keepalived/vrrp/vrrp_daemon.c | 10 ++++------ keepalived/vrrp/vrrp_data.c | 6 +++++- 12 files changed, 39 insertions(+), 25 deletions(-) diff --git a/keepalived/bfd/bfd_daemon.c b/keepalived/bfd/bfd_daemon.c index 113f86cba..e5fe7cc4a 100644 --- a/keepalived/bfd/bfd_daemon.c +++ b/keepalived/bfd/bfd_daemon.c @@ -85,9 +85,9 @@ stop_bfd(int status) pidfile_rm(&bfd_pidfile); /* Clean data */ - free_global_data(global_data); + free_global_data(&global_data); bfd_dispatcher_release(bfd_data); - free_bfd_data(bfd_data); + free_bfd_data(&bfd_data); free_bfd_buffer(); thread_destroy_master(master); free_parent_mallocs_exit(); @@ -288,8 +288,8 @@ reload_bfd_thread(__attribute__((unused)) thread_ref_t thread) signal_set(SIGCHLD, thread_child_handler, master); start_bfd(old_global_data); - free_bfd_data(old_bfd_data); - free_global_data(old_global_data); + free_bfd_data(&old_bfd_data); + free_global_data(&old_global_data); #ifndef _ONE_PROCESS_DEBUG_ save_config(true, "bfd", dump_bfd_data_global); diff --git a/keepalived/bfd/bfd_data.c b/keepalived/bfd/bfd_data.c index abd758fba..6f0953806 100644 --- a/keepalived/bfd/bfd_data.c +++ b/keepalived/bfd/bfd_data.c @@ -252,12 +252,16 @@ alloc_bfd_data(void) } void -free_bfd_data(bfd_data_t *data) +free_bfd_data(bfd_data_t **datap) { + bfd_data_t *data = *datap; + assert(data); free_bfd_list(&data->bfd); FREE(data); + + *datap = NULL; } void diff --git a/keepalived/check/check_daemon.c b/keepalived/check/check_daemon.c index d5bbae4fb..120798801 100644 --- a/keepalived/check/check_daemon.c +++ b/keepalived/check/check_daemon.c @@ -181,9 +181,9 @@ checker_terminate_phase2(void) /* Clean data */ if (global_data) - free_global_data(global_data); + free_global_data(&global_data); if (check_data) - free_check_data(check_data); + free_check_data(&check_data); free_parent_mallocs_exit(); /* @@ -519,8 +519,8 @@ reload_check_thread(__attribute__((unused)) thread_ref_t thread) start_check(old_global_data); /* free backup data */ - free_check_data(old_check_data); - free_global_data(old_global_data); + free_check_data(&old_check_data); + free_global_data(&old_global_data); #ifndef _ONE_PROCESS_DEBUG_ save_config(true, "check", dump_data_check); diff --git a/keepalived/check/check_data.c b/keepalived/check/check_data.c index 913f63364..9e5fa4ec8 100644 --- a/keepalived/check/check_data.c +++ b/keepalived/check/check_data.c @@ -924,8 +924,10 @@ alloc_check_data(void) } void -free_check_data(check_data_t *data) +free_check_data(check_data_t **datap) { + check_data_t *data = *datap; + free_vs_list(&data->vs); free_vsg_list(&data->vs_group); free_track_file_list(&data->track_files); @@ -933,6 +935,8 @@ free_check_data(check_data_t *data) free_checker_bfd_list(&data->track_bfds); #endif FREE(data); + + *datap = NULL; } static void diff --git a/keepalived/core/global_data.c b/keepalived/core/global_data.c index 96a94cafd..fc2a6759b 100644 --- a/keepalived/core/global_data.c +++ b/keepalived/core/global_data.c @@ -424,8 +424,10 @@ init_global_data(data_t * data, data_t *prev_global_data, bool copy_unchangeable } void -free_global_data(data_t * data) +free_global_data(data_t **datap) { + data_t *data = *datap; + if (!data) return; @@ -502,6 +504,8 @@ free_global_data(data_t * data) #endif FREE_CONST_PTR(data->config_directory); FREE(data); + + *datap = NULL; } FILE * __attribute__((malloc)) diff --git a/keepalived/core/main.c b/keepalived/core/main.c index a9c7f2e23..52de890b3 100644 --- a/keepalived/core/main.c +++ b/keepalived/core/main.c @@ -831,7 +831,7 @@ static bool reload_config(void) if (unsupported_change) { /* We cannot reload the configuration, so continue with the old config */ - free_global_data (global_data); + free_global_data(&global_data); global_data = old_global_data; } else { @@ -840,7 +840,7 @@ static bool reload_config(void) (global_data->process_name && strcmp(global_data->process_name, old_global_data->process_name))) set_process_name(global_data->process_name); - free_global_data (old_global_data); + free_global_data(&old_global_data); } /* There is no point checking the script security of the @@ -2910,7 +2910,7 @@ keepalived_main(int argc, char **argv) free_parent_mallocs_startup(false); free_parent_mallocs_exit(); - free_global_data(global_data); + free_global_data(&global_data); closelog(); diff --git a/keepalived/include/bfd_data.h b/keepalived/include/bfd_data.h index 7d3b9500d..0ce881be1 100644 --- a/keepalived/include/bfd_data.h +++ b/keepalived/include/bfd_data.h @@ -51,7 +51,7 @@ extern void dump_bfd_data(FILE *, const bfd_data_t *); extern void dump_bfd_data_global(FILE *); #endif extern void bfd_print_data(void); -extern void free_bfd_data(bfd_data_t *); +extern void free_bfd_data(bfd_data_t **); extern void bfd_complete_init(void); extern void alloc_bfd_buffer(void); extern void free_bfd_buffer(void); diff --git a/keepalived/include/check_data.h b/keepalived/include/check_data.h index 1beed31db..9e9662bef 100644 --- a/keepalived/include/check_data.h +++ b/keepalived/include/check_data.h @@ -319,7 +319,7 @@ extern void alloc_ssvr(const char *, const char *); extern void free_checker_bfd(checker_tracked_bfd_t *); #endif extern check_data_t *alloc_check_data(void); -extern void free_check_data(check_data_t *); +extern void free_check_data(check_data_t **); extern void dump_data_check(FILE *); extern const char *format_vs (const virtual_server_t *); extern const char *format_vsge (const virtual_server_group_entry_t *); diff --git a/keepalived/include/global_data.h b/keepalived/include/global_data.h index 3440a6163..72701e051 100644 --- a/keepalived/include/global_data.h +++ b/keepalived/include/global_data.h @@ -305,7 +305,7 @@ extern const char * format_email_addr(const char *); extern void alloc_email(const char *); extern data_t *alloc_global_data(void); extern void init_global_data(data_t *, data_t *, bool); -extern void free_global_data(data_t *); +extern void free_global_data(data_t **); extern FILE *open_dump_file(const char *) __attribute__((malloc)); extern void dump_global_data(FILE *, data_t *); diff --git a/keepalived/include/vrrp_data.h b/keepalived/include/vrrp_data.h index 18cc28947..d4b3f283f 100644 --- a/keepalived/include/vrrp_data.h +++ b/keepalived/include/vrrp_data.h @@ -101,7 +101,7 @@ extern void alloc_vrrp_vrule(const vector_t *); extern void alloc_vrrp_buffer(size_t); extern void free_vrrp_buffer(void); extern vrrp_data_t *alloc_vrrp_data(void); -extern void free_vrrp_data(vrrp_data_t *); +extern void free_vrrp_data(vrrp_data_t **); extern void free_sync_group(vrrp_sgroup_t *); extern void free_sock_list(list_head_t *); extern void dump_sock_list(FILE *, const list_head_t *); diff --git a/keepalived/vrrp/vrrp_daemon.c b/keepalived/vrrp/vrrp_daemon.c index 09c3557f6..1f0fd6928 100644 --- a/keepalived/vrrp/vrrp_daemon.c +++ b/keepalived/vrrp/vrrp_daemon.c @@ -324,8 +324,8 @@ vrrp_terminate_phase2(int exit_status) if (global_data->disable_local_igmp) reset_disable_local_igmp(); - free_global_data(global_data); - free_vrrp_data(vrrp_data); + free_global_data(&global_data); + free_vrrp_data(&vrrp_data); free_vrrp_buffer(); free_interface_queue(); free_parent_mallocs_exit(); @@ -894,10 +894,8 @@ reload_vrrp_thread(__attribute__((unused)) thread_ref_t thread) #endif /* free backup data */ - free_vrrp_data(old_vrrp_data); - old_vrrp_data = NULL; - free_global_data(old_global_data); - old_global_data = NULL; + free_vrrp_data(&old_vrrp_data); + free_global_data(&old_global_data); free_old_interface_queue(); diff --git a/keepalived/vrrp/vrrp_data.c b/keepalived/vrrp/vrrp_data.c index 16594c9a1..e3a55207b 100644 --- a/keepalived/vrrp/vrrp_data.c +++ b/keepalived/vrrp/vrrp_data.c @@ -1260,8 +1260,10 @@ alloc_vrrp_data(void) } void -free_vrrp_data(vrrp_data_t * data) +free_vrrp_data(vrrp_data_t ** datap) { + vrrp_data_t *data = *datap; + free_ipaddress_list(&data->static_addresses); free_iproute_list(&data->static_routes); free_iprule_list(&data->static_rules); @@ -1277,6 +1279,8 @@ free_vrrp_data(vrrp_data_t * data) #endif free_vrrp_list(&data->vrrp); FREE(data); + + *datap = NULL; } static void