From: Arran Cudbard-Bell Date: Tue, 3 Apr 2018 23:42:10 +0000 (+0100) Subject: Don't leak worker heaps X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=5926fbef05852c2db7c6291fbfccd2099c3d3638;p=thirdparty%2Ffreeradius-server.git Don't leak worker heaps --- diff --git a/src/include/heap.h b/src/include/heap.h index eca2bf95a5d..3ccde24bd51 100644 --- a/src/include/heap.h +++ b/src/include/heap.h @@ -38,15 +38,17 @@ typedef struct fr_heap_t fr_heap_t; /** Creates a heap that can be used with non-talloced elements * + * @param[in] _ctx Talloc ctx to allocate heap in. * @param[in] _cmp Comparator used to compare elements. * @param[in] _type Of elements. * @param[in] _field to store heap indexes in. */ -#define fr_heap_create(_cmp, _type, _field) \ - _fr_heap_create(_cmp, NULL, (size_t)offsetof(_type, _field)) +#define fr_heap_create(_ctx, _cmp, _type, _field) \ + _fr_heap_create(_ctx, _cmp, NULL, (size_t)offsetof(_type, _field)) /** Creates a heap that verifies elements are of a specific talloc type * + * @param[in] _ctx Talloc ctx to allocate heap in. * @param[in] _cmp Comparator used to compare elements. * @param[in] _talloc_type of elements. * @param[in] _field to store heap indexes in. @@ -54,10 +56,10 @@ typedef struct fr_heap_t fr_heap_t; * - A new heap. * - NULL on error. */ -#define fr_heap_talloc_create(_cmp, _talloc_type, _field) \ - _fr_heap_create(_cmp, #_talloc_type, (size_t)offsetof(_talloc_type, _field)) +#define fr_heap_talloc_create(_ctx, _cmp, _talloc_type, _field) \ + _fr_heap_create(_ctx, _cmp, #_talloc_type, (size_t)offsetof(_talloc_type, _field)) -fr_heap_t *_fr_heap_create(fr_heap_cmp_t cmp, char const *talloc_type, size_t offset); +fr_heap_t *_fr_heap_create(TALLOC_CTX *ctx, fr_heap_cmp_t cmp, char const *talloc_type, size_t offset); int fr_heap_insert(fr_heap_t *hp, void *data); int fr_heap_extract(fr_heap_t *hp, void *data); diff --git a/src/lib/io/network.c b/src/lib/io/network.c index 2c726c65d4c..1639fd300ba 100644 --- a/src/lib/io/network.c +++ b/src/lib/io/network.c @@ -699,7 +699,7 @@ static void fr_network_socket_callback(void *ctx, void const *data, size_t data_ rad_assert(s != NULL); memcpy(&s->listen, data, sizeof(s->listen)); - MEM(s->waiting = fr_heap_create(waiting_cmp, fr_channel_data_t, channel.heap_id)); + MEM(s->waiting = fr_heap_create(s, waiting_cmp, fr_channel_data_t, channel.heap_id)); FR_DLIST_INIT(s->entry); talloc_set_destructor(s, _network_socket_free); @@ -773,7 +773,7 @@ static void fr_network_directory_callback(void *ctx, void const *data, size_t da rad_assert(s != NULL); memcpy(&s->listen, data, sizeof(s->listen)); - MEM(s->waiting = fr_heap_create(waiting_cmp, fr_channel_data_t, channel.heap_id)); + MEM(s->waiting = fr_heap_create(s, waiting_cmp, fr_channel_data_t, channel.heap_id)); FR_DLIST_INIT(s->entry); talloc_set_destructor(s, _network_socket_free); @@ -1017,7 +1017,7 @@ fr_network_t *fr_network_create(TALLOC_CTX *ctx, fr_event_list_t *el, fr_log_t c goto fail2; } - nr->replies = fr_heap_create(reply_cmp, fr_channel_data_t, channel.heap_id); + nr->replies = fr_heap_create(nr, reply_cmp, fr_channel_data_t, channel.heap_id); if (!nr->replies) { fr_strerror_printf_push("Failed creating heap for replies"); goto fail2; diff --git a/src/lib/io/worker.c b/src/lib/io/worker.c index 545fd958530..08e7ef39f6a 100644 --- a/src/lib/io/worker.c +++ b/src/lib/io/worker.c @@ -163,7 +163,7 @@ static void fr_worker_post_event(fr_event_list_t *el, struct timeval *now, void */ #define WORKER_HEAP_INIT(_name, _func, _type, _member) do { \ FR_DLIST_INIT(worker->_name.list); \ - worker->_name.heap = fr_heap_create(_func, _type, _member); \ + worker->_name.heap = fr_heap_create(worker, _func, _type, _member); \ if (!worker->_name.heap) { \ (void) fr_event_user_delete(worker->el, fr_worker_evfilt_user, worker); \ talloc_free(worker); \ @@ -1217,10 +1217,7 @@ void fr_worker_destroy(fr_worker_t *worker) worker_stop_request(worker, request, now); talloc_free(request); } - talloc_free(worker->time_order); - rad_assert(fr_heap_num_elements(worker->runnable) == 0); - talloc_free(worker->runnable); #if 0 /* @@ -1336,13 +1333,13 @@ nomem: WORKER_HEAP_INIT(to_decode, worker_message_cmp, fr_channel_data_t, channel.heap_id); WORKER_HEAP_INIT(localized, worker_message_cmp, fr_channel_data_t, channel.heap_id); - worker->runnable = fr_heap_talloc_create(worker_runnable_cmp, REQUEST, runnable_id); + worker->runnable = fr_heap_talloc_create(worker, worker_runnable_cmp, REQUEST, runnable_id); if (!worker->runnable) { fr_strerror_printf("Failed creating runnable heap"); goto fail; } - worker->time_order = fr_heap_talloc_create(worker_time_order_cmp, REQUEST, time_order_id); + worker->time_order = fr_heap_talloc_create(worker, worker_time_order_cmp, REQUEST, time_order_id); if (!worker->time_order) { fr_strerror_printf("Failed creating time_order heap"); goto fail; diff --git a/src/lib/util/event.c b/src/lib/util/event.c index 6b81ec99301..0f4eaa71854 100644 --- a/src/lib/util/event.c +++ b/src/lib/util/event.c @@ -1786,8 +1786,6 @@ static int _event_list_free(fr_event_list_t *el) while ((ev = fr_heap_peek(el->times)) != NULL) fr_event_timer_delete(el, &ev); - talloc_free(el->times); - talloc_free_children(el); if (el->kq >= 0) close(el->kq); @@ -1817,7 +1815,7 @@ fr_event_list_t *fr_event_list_alloc(TALLOC_CTX *ctx, fr_event_status_cb_t statu el->kq = -1; /* So destructor can be used before kqueue() provides us with fd */ talloc_set_destructor(el, _event_list_free); - el->times = fr_heap_talloc_create(fr_event_timer_cmp, fr_event_timer_t, heap_id); + el->times = fr_heap_talloc_create(el, fr_event_timer_cmp, fr_event_timer_t, heap_id); if (!el->times) { fr_strerror_printf("Failed allocating event heap"); error: diff --git a/src/lib/util/heap.c b/src/lib/util/heap.c index da0dbe41b4f..0674e1794c2 100644 --- a/src/lib/util/heap.c +++ b/src/lib/util/heap.c @@ -58,13 +58,13 @@ struct fr_heap_t { static void fr_heap_bubble(fr_heap_t *hp, int32_t child); -fr_heap_t *_fr_heap_create(fr_heap_cmp_t cmp, char const *type, size_t offset) +fr_heap_t *_fr_heap_create(TALLOC_CTX *ctx, fr_heap_cmp_t cmp, char const *type, size_t offset) { fr_heap_t *fh; if (!cmp) return NULL; - fh = talloc_zero(NULL, fr_heap_t); + fh = talloc_zero(ctx, fr_heap_t); if (!fh) return NULL; fh->size = 2048; @@ -332,7 +332,7 @@ int main(int argc, char **argv) skip = atoi(argv[1]); } - hp = fr_heap_create(heap_cmp, offsetof(heap_thing, heap_id)); + hp = fr_heap_create(NULL, heap_cmp, offsetof(heap_thing, heap_id)); if (!hp) { fprintf(stderr, "Failed creating heap!\n"); fr_exit(1); diff --git a/src/main/module.c b/src/main/module.c index 30136d32f56..8cf1f69b029 100644 --- a/src/main/module.c +++ b/src/main/module.c @@ -492,9 +492,8 @@ static void _module_thread_instance_free(void *to_free) { module_thread_instance_t *ti = talloc_get_type_abort(to_free, module_thread_instance_t); - if (ti->module->thread_detach) { - (void) ti->module->thread_detach(ti->el, ti->data); - } + DEBUG3("Worker cleaning up %s thread instance data (%p/%p)", ti->module->name, ti, ti->data); + if (ti->module->thread_detach) (void) ti->module->thread_detach(ti->el, ti->data); talloc_free(ti); } @@ -507,6 +506,8 @@ static void _module_thread_inst_tree_free(void *to_free) { rbtree_t *thread_inst_tree = talloc_get_type_abort(to_free , rbtree_t); + DEBUG3("Worker cleaning up thread instance tree"); + talloc_free(thread_inst_tree); } @@ -563,20 +564,18 @@ static int _module_thread_instantiate(void *instance, void *ctx) MEM(type_name = talloc_typed_asprintf(NULL, "rlm_%s_thread_t", mi->module->name)); talloc_set_name(ti->data, "%s", type_name); talloc_free(type_name); - } + DEBUG3("Worker alloced %s thread instance data (%p/%p)", ti->module->name, ti, ti->data); if (mi->module->thread_instantiate) { ret = mi->module->thread_instantiate(mi->dl_inst->conf, mi->dl_inst->data, - thread_inst_ctx->el, ti->data); + thread_inst_ctx->el, ti->data); if (ret < 0) { - ERROR("Thread instantiation failed for module \"%s\"", - mi->name); + ERROR("Thread instantiation failed for module \"%s\"", mi->name); return -1; } } - rbtree_insert(thread_inst_ctx->tree, ti); return 0; diff --git a/src/main/pool.c b/src/main/pool.c index b6899b83e2d..5f5b9ea89c4 100644 --- a/src/main/pool.c +++ b/src/main/pool.c @@ -1000,7 +1000,7 @@ fr_pool_t *fr_pool_init(TALLOC_CTX *ctx, * https://code.facebook.com/posts/1499322996995183/solving-the-mystery-of-link-imbalance-a-metastable-failure-state-at-scale/ */ if (!pool->spread) { - pool->heap = fr_heap_talloc_create(last_reserved_cmp, fr_pool_connection_t, heap_id); + pool->heap = fr_heap_talloc_create(pool, last_reserved_cmp, fr_pool_connection_t, heap_id); /* * For some types of connections we need to used a different * algorithm, because load balancing benefits are secondary @@ -1020,7 +1020,7 @@ fr_pool_t *fr_pool_init(TALLOC_CTX *ctx, * That way we maximise time between connection use. */ } else { - pool->heap = fr_heap_talloc_create(last_released_cmp, fr_pool_connection_t, heap_id); + pool->heap = fr_heap_talloc_create(pool, last_released_cmp, fr_pool_connection_t, heap_id); } if (!pool->heap) { ERROR("%s: Failed creating connection heap", __FUNCTION__); @@ -1323,8 +1323,6 @@ void fr_pool_free(fr_pool_t *pool) connection_close_internal(pool, NULL, this); } - talloc_free(pool->heap); - fr_pool_trigger_exec(pool, NULL, "stop"); rad_assert(pool->head == NULL); diff --git a/src/modules/rlm_cache/drivers/rlm_cache_rbtree/rlm_cache_rbtree.c b/src/modules/rlm_cache/drivers/rlm_cache_rbtree/rlm_cache_rbtree.c index 845dc7e34ce..56f0dd38232 100644 --- a/src/modules/rlm_cache/drivers/rlm_cache_rbtree/rlm_cache_rbtree.c +++ b/src/modules/rlm_cache/drivers/rlm_cache_rbtree/rlm_cache_rbtree.c @@ -86,7 +86,6 @@ static int mod_detach(void *instance) { rlm_cache_rbtree_t *driver = talloc_get_type_abort(instance, rlm_cache_rbtree_t); - if (driver->heap) talloc_free(driver->heap); if (driver->cache) { rbtree_walk(driver->cache, RBTREE_DELETE_ORDER, _cache_entry_free, NULL); talloc_free(driver->cache); @@ -118,7 +117,7 @@ static int mod_instantiate(UNUSED rlm_cache_config_t const *config, void *instan /* * The heap of entries to expire. */ - driver->heap = fr_heap_talloc_create(cache_heap_cmp, rlm_cache_rbtree_entry_t, heap_id); + driver->heap = fr_heap_talloc_create(driver, cache_heap_cmp, rlm_cache_rbtree_entry_t, heap_id); if (!driver->heap) { ERROR("Failed to create heap for the cache"); return -1; diff --git a/src/modules/rlm_radius/rlm_radius_udp.c b/src/modules/rlm_radius/rlm_radius_udp.c index 9b8c7261900..d7cc0357816 100644 --- a/src/modules/rlm_radius/rlm_radius_udp.c +++ b/src/modules/rlm_radius/rlm_radius_udp.c @@ -2594,13 +2594,13 @@ static int mod_thread_instantiate(UNUSED CONF_SECTION const *cs, void *instance, t->inst = instance; t->el = el; - t->queued = fr_heap_talloc_create(queue_cmp, rlm_radius_udp_request_t, heap_id); + t->queued = fr_heap_talloc_create(t, queue_cmp, rlm_radius_udp_request_t, heap_id); FR_DLIST_INIT(t->blocked); FR_DLIST_INIT(t->full); FR_DLIST_INIT(t->zombie); FR_DLIST_INIT(t->opening); - t->active = fr_heap_talloc_create(conn_cmp, rlm_radius_udp_connection_t, heap_id); + t->active = fr_heap_talloc_create(t, conn_cmp, rlm_radius_udp_connection_t, heap_id); conn_alloc(t->inst, t); @@ -2624,8 +2624,6 @@ static int mod_thread_detach(UNUSED fr_event_list_t *el, void *thread) * Free all of the heaps, lists, and sockets. */ talloc_free_children(t); - talloc_free(t->queued); - talloc_free(t->active); entry = FR_DLIST_FIRST(t->opening); if (entry != NULL) { diff --git a/src/unlang/interpret.c b/src/unlang/interpret.c index bfc9fc9b5bd..6798c558d23 100644 --- a/src/unlang/interpret.c +++ b/src/unlang/interpret.c @@ -773,7 +773,7 @@ rlm_rcode_t unlang_interpret_synchronous(REQUEST *request, CONF_SECTION *cs, rlm return RLM_MODULE_FAIL; } - MEM(backlog = fr_heap_talloc_create(_unlang_request_ptr_cmp, REQUEST, runnable_id)); + MEM(backlog = fr_heap_talloc_create(el, _unlang_request_ptr_cmp, REQUEST, runnable_id)); old_el = request->el; old_backlog = request->backlog; caller = request->module; @@ -806,7 +806,6 @@ rlm_rcode_t unlang_interpret_synchronous(REQUEST *request, CONF_SECTION *cs, rlm talloc_free(el); request->el = old_el; - talloc_free(backlog); request->backlog = old_backlog; request->module = caller;