From 46c15ea8fa98bb3d92580b192f03863c2e2a2d9c Mon Sep 17 00:00:00 2001 From: Mohit Agrawal Date: Thu, 29 Nov 2018 19:55:39 +0530 Subject: server: Resolve memory leak path in server_init Problem: 1) server_init does not cleanup allocate resources while it is failed before return error 2) dict leak at the time of graph destroying Solution: 1) free resources in case of server_init is failed 2) Take dict_ref of graph xlator before destroying the graph to avoid leak Change-Id: I9e31e156b9ed6bebe622745a8be0e470774e3d15 fixes: bz#1654917 Signed-off-by: Mohit Agrawal --- glusterfsd/src/glusterfsd.c | 4 +++ libglusterfs/src/libglusterfs.sym | 1 + libglusterfs/src/xlator.c | 31 +++++++++++++++++ libglusterfs/src/xlator.h | 2 ++ rpc/rpc-lib/src/libgfrpc.sym | 1 + rpc/rpc-lib/src/rpc-transport.c | 28 +++++++++++----- rpc/rpc-lib/src/rpc-transport.h | 3 ++ rpc/rpc-lib/src/rpcsvc.c | 40 ++++++++++++++++++++++ rpc/rpc-lib/src/rpcsvc.h | 3 ++ xlators/mgmt/glusterd/src/glusterd-utils.c | 33 ++---------------- xlators/protocol/server/src/server.c | 54 +++++++++++++++++++++++++----- 11 files changed, 151 insertions(+), 49 deletions(-) diff --git a/glusterfsd/src/glusterfsd.c b/glusterfsd/src/glusterfsd.c index 771af423ea5..5bf0db0edfb 100644 --- a/glusterfsd/src/glusterfsd.c +++ b/glusterfsd/src/glusterfsd.c @@ -2618,6 +2618,10 @@ out: xl = graph->first; if ((ctx->active != graph) && (xl && !strcmp(xl->type, "protocol/server"))) { + /* Take dict ref for every graph xlator to avoid dict leak + at the time of graph destroying + */ + gluster_graph_take_reference(graph->first); glusterfs_graph_fini(graph); glusterfs_graph_destroy(graph); } diff --git a/libglusterfs/src/libglusterfs.sym b/libglusterfs/src/libglusterfs.sym index c8c42311c80..baf44de64ad 100644 --- a/libglusterfs/src/libglusterfs.sym +++ b/libglusterfs/src/libglusterfs.sym @@ -1121,6 +1121,7 @@ xlator_volume_option_get xlator_volume_option_get_list xlator_memrec_free xlator_mem_cleanup +gluster_graph_take_reference default_fops gf_fop_list gf_upcall_list diff --git a/libglusterfs/src/xlator.c b/libglusterfs/src/xlator.c index cbbe8cf7f12..dedce05d5c3 100644 --- a/libglusterfs/src/xlator.c +++ b/libglusterfs/src/xlator.c @@ -1555,3 +1555,34 @@ glusterfs_delete_volfile_checksum(glusterfs_ctx_t *ctx, const char *volfile_id) return 0; } + +/* + The function is required to take dict ref for every xlator at graph. + At the time of compare graph topology create a graph and populate + key values in the dictionary, after finished graph comparison we do destroy + the new graph.At the time of construct graph we don't take any reference + so to avoid dict leak at the of destroying graph due to ref counter underflow + we need to call dict_ref here. + +*/ + +void +gluster_graph_take_reference(xlator_t *tree) +{ + xlator_t *trav = tree; + xlator_t *prev = tree; + + if (!tree) { + gf_msg("parser", GF_LOG_ERROR, 0, LG_MSG_TREE_NOT_FOUND, + "Translator tree not found"); + return; + } + + while (prev) { + trav = prev->next; + if (prev->options) + dict_ref(prev->options); + prev = trav; + } + return; +} diff --git a/libglusterfs/src/xlator.h b/libglusterfs/src/xlator.h index 64a61ac0bed..30c9e5875e5 100644 --- a/libglusterfs/src/xlator.h +++ b/libglusterfs/src/xlator.h @@ -1082,4 +1082,6 @@ xlator_mem_cleanup(xlator_t *this); void handle_default_options(xlator_t *xl, dict_t *options); +void +gluster_graph_take_reference(xlator_t *tree); #endif /* _XLATOR_H */ diff --git a/rpc/rpc-lib/src/libgfrpc.sym b/rpc/rpc-lib/src/libgfrpc.sym index 4f42485044f..f3544e3741a 100644 --- a/rpc/rpc-lib/src/libgfrpc.sym +++ b/rpc/rpc-lib/src/libgfrpc.sym @@ -26,6 +26,7 @@ rpcsvc_drc_priv rpcsvc_drc_reconfigure rpcsvc_get_program_vector_sizer rpcsvc_init +rpcsvc_destroy rpcsvc_init_options rpcsvc_listener_destroy rpcsvc_program_register diff --git a/rpc/rpc-lib/src/rpc-transport.c b/rpc/rpc-lib/src/rpc-transport.c index 54636dcbf00..8bb6b595175 100644 --- a/rpc/rpc-lib/src/rpc-transport.c +++ b/rpc/rpc-lib/src/rpc-transport.c @@ -159,6 +159,24 @@ out: return msg; } +void +rpc_transport_cleanup(rpc_transport_t *trans) +{ + if (!trans) + return; + + trans->fini(trans); + GF_FREE(trans->name); + + if (trans->xl) + pthread_mutex_destroy(&trans->lock); + + if (trans->dl_handle) + dlclose(trans->dl_handle); + + GF_FREE(trans); +} + rpc_transport_t * rpc_transport_load(glusterfs_ctx_t *ctx, dict_t *options, char *trans_name) { @@ -354,15 +372,7 @@ rpc_transport_load(glusterfs_ctx_t *ctx, dict_t *options, char *trans_name) fail: if (!success) { - if (trans) { - GF_FREE(trans->name); - - if (trans->dl_handle) - dlclose(trans->dl_handle); - - GF_FREE(trans); - } - + rpc_transport_cleanup(trans); GF_FREE(name); return_trans = NULL; diff --git a/rpc/rpc-lib/src/rpc-transport.h b/rpc/rpc-lib/src/rpc-transport.h index 87ea3a28dfd..d7b86b63748 100644 --- a/rpc/rpc-lib/src/rpc-transport.h +++ b/rpc/rpc-lib/src/rpc-transport.h @@ -308,4 +308,7 @@ rpc_transport_unix_options_build(dict_t **options, char *filepath, int rpc_transport_inet_options_build(dict_t **options, const char *hostname, int port); + +void +rpc_transport_cleanup(rpc_transport_t *); #endif /* __RPC_TRANSPORT_H__ */ diff --git a/rpc/rpc-lib/src/rpcsvc.c b/rpc/rpc-lib/src/rpcsvc.c index 8dcc2947b33..fd531fbc1ee 100644 --- a/rpc/rpc-lib/src/rpcsvc.c +++ b/rpc/rpc-lib/src/rpcsvc.c @@ -37,6 +37,7 @@ #include #include #include +#include #ifdef IPV6_DEFAULT #include @@ -2009,6 +2010,7 @@ rpcsvc_create_listener(rpcsvc_t *svc, dict_t *options, char *name) listener = rpcsvc_listener_alloc(svc, trans); if (listener == NULL) { + ret = -1; goto out; } @@ -2016,6 +2018,7 @@ rpcsvc_create_listener(rpcsvc_t *svc, dict_t *options, char *name) out: if (!listener && trans) { rpc_transport_disconnect(trans, _gf_true); + rpc_transport_cleanup(trans); } return ret; @@ -2747,6 +2750,43 @@ rpcsvc_get_throttle(rpcsvc_t *svc) return svc->throttle; } +/* Function call to cleanup resources for svc + */ +int +rpcsvc_destroy(rpcsvc_t *svc) +{ + struct rpcsvc_auth_list *auth = NULL; + struct rpcsvc_auth_list *tmp = NULL; + rpcsvc_listener_t *listener = NULL; + rpcsvc_listener_t *next = NULL; + int ret = 0; + + if (!svc) + return ret; + + list_for_each_entry_safe(listener, next, &svc->listeners, list) + { + rpcsvc_listener_destroy(listener); + } + + list_for_each_entry_safe(auth, tmp, &svc->authschemes, authlist) + { + list_del_init(&auth->authlist); + GF_FREE(auth); + } + + rpcsvc_program_unregister(svc, &gluster_dump_prog); + if (svc->rxpool) { + mem_pool_destroy(svc->rxpool); + svc->rxpool = NULL; + } + + pthread_rwlock_destroy(&svc->rpclock); + GF_FREE(svc); + + return ret; +} + /* The global RPC service initializer. */ rpcsvc_t * diff --git a/rpc/rpc-lib/src/rpcsvc.h b/rpc/rpc-lib/src/rpcsvc.h index d6260ca5028..b296f9a4bde 100644 --- a/rpc/rpc-lib/src/rpcsvc.h +++ b/rpc/rpc-lib/src/rpcsvc.h @@ -677,4 +677,7 @@ rpcsvc_get_program_vector_sizer(rpcsvc_t *svc, uint32_t prognum, uint32_t progver, int procnum); void rpcsvc_autoscale_threads(glusterfs_ctx_t *ctx, rpcsvc_t *rpc, int incr); + +extern int +rpcsvc_destroy(rpcsvc_t *svc); #endif diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c b/xlators/mgmt/glusterd/src/glusterd-utils.c index 2deda2945c0..cdac6d5b8bf 100644 --- a/xlators/mgmt/glusterd/src/glusterd-utils.c +++ b/xlators/mgmt/glusterd/src/glusterd-utils.c @@ -9301,35 +9301,6 @@ glusterd_defrag_volume_status_update(glusterd_volinfo_t *volinfo, return ret; } -/* - The function is required to take dict ref for every xlator at graph. - At the time of compare graph topology create a graph and populate - key values in the dictionary, after finished graph comparison we do destroy - the new graph.At the time of construct graph we don't take any reference - so to avoid leak due to ref counter underflow we need to call dict_ref here. - -*/ - -void -glusterd_graph_take_reference(xlator_t *tree) -{ - xlator_t *trav = tree; - xlator_t *prev = tree; - - if (!tree) { - gf_msg("parser", GF_LOG_ERROR, 0, LG_MSG_TREE_NOT_FOUND, - "Translator tree not found"); - return; - } - - while (prev) { - trav = prev->next; - if (prev->options) - dict_ref(prev->options); - prev = trav; - } - return; -} int glusterd_check_topology_identical(const char *filename1, const char *filename2, @@ -9376,14 +9347,14 @@ glusterd_check_topology_identical(const char *filename1, const char *filename2, if (grph1 == NULL) goto out; - glusterd_graph_take_reference(grph1->first); + gluster_graph_take_reference(grph1->first); /* create the graph for filename2 */ grph2 = glusterfs_graph_construct(fp2); if (grph2 == NULL) goto out; - glusterd_graph_take_reference(grph2->first); + gluster_graph_take_reference(grph2->first); /* compare the graph topology */ *identical = is_graph_topology_equal(grph1, grph2); diff --git a/xlators/protocol/server/src/server.c b/xlators/protocol/server/src/server.c index 77e5d74e7c5..81aa72fbbc0 100644 --- a/xlators/protocol/server/src/server.c +++ b/xlators/protocol/server/src/server.c @@ -986,6 +986,49 @@ server_dump_metrics(xlator_t *this, int fd) return 0; } +void +server_cleanup(xlator_t *this, server_conf_t *conf) +{ + if (!this || !conf) + return; + + LOCK_DESTROY(&conf->itable_lock); + pthread_mutex_destroy(&conf->mutex); + + if (this->ctx->event_pool) { + /* Free the event pool */ + (void)event_pool_destroy(this->ctx->event_pool); + } + + if (dict_get(this->options, "config-directory")) { + GF_FREE(conf->conf_dir); + conf->conf_dir = NULL; + } + + if (conf->child_status) { + GF_FREE(conf->child_status); + conf->child_status = NULL; + } + + if (this->ctx->statedump_path) { + GF_FREE(this->ctx->statedump_path); + this->ctx->statedump_path = NULL; + } + + if (conf->auth_modules) { + gf_auth_fini(conf->auth_modules); + dict_unref(conf->auth_modules); + } + + if (conf->rpc) { + (void)rpcsvc_destroy(conf->rpc); + conf->rpc = NULL; + } + + GF_FREE(conf); + this->private = NULL; +} + int server_init(xlator_t *this) { @@ -1061,6 +1104,7 @@ server_init(xlator_t *this) ret = gf_auth_init(this, conf->auth_modules); if (ret) { dict_unref(conf->auth_modules); + conf->auth_modules = NULL; goto out; } @@ -1239,15 +1283,7 @@ out: if (this != NULL) { this->fini(this); } - - if (conf && conf->rpc) { - rpcsvc_listener_t *listener, *next; - list_for_each_entry_safe(listener, next, &conf->rpc->listeners, - list) - { - rpcsvc_listener_destroy(listener); - } - } + server_cleanup(this, conf); } return ret; -- cgit