ipcpd: Fix memory leaks

ipcp-data was not correctly destroyed.

6 files changed, 55 insertions, 44 deletions

diff --git a/src/ipcpd/ipcp-data.c b/src/ipcpd/ipcp-data.c
index 8dc708b2..a2fef08c 100644
--- a/src/ipcpd/ipcp-data.c
+++ b/src/ipcpd/ipcp-data.c
@@ -45,13 +45,13 @@ struct dir_entry {
         uint64_t   addr;
 };
 
-static struct reg_entry * reg_entry_create(const char * name)
+static struct reg_entry * reg_entry_create(char * name)
 {
-        struct reg_entry * entry = malloc(sizeof *entry);
+        struct reg_entry * entry = malloc(sizeof(*entry));
         if (entry == NULL)
                 return NULL;
 
-        entry->name = strdup(name);
+        entry->name = name;
         if (entry->name == NULL)
                 return NULL;
 
@@ -63,19 +63,20 @@ static void reg_entry_destroy(struct reg_entry * entry)
         if (entry == NULL)
                 return;
 
-        free(entry->name);
+        if (entry->name != NULL)
+                free(entry->name);
         free(entry);
 }
 
-static struct dir_entry * dir_entry_create(const char * ap_name,
-                                           uint64_t     addr)
+static struct dir_entry * dir_entry_create(char *   ap_name,
+                                           uint64_t addr)
 {
-        struct dir_entry * entry = malloc(sizeof *entry);
+        struct dir_entry * entry = malloc(sizeof(*entry));
         if (entry == NULL)
                 return NULL;
 
         entry->addr    = addr;
-        entry->ap_name = strdup(ap_name);
+        entry->ap_name = ap_name;
         if (entry->ap_name == NULL)
                 return NULL;
 
@@ -87,17 +88,18 @@ static void dir_entry_destroy(struct dir_entry * entry)
         if (entry == NULL)
                 return;
 
-        free(entry->ap_name);
+        if (entry->ap_name != NULL)
+                free(entry->ap_name);
         free(entry);
 }
 
 struct ipcp_data * ipcp_data_create()
 {
-        struct ipcp_data * data = malloc(sizeof *data);
+        struct ipcp_data * data = malloc(sizeof(*data));
         if (data == NULL)
                 return NULL;
 
-        data->type  = 0;
+        data->type = 0;
 
         return data;
 }
@@ -125,16 +127,22 @@ static void clear_registry(struct ipcp_data * data)
 {
         struct list_head * h;
         struct list_head * t;
-        list_for_each_safe(h, t, &data->registry)
-                reg_entry_destroy(list_entry(h, struct reg_entry, list));
+        list_for_each_safe(h, t, &data->registry) {
+                struct reg_entry * e = list_entry(h, struct reg_entry, list);
+                list_del(&e->list);
+                reg_entry_destroy(e);
+        }
 }
 
 static void clear_directory(struct ipcp_data * data)
 {
         struct list_head * h;
         struct list_head * t;
-        list_for_each_safe(h, t, &data->directory)
-                dir_entry_destroy(list_entry(h, struct dir_entry, list));
+        list_for_each_safe(h, t, &data->directory) {
+                struct dir_entry * e = list_entry(h, struct dir_entry, list);
+                list_del(&e->list);
+                dir_entry_destroy(e);
+        }
 }
 
 void ipcp_data_destroy(struct ipcp_data * data)
@@ -142,8 +150,6 @@ void ipcp_data_destroy(struct ipcp_data * data)
         if (data == NULL)
                 return;
 
-        /* FIXME: finish all pending operations here and cancel all threads */
-
         pthread_mutex_lock(&data->reg_lock);
         pthread_mutex_lock(&data->dir_lock);
 
@@ -151,11 +157,11 @@ void ipcp_data_destroy(struct ipcp_data * data)
         clear_registry(data);
         clear_directory(data);
 
