Static analysis coverity fixes (#13)

Fixes includes:
1. Explicitly 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.
6. Fix for TOCTOU: Use mkdir directly and check for errno instead of
   using stat and then mkdir.
7. Added explicit break statements in switch case.
8. int to enum explicit type casting.

Signed-off-by: Priyankar Jain <[email protected]>

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;

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,13 +609,23 @@ 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,
                                                 cb_args->ips_on_host);
 
     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);

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];

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);
     }
 

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);

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);
 

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,15 +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);
-        if (lvl == -1) {
-            LOG(ERROR, "%s: Unable to parse log level in config.", __func__);
-            exit(EXIT_FAILURE);
-        }
-
-        conf->log_lvl = lvl;
+        conf->log_lvl = parse_log_level_str(val);
+        g_free((gpointer)val);
         return;
     }
 

src/log.c

@@ -15,6 +15,7 @@
 #include <fcntl.h>
 #include <stdarg.h>
 #include <stdbool.h>
+#include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -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;
     }
-    ret = strftime(buf, 100, "%F %T", gmtime(&tp.tv_sec));
-    ret += snprintf(buf + ret, 100, ".%06u", tp.tv_nsec/1000);
-    return ret;
+
+    tm_info = gmtime(&tp.tv_sec);
+    if (tm_info == NULL) {
+        printf("ERROR: can't get the clock time. \n");
+        return num_bytes;
+    }
+    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,
@@ -238,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;
@@ -295,12 +307,15 @@ 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;
+    char *log_file_path = NULL;
     int level, fd;
 
-    if (stat(log_dir, &st) == -1) {
-        mkdir(log_dir, 0700);
+    if (mkdir(log_dir, 0700) == -1) {
+        if (errno != EEXIST) {
+            printf("ERROR: Cannot create dir %s. %s\n", log_dir,
+                   strerror(errno));
+            goto err;
+        }
     }
 
     log_file_path = g_malloc0(sizeof(char) * 512);
@@ -319,6 +334,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:

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
@@ -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);

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
@@ -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

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