From c9cffcf863b23e75ccb6d7800ac0d48fd1612259 Mon Sep 17 00:00:00 2001 From: dimitri staessens Date: Thu, 4 Aug 2016 11:24:24 +0200 Subject: ipcpd: Fix incorrect cleanup of fd The cleanup was trying to close a pointer to the file descriptor. --- src/ipcpd/ipcp.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'src/ipcpd/ipcp.c') diff --git a/src/ipcpd/ipcp.c b/src/ipcpd/ipcp.c index fb31cf1b..8fed60eb 100644 --- a/src/ipcpd/ipcp.c +++ b/src/ipcpd/ipcp.c @@ -86,6 +86,11 @@ int ipcp_parse_arg(int argc, char * argv[]) return 0; } +static void close_ptr(void * o) +{ + close(*((int *) o)); +} + void * ipcp_main_loop(void * o) { int lsockfd; @@ -118,7 +123,7 @@ void * ipcp_main_loop(void * o) return (void *) 1; } - pthread_cleanup_push((void(*)(void *)) close, + pthread_cleanup_push(close_ptr, (void *) &sockfd); free(sock_path); @@ -268,7 +273,7 @@ void * ipcp_main_loop(void * o) close(lsockfd); } - pthread_cleanup_pop(0); + pthread_cleanup_pop(false); return NULL; } -- cgit v1.2.3 From 139001b60b32e756e947d6e3a55767be9063029d Mon Sep 17 00:00:00 2001 From: dimitri staessens Date: Thu, 4 Aug 2016 12:53:28 +0200 Subject: ipcpd: Fix memory leaks ipcp-data was not correctly destroyed. --- src/ipcpd/ipcp-data.c | 56 ++++++++++++++++++++++++------------------- src/ipcpd/ipcp-data.h | 4 ++-- src/ipcpd/ipcp.c | 28 ++++++++++++++-------- src/ipcpd/local/main.c | 7 ++---- src/ipcpd/shim-eth-llc/main.c | 2 +- src/ipcpd/shim-udp/main.c | 2 +- src/irmd/main.c | 2 +- 7 files changed, 56 insertions(+), 45 deletions(-) (limited to 'src/ipcpd/ipcp.c') 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); diff --git a/src/irmd/main.c b/src/irmd/main.c index 1c731788..b3228789 100644 --- a/src/irmd/main.c +++ b/src/irmd/main.c @@ -1464,7 +1464,7 @@ void * irm_flow_cleaner() } } -void clean_msg(void * msg) +static void clean_msg(void * msg) { irm_msg__free_unpacked(msg, NULL); } -- cgit v1.2.3 From 2567329483ab1fe5384782da50e06aa0bbdd3cfe Mon Sep 17 00:00:00 2001 From: dimitri staessens Date: Thu, 4 Aug 2016 15:25:42 +0200 Subject: irmd: Fix destroying allocated flows When a flow was in FLOW_ALLOCATED state, it would not change to NULL state and irm_flow_destroy would hang forever. --- src/ipcpd/ipcp.c | 2 +- src/irmd/main.c | 23 ++++++++++++++++------- 2 files changed, 17 insertions(+), 8 deletions(-) (limited to 'src/ipcpd/ipcp.c') diff --git a/src/ipcpd/ipcp.c b/src/ipcpd/ipcp.c index 6b76f20e..9ecc411d 100644 --- a/src/ipcpd/ipcp.c +++ b/src/ipcpd/ipcp.c @@ -91,7 +91,7 @@ static void close_ptr(void * o) close(*((int *) o)); } -static void clean_msg (void * msg) +static void clean_msg(void * msg) { ipcp_msg__free_unpacked(msg, NULL); } diff --git a/src/irmd/main.c b/src/irmd/main.c index b3228789..8c19990a 100644 --- a/src/irmd/main.c +++ b/src/irmd/main.c @@ -151,11 +151,13 @@ static void irm_flow_destroy(struct irm_flow * e) if (e->state == FLOW_PENDING) e->state = FLOW_DESTROY; + else + e->state = FLOW_NULL; pthread_cond_signal(&e->state_cond); pthread_mutex_unlock(&e->state_lock); - pthread_cleanup_push((void(*)(void *)) pthread_mutex_unlock, + pthread_cleanup_push((void (*)(void *)) pthread_mutex_unlock, (void *) &e->state_lock); while (e->state != FLOW_NULL) @@ -942,6 +944,14 @@ static struct irm_flow * flow_alloc(pid_t api, return pme; } +static void cleanup_alloc_res(void * o) +{ + struct irm_flow * e = (struct irm_flow *) o; + if (e->state == FLOW_PENDING) + e->state = FLOW_NULL; + pthread_mutex_unlock(&e->state_lock); +} + static int flow_alloc_res(int port_id) { struct irm_flow * e; @@ -979,8 +989,7 @@ static int flow_alloc_res(int port_id) pthread_rwlock_unlock(&irmd->state_lock); pthread_mutex_lock(&e->state_lock); - pthread_cleanup_push((void(*)(void *))pthread_mutex_unlock, - (void*) &e->state_lock); + pthread_cleanup_push(cleanup_alloc_res, (void *) e); while (e->state == FLOW_PENDING) pthread_cond_wait(&e->state_cond, &e->state_lock); @@ -1035,7 +1044,7 @@ static int flow_dealloc(int port_id) pthread_rwlock_unlock(&irmd->state_lock); - free(e); + irm_flow_destroy(e); return ret; } @@ -1147,7 +1156,7 @@ static struct irm_flow * flow_req_arr(pid_t api, pthread_rwlock_unlock(&irmd->reg_lock); pthread_mutex_lock(&rne->state_lock); - pthread_cleanup_push((void(*)(void *)) pthread_mutex_unlock, + pthread_cleanup_push((void (*)(void *)) pthread_mutex_unlock, (void *) &rne->state_lock); while (rne->state == REG_NAME_AUTO_EXEC) @@ -1203,7 +1212,7 @@ static struct irm_flow * flow_req_arr(pid_t api, pthread_rwlock_unlock(&irmd->state_lock); - pthread_cleanup_push((void(*)(void *)) pthread_mutex_unlock, + pthread_cleanup_push((void (*)(void *)) pthread_mutex_unlock, (void *) &rne->state_lock); while (rne->state == REG_NAME_FLOW_ARRIVED && @@ -1268,7 +1277,7 @@ static int flow_dealloc_ipcp(int port_id) pthread_rwlock_unlock(&irmd->flows_lock); pthread_rwlock_unlock(&irmd->state_lock); - free(e); + irm_flow_destroy(e); return 0; } -- cgit v1.2.3