-        /*
-         * no need to unlock, just free the entire thing
-         * pthread_mutex_unlock(&data->dir_lock);
-         * pthread_mutex_unlock(&data->reg_lock);
-         */
+        pthread_mutex_unlock(&data->dir_lock);
+        pthread_mutex_unlock(&data->reg_lock);
+
+        pthread_mutex_destroy(&data->dir_lock);
+        pthread_mutex_destroy(&data->reg_lock);
 
         free(data);
 }
@@ -258,7 +264,7 @@ int ipcp_data_del_dir_entry(struct ipcp_data * data,
 }
 
 int ipcp_data_add_reg_entry(struct ipcp_data * data,
-                            const char *       name)
+                            char *             name)
 {
         struct reg_entry * entry;
 
@@ -278,7 +284,7 @@ int ipcp_data_add_reg_entry(struct ipcp_data * data,
                 return -1;
         }
 
-        list_add(&entry->list,&data->registry);
+        list_add(&entry->list, &data->registry);
 
         pthread_mutex_unlock(&data->reg_lock);
 
@@ -286,7 +292,7 @@ int ipcp_data_add_reg_entry(struct ipcp_data * data,
 }
 
 int ipcp_data_add_dir_entry(struct ipcp_data * data,
-                            const char *       ap_name,
+                            char *             ap_name,
                             uint64_t           addr)
 {
         struct dir_entry * entry;
diff --git a/src/ipcpd/ipcp-data.h b/src/ipcpd/ipcp-data.h
index 1e183dca..5bf25649 100644
--- a/src/ipcpd/ipcp-data.h
+++ b/src/ipcpd/ipcp-data.h
@@ -50,11 +50,11 @@ struct ipcp_data * ipcp_data_init(struct ipcp_data * dst,
 void               ipcp_data_destroy(struct ipcp_data * data);
 
 int          ipcp_data_add_reg_entry(struct ipcp_data * data,
-                                     const char *       name);
+                                     char *             name);
 int          ipcp_data_del_reg_entry(struct ipcp_data * data,
                                      const char *       name);
 int          ipcp_data_add_dir_entry(struct ipcp_data * data,
-                                     const char *       ap_name,
+                                     char *             ap_name,
                                      uint64_t           addr);
 int          ipcp_data_del_dir_entry(struct ipcp_data * data,
                                      const char *       ap_name,
diff --git a/src/ipcpd/ipcp.c b/src/ipcpd/ipcp.c
index 8fed60eb..6b76f20e 100644
--- a/src/ipcpd/ipcp.c
+++ b/src/ipcpd/ipcp.c
@@ -91,6 +91,11 @@ static void close_ptr(void * o)
         close(*((int *) o));
 }
 
+static void clean_msg (void * msg)
+{
+         ipcp_msg__free_unpacked(msg, NULL);
+}
+
 void * ipcp_main_loop(void * o)
 {
         int     lsockfd;
@@ -123,11 +128,10 @@ void * ipcp_main_loop(void * o)
                 return (void *) 1;
         }
 
-        pthread_cleanup_push(close_ptr,
-                             (void *) &sockfd);
-
         free(sock_path);
 
+        pthread_cleanup_push(close_ptr, (void *) &sockfd);
+
         while (true) {
                 ret_msg.code = IPCP_MSG_CODE__IPCP_REPLY;
 
@@ -150,6 +154,8 @@ void * ipcp_main_loop(void * o)
                         continue;
                 }
 
+                pthread_cleanup_push(clean_msg, (void *) msg);
+
                 switch (msg->code) {
                 case IPCP_MSG_CODE__IPCP_BOOTSTRAP:
                         if (_ipcp->ops->ipcp_bootstrap == NULL) {
@@ -175,9 +181,8 @@ void * ipcp_main_loop(void * o)
                                 conf.dns_addr = conf_msg->dns_addr;
                         }
 
-                        if (conf_msg->ipcp_type == IPCP_SHIM_ETH_LLC) {
+                        if (conf_msg->ipcp_type == IPCP_SHIM_ETH_LLC)
                                 conf.if_name = conf_msg->if_name;
-                        }
 
                         ret_msg.has_result = true;
                         ret_msg.result = _ipcp->ops->ipcp_bootstrap(&conf);
@@ -198,7 +203,8 @@ void * ipcp_main_loop(void * o)
                                 break;
                         }
                         ret_msg.has_result = true;
-                        ret_msg.result = _ipcp->ops->ipcp_name_reg(msg->name);
+                        ret_msg.result =
+                                _ipcp->ops->ipcp_name_reg(strdup(msg->name));
                         break;
                 case IPCP_MSG_CODE__IPCP_NAME_UNREG:
                         if (_ipcp->ops->ipcp_name_unreg == NULL) {
@@ -206,7 +212,8 @@ void * ipcp_main_loop(void * o)
                                 break;
                         }
                         ret_msg.has_result = true;
-                        ret_msg.result = _ipcp->ops->ipcp_name_unreg(msg->name);
+                        ret_msg.result =
+                                _ipcp->ops->ipcp_name_unreg(msg->name);
                         break;
                 case IPCP_MSG_CODE__IPCP_FLOW_ALLOC:
                         if (_ipcp->ops->ipcp_flow_alloc == NULL) {
@@ -246,7 +253,8 @@ void * ipcp_main_loop(void * o)
                         break;
                 }
 
-                ipcp_msg__free_unpacked(msg, NULL);
+                pthread_cleanup_pop(true);
+
 
                 buffer.len = ipcp_msg__get_packed_size(&ret_msg);
                 if (buffer.len == 0) {
@@ -270,10 +278,10 @@ void * ipcp_main_loop(void * o)
                 }
 
                 free(buffer.data);
-                close(lsockfd);
+
         }
 
-        pthread_cleanup_pop(false);
+        pthread_cleanup_pop(true);
 
         return NULL;
 }
