From 65e4ed323b4f707d9b6d8db5f424dd9573e2fde8 Mon Sep 17 00:00:00 2001 From: Priyankar Jain Date: Tue, 20 Aug 2024 15:56:27 +0530 Subject: [PATCH 1/2] Static analysis coverity fixes Fixes includes: 1. Explicit initialise variables before use. 2. NULL checks for pointers before use. 3. Missing deallocations. 4. Changed time(NULL) to timespec to avoid int overflow. 5. Fixed few printfs/LOG. Signed-off-by: Priyankar Jain --- src/common.c | 7 ++++--- src/conntrack.c | 16 +++++++++++++--- src/conntrack_entry.c | 5 ++--- src/conntrack_entry_print.c | 2 +- src/conntrack_store.c | 2 +- src/dbus_server.c | 10 ++++++++-- src/lmct_config.c | 1 + src/log.c | 34 ++++++++++++++++++++++++---------- src/main.c | 4 ++-- src/marshal.c | 6 +++--- 10 files changed, 59 insertions(+), 28 deletions(-) diff --git a/src/common.c b/src/common.c index 466a067..e79ee3b 100644 --- a/src/common.c +++ b/src/common.c @@ -48,7 +48,7 @@ create_hashtable_from_ip_list(const char *ip_list[], int num_ips) ht = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, NULL); for (i = 0; i < num_ips; i++) { - struct in_addr ip; + struct in_addr ip = {0}; int success; success = inet_aton(ip_list[i], &ip); @@ -63,9 +63,10 @@ create_hashtable_from_ip_list(const char *ip_list[], int num_ips) g_hash_table_destroy(ht); return NULL; } + } else { + g_hash_table_insert(ht, GUINT_TO_POINTER(ip.s_addr), + GINT_TO_POINTER(1)); } - g_hash_table_insert(ht, GUINT_TO_POINTER(ip.s_addr), - GINT_TO_POINTER(1)); } return ht; diff --git a/src/conntrack.c b/src/conntrack.c index 6867a5a..5097675 100644 --- a/src/conntrack.c +++ b/src/conntrack.c @@ -302,7 +302,7 @@ create_nfct_filter(GHashTable *ips, bool is_src_filter) { struct nfct_filter *filter; GHashTableIter iter; - gpointer key; + gpointer key = NULL; filter = nfct_filter_create(); if (filter == NULL) { @@ -609,6 +609,11 @@ delete_conntrack_dump_callback(enum nf_conntrack_msg_type type, cb_args = data; src_addr = (struct in_addr *)nfct_get_attr(ct, ATTR_ORIG_IPV4_SRC); dst_addr = (struct in_addr *)nfct_get_attr(ct, ATTR_ORIG_IPV4_DST); + if (src_addr == NULL || dst_addr == NULL) { + LOG(WARNING, "%s: ct entry with NULL src/dst IP received. Skipping.", + __func__); + return NFCT_CB_FAILURE; + } in_ips_migrated = is_src_or_dst_in_hashtable(src_addr, dst_addr, cb_args->ips_migrated); in_ips_on_host = is_src_or_dst_in_hashtable(src_addr, dst_addr, @@ -616,6 +621,11 @@ delete_conntrack_dump_callback(enum nf_conntrack_msg_type type, if (in_ips_migrated && !in_ips_on_host) { uint32_t ct_id = nfct_get_attr_u32(ct, ATTR_ID); + if (ct_id == 0) { + LOG(WARNING, "%s: ct entry with 0 id received. Skipping.", + __func__); + return NFCT_CB_FAILURE; + } g_hash_table_insert(cb_args->ct_store, GUINT_TO_POINTER(ct_id), ct); return NFCT_CB_STOLEN; } @@ -637,10 +647,10 @@ static int ct_entry_delete(struct nfct_handle *h, struct nf_conntrack *ct) { int ret; + char buf[1024] = {0}; ret = nfct_query(h, NFCT_Q_DESTROY, ct); if (ret == -1) { - char buf[1024]; nfct_snprintf(buf, sizeof(buf), ct, NFCT_T_UNKNOWN, NFCT_O_DEFAULT, NFCT_OF_SHOW_LAYER3); LOG(WARNING, "%s: Failed to delete the conntrack entry %s. " @@ -688,7 +698,7 @@ _delete_ct_entries(struct nfct_handle *handle, int ret, failed, success; GHashTable *ct_store; // Hashtable to store the CT entries to be deleted GHashTableIter iter; // Iterator for ct_store - gpointer key, value; + gpointer key, value = NULL; ct_store = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, ct_destroy_g_wrapper); diff --git a/src/conntrack_entry.c b/src/conntrack_entry.c index 9c28b3a..3f83df2 100644 --- a/src/conntrack_entry.c +++ b/src/conntrack_entry.c @@ -445,10 +445,9 @@ get_conntrack_entry_from_update(struct conntrack_entry *ct_entry, offset = offset + ct_entry_attr_to_size[i]; } } - // Case 4: attribute set in conntrack_entry but not in update - if (is_set_in_bitmap(ct_entry->bitmap, i) && - nfct_attr_is_set(ct, nf_ct_attr) <= 0) { + else if ((is_set_in_bitmap(ct_entry->bitmap, i) && + nfct_attr_is_set(ct, nf_ct_attr) <= 0)) { // Take value from original entry attr_value = offset; offset = offset + ct_entry_attr_to_size[i]; diff --git a/src/conntrack_entry_print.c b/src/conntrack_entry_print.c index acdeb95..2968c46 100644 --- a/src/conntrack_entry_print.c +++ b/src/conntrack_entry_print.c @@ -308,7 +308,7 @@ conntrack_entry_to_string(struct conntrack_entry *ct_entry) tmp_u8 = offset_ptr; written = snprintf_tcp_flags(buf + cur_offset, size_left, *tmp_u8, false); - offset_ptr += ct_entry_attr_to_size[CT_ATTR_TCP_ORIG_FLAGS_VALUE]; + offset_ptr += ct_entry_attr_to_size[CT_ATTR_TCP_REPL_FLAGS_VALUE]; UPDATE_OFFSET(size_left, cur_offset, written); } diff --git a/src/conntrack_store.c b/src/conntrack_store.c index b0aaccc..c677955 100644 --- a/src/conntrack_store.c +++ b/src/conntrack_store.c @@ -42,7 +42,7 @@ conntrack_store_new(void) struct conntrack_store *conn_store; int ret; - conn_store = g_malloc(sizeof(struct conntrack_store)); + conn_store = g_malloc0(sizeof(struct conntrack_store)); conn_store->store = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, conntrack_entry_destroy_g_wrapper); diff --git a/src/dbus_server.c b/src/dbus_server.c index cc7540d..877576c 100644 --- a/src/dbus_server.c +++ b/src/dbus_server.c @@ -100,7 +100,12 @@ on_load(VMState1 *object, GDBusMethodInvocation *invocation, char send_buf[MNL_SOCKET_BUFFER_SIZE * 2]; void *curr_batch_offset; struct mnl_nlmsg_batch *batch; - int seq = time(NULL); + struct timespec tp; + int seq = 1; + + if (clock_gettime(CLOCK_REALTIME, &tp) != -1) { + seq = (int)tp.tv_sec; + } LOG(INFO, "%s: Load start", __func__); args = g_dbus_method_invocation_get_parameters(invocation); @@ -287,7 +292,7 @@ on_clear(LmctMgmt *object, GDBusMethodInvocation *invocation, { LOG(INFO, "%s: Clear start", __func__); GVariant *args, *var; - gsize num_ip_address; + gsize num_ip_address = 0; char **ip_addresses; GHashTable *ips_on_host; @@ -359,6 +364,7 @@ on_bus_acquired(GDBusConnection *connection, const gchar *name, const gchar *g_obj_path; g_obj_path = g_strdup_printf("%s", object_path); obj_skeleton = object_skeleton_new(g_obj_path); + g_free((gpointer)g_obj_path); object_skeleton_set_vmstate1(obj_skeleton, vmstate1_obj); object_skeleton_set_lmct_mgmt(obj_skeleton, lmct_mgmt_obj); diff --git a/src/lmct_config.c b/src/lmct_config.c index 7763b26..a53e55e 100644 --- a/src/lmct_config.c +++ b/src/lmct_config.c @@ -80,6 +80,7 @@ populate_log_level(GKeyFile *key_file, struct lmct_config *conf) int lvl; lvl = parse_log_level_str(val); + g_free((gpointer)val); if (lvl == -1) { LOG(ERROR, "%s: Unable to parse log level in config.", __func__); exit(EXIT_FAILURE); diff --git a/src/log.c b/src/log.c index ceb7842..d4b791e 100644 --- a/src/log.c +++ b/src/log.c @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -60,20 +61,28 @@ const char *log_dir = "/var/log/conntrack_migrator"; * Returns: * number of bytes written in buffer. */ -static int +static uint32_t put_timestamp(char *buf) { struct timespec tp; + struct tm* tm_info = NULL; + size_t num_bytes = 0; int ret; ret = clock_gettime(CLOCK_REALTIME, &tp); if (ret == -1) { printf("ERROR: can't get the clock time. \n"); - return 0; + return num_bytes; + } + + tm_info = gmtime(&tp.tv_sec); + if (tm_info == NULL) { + printf("ERROR: can't get the clock time. \n"); + return num_bytes; } - ret = strftime(buf, 100, "%F %T", gmtime(&tp.tv_sec)); - ret += snprintf(buf + ret, 100, ".%06u", tp.tv_nsec/1000); - return ret; + num_bytes = strftime(buf, 100, "%F %T", tm_info); + num_bytes += snprintf(buf + num_bytes, 100, ".%06ld", tp.tv_nsec/1000); + return num_bytes; } /** @@ -91,10 +100,10 @@ put_timestamp(char *buf) * Returns: * number of bytes written. */ -static int +static uint32_t putbuf(char *buf, int level, const char *format, va_list ap) { - int _written = 0; + uint32_t _written = 0; _written += put_timestamp(buf); _written += snprintf(buf + _written, MAX_BUF_LEN - _written, @@ -296,11 +305,15 @@ int init_log(enum log_level lvl, const char *helper_id) { struct stat st = { 0 }; - char *log_file_path; - int level, fd; + char *log_file_path = NULL; + int ret, level, fd; if (stat(log_dir, &st) == -1) { - mkdir(log_dir, 0700); + ret = mkdir(log_dir, 0700); + if (ret == -1) { + printf("ERROR: Cannot create dir %s. \n", log_dir); + goto err; + } } log_file_path = g_malloc0(sizeof(char) * 512); @@ -319,6 +332,7 @@ init_log(enum log_level lvl, const char *helper_id) config->pid = getpid(); config->fd = fd; + g_free(log_file_path); return 0; err: diff --git a/src/main.c b/src/main.c index 9e61776..223255f 100644 --- a/src/main.c +++ b/src/main.c @@ -213,8 +213,8 @@ start_in_save_mode(GHashTable *ips_to_migrate, bool *stop_flag) } ret = pthread_setname_np(ct_del_args.tid, "ct_delete"); if (ret != 0) { - LOG(WARNING, "%s: Failed to set thread name \"ct_delete\"", __func__, - strerror(ret)); + LOG(WARNING, "%s: Failed to set thread name \"ct_delete\": %s", + __func__, strerror(ret)); } // Listen for conntrack events which contains their src IP address diff --git a/src/marshal.c b/src/marshal.c index cbff295..46db450 100644 --- a/src/marshal.c +++ b/src/marshal.c @@ -43,7 +43,7 @@ calculate_payload_size(struct conntrack_store *conn_store, { uint32_t payload_size = 0; GHashTableIter iter; - gpointer key, value; + gpointer key, value = NULL; struct conntrack_entry *ct_entry; // First is payload size itself -> uint32_t @@ -138,13 +138,13 @@ marshal_conntrack_store(void *start, struct conntrack_store *conn_store) LOG(VERBOSE, "%s: writing CT entries.", __func__); GHashTableIter iter; - gpointer key, value; + gpointer key, value = NULL; // Write the CT entries. pthread_mutex_lock(&conn_store->lock); g_hash_table_iter_init(&iter, conn_store->store); while (g_hash_table_iter_next(&iter, &key, &value)) { - struct conntrack_entry *ct_entry; + struct conntrack_entry *ct_entry = NULL; ct_entry = (struct conntrack_entry *)value; // Write the bitmap From 5f493ca6d395f90ff0f1ee617ea490826f6c2cba Mon Sep 17 00:00:00 2001 From: Priyankar Jain Date: Wed, 21 Aug 2024 08:59:34 +0530 Subject: [PATCH 2/2] Coverity fixes - part 2 1. Fix for TOCTOU: Use mkdir directly and check for errno instead of using stat and then mkdir. 2. Added explicit break statements in switch case. 3. int to enum explicit type casting. Signed-off-by: Priyankar Jain --- src/lmct_config.c | 19 ++++++------------- src/log.c | 22 ++++++++++++---------- src/main.c | 2 +- src/marshal.c | 8 ++++++-- tests/test_lmct_config.c | 27 ++++++++++----------------- 5 files changed, 35 insertions(+), 43 deletions(-) diff --git a/src/lmct_config.c b/src/lmct_config.c index a53e55e..fae27a0 100644 --- a/src/lmct_config.c +++ b/src/lmct_config.c @@ -37,10 +37,10 @@ struct lmct_config lmct_conf = { * Args: * @val log level string * Returns: - * int log level representation if success in parsing, - * -1 otherwise + * enum: logging level if success in parsing. + * INFO level is chosen as default in case of parsing error. */ -static int +static enum log_level parse_log_level_str(const char *val) { if (strcmp(val, "VERBOSE") == 0) { @@ -55,7 +55,8 @@ parse_log_level_str(const char *val) if (strcmp(val, "ERROR") == 0) { return ERROR; } - return -1; + LOG(ERROR, "%s: Unable to parse log level in config. %s", __func__, val); + return INFO; } /** @@ -77,16 +78,8 @@ populate_log_level(GKeyFile *key_file, struct lmct_config *conf) val = g_key_file_get_string(key_file, "LOG", "level", &error); if (val != NULL) { - int lvl; - - lvl = parse_log_level_str(val); + conf->log_lvl = parse_log_level_str(val); g_free((gpointer)val); - if (lvl == -1) { - LOG(ERROR, "%s: Unable to parse log level in config.", __func__); - exit(EXIT_FAILURE); - } - - conf->log_lvl = lvl; return; } diff --git a/src/log.c b/src/log.c index d4b791e..c7bdc7b 100644 --- a/src/log.c +++ b/src/log.c @@ -247,13 +247,16 @@ parse_log_level(enum log_level lvl) switch(lvl) { case VERBOSE: - log_level |= VERBOSE; + log_level = VERBOSE|INFO|WARNING|ERROR; + break; case INFO: - log_level |= INFO; + log_level = INFO|WARNING|ERROR; + break; case WARNING: - log_level |= WARNING; + log_level = WARNING|ERROR; + break; case ERROR: - log_level |= ERROR; + log_level = ERROR; break; default: log_level = INFO|WARNING|ERROR; @@ -304,14 +307,13 @@ set_log_level(enum log_level level) int init_log(enum log_level lvl, const char *helper_id) { - struct stat st = { 0 }; char *log_file_path = NULL; - int ret, level, fd; + int level, fd; - if (stat(log_dir, &st) == -1) { - ret = mkdir(log_dir, 0700); - if (ret == -1) { - printf("ERROR: Cannot create dir %s. \n", log_dir); + if (mkdir(log_dir, 0700) == -1) { + if (errno != EEXIST) { + printf("ERROR: Cannot create dir %s. %s\n", log_dir, + strerror(errno)); goto err; } } diff --git a/src/main.c b/src/main.c index 223255f..725105e 100644 --- a/src/main.c +++ b/src/main.c @@ -350,7 +350,7 @@ dmain(int argc, char *argv[]) // Start the dbus server dbus_server_args.helper_id = helper_id; dbus_server_args.stop_flag = &stop_flag; - dbus_server_args.mode = mode; + dbus_server_args.mode = (enum op_mode) mode; ret = pthread_create(&dbus_server_args.tid, NULL, dbus_server_init, &dbus_server_args); diff --git a/src/marshal.c b/src/marshal.c index 46db450..f6665b2 100644 --- a/src/marshal.c +++ b/src/marshal.c @@ -198,7 +198,7 @@ handle_no_data_to_migrate(uint32_t *data_size) */ void * marshal(struct conntrack_store *conn_store, struct data_template *data_tmpl, - uint32_t *data_size) + uint32_t *data_size) { uint32_t num_ct_entries = 0; void *buffer, *buffer_offset; @@ -214,7 +214,11 @@ marshal(struct conntrack_store *conn_store, struct data_template *data_tmpl, if (num_ct_entries == 0) { LOG(INFO, "%s: No entries to migrate. Skipping conntrack store " "marshalling", __func__); - return handle_no_data_to_migrate(data_size); + if (data_size == NULL) { + return NULL; + } else { + return handle_no_data_to_migrate(data_size); + } } // CASE 2: Data to migrate exceeds the limit set by the user. This is diff --git a/tests/test_lmct_config.c b/tests/test_lmct_config.c index a616f74..ac863c3 100644 --- a/tests/test_lmct_config.c +++ b/tests/test_lmct_config.c @@ -95,24 +95,17 @@ END_TEST START_TEST(test_lmct_config_invalid_log_level) { - int pid = fork(); - int status; - ck_assert(pid >= 0); - - if (pid == 0) { - char conf_str[] = "[CONNTRACK]\n" - "max_entries_to_migrate=100\n" - "[LOG]\n" - "level=abcdef\n"; + init_config_for_test(); + char conf_str[] = "[CONNTRACK]\n" + "max_entries_to_migrate=100\n" + "[LOG]\n" + "level=abcdef\n"; + prepare_config_file(conf_str); + init_lmct_config(conf_path); + cleanup(); - prepare_config_file(conf_str); - init_lmct_config(conf_path); - exit(EXIT_SUCCESS); - } else { - waitpid(pid, &status, 0); - cleanup(); - ck_assert(WEXITSTATUS(status) == EXIT_FAILURE); - } + ck_assert(lmct_conf.max_entries_to_migrate == 100); + ck_assert(lmct_conf.log_lvl == INFO); } END_TEST