diff --git a/src/ipcpd/local/main.c b/src/ipcpd/local/main.c
index 4b9dcbbc..e77a0403 100644
--- a/src/ipcpd/local/main.c
+++ b/src/ipcpd/local/main.c
@@ -292,10 +292,6 @@ static int ipcp_local_name_reg(char * name)
                 return -1; /* -ENOTENROLLED */
         }
 
-        pthread_rwlock_unlock(&_ipcp->state_lock);
-
-        pthread_rwlock_rdlock(&_ipcp->state_lock);
-
         if (ipcp_data_add_reg_entry(_ipcp->data, name)) {
                 pthread_rwlock_unlock(&_ipcp->state_lock);
                 LOG_DBGF("Failed to add %s to local registry.", name);
@@ -627,7 +623,8 @@ int main(int argc, char * argv[])
 
         shim_ap_fini();
 
-        free(_ipcp->data);
+        ipcp_data_destroy(_ipcp->data);
+
         free(_ipcp->ops);
         free(_ipcp);
 
diff --git a/src/ipcpd/shim-eth-llc/main.c b/src/ipcpd/shim-eth-llc/main.c
index f98799a5..3b70b955 100644
--- a/src/ipcpd/shim-eth-llc/main.c
+++ b/src/ipcpd/shim-eth-llc/main.c
@@ -218,7 +218,7 @@ void eth_llc_ipcp_data_destroy()
         pthread_rwlock_unlock(&shim_data(_ipcp)->flows_lock);
         pthread_rwlock_unlock(&_ipcp->state_lock);
 
-        free(_ipcp->data);
+        ipcp_data_destroy(_ipcp->data);
 }
 
 /* only call this under flows_lock */
diff --git a/src/ipcpd/shim-udp/main.c b/src/ipcpd/shim-udp/main.c
index 1b0bec07..49fd7772 100644
--- a/src/ipcpd/shim-udp/main.c
+++ b/src/ipcpd/shim-udp/main.c
@@ -1611,7 +1611,7 @@ int main(int argc, char * argv[])
 
         shim_ap_fini();
 
-        free(_ipcp->data);
+        ipcp_data_destroy(_ipcp->data);
         free(_ipcp->ops);
         free(_ipcp);