From 77267ae548bf920a01fa56c828520d08a1a5eef4 Mon Sep 17 00:00:00 2001 From: William Brown Date: Tue, 20 Dec 2016 16:26:58 +1000 Subject: [PATCH 1/2] Ticket 49066 - Memory leaks in server - part 2 Bug Description: This resolves a number of memory leaks and code cleanups in the server. Fix Description: This fixes leaks from server shutdown especially in ldbm which was not freeing a number of mutexes correctly, and replication which was not freeing configurations and certain changelog items correctly. https://fedorahosted.org/389/ticket/49066 Author: wibrown Review by: ???? --- ldap/servers/plugins/replication/cl5_api.c | 5 ++++ .../servers/plugins/replication/repl5_mtnode_ext.c | 20 +++++++++++++--- ldap/servers/plugins/replication/repl5_replica.c | 28 ++++------------------ ldap/servers/slapd/back-ldbm/ldbm_config.c | 6 +++++ ldap/servers/slapd/backend_manager.c | 12 ++++++++++ ldap/servers/slapd/connection.c | 15 ++++++++++++ ldap/servers/slapd/pblock.c | 25 +++++++++++++++++++ 7 files changed, 85 insertions(+), 26 deletions(-) diff --git a/ldap/servers/plugins/replication/cl5_api.c b/ldap/servers/plugins/replication/cl5_api.c index 5cf95a0..1fe4500 100644 --- a/ldap/servers/plugins/replication/cl5_api.c +++ b/ldap/servers/plugins/replication/cl5_api.c @@ -433,6 +433,11 @@ void cl5Cleanup () s_cl5Desc.clLock = NULL; } + if (s_cl5Desc.clCvar != NULL) { + PR_DestroyCondVar(s_cl5Desc.clCvar); + s_cl5Desc.clCvar = NULL; + } + memset (&s_cl5Desc, 0, sizeof (s_cl5Desc)); } diff --git a/ldap/servers/plugins/replication/repl5_mtnode_ext.c b/ldap/servers/plugins/replication/repl5_mtnode_ext.c index 8d0c514..c335682 100644 --- a/ldap/servers/plugins/replication/repl5_mtnode_ext.c +++ b/ldap/servers/plugins/replication/repl5_mtnode_ext.c @@ -35,10 +35,24 @@ multimaster_mtnode_extension_init () } void +multimaster_mtnode_free_replica_object(const Slapi_DN *root) { + mapping_tree_node *mtnode; + multimaster_mtnode_extension *ext; + + mtnode = slapi_get_mapping_tree_node_by_dn(root); + ext = (multimaster_mtnode_extension *)repl_con_get_ext (REPL_CON_EXT_MTNODE, mtnode); + if (ext != NULL && ext->replica != NULL) { + object_release(ext->replica); + } +} + +void multimaster_mtnode_extension_destroy () { - dl_cleanup (root_list, (FREEFN)slapi_sdn_free); - dl_free (&root_list); + /* First iterate over the list to free the replica infos */ + dl_cleanup (root_list, (FREEFN)multimaster_mtnode_free_replica_object); + dl_cleanup (root_list, (FREEFN)slapi_sdn_free); + dl_free (&root_list); } /* This function loops over the list of node roots, constructing replica objects @@ -59,7 +73,7 @@ multimaster_mtnode_construct_replicas () if (r) { - mtnode = slapi_get_mapping_tree_node_by_dn(root); + mtnode = slapi_get_mapping_tree_node_by_dn(root); if (mtnode == NULL) { slapi_log_err(SLAPI_LOG_REPL, repl_plugin_name, diff --git a/ldap/servers/plugins/replication/repl5_replica.c b/ldap/servers/plugins/replication/repl5_replica.c index b66eb8b..04e7e8f 100644 --- a/ldap/servers/plugins/replication/repl5_replica.c +++ b/ldap/servers/plugins/replication/repl5_replica.c @@ -156,7 +156,6 @@ replica_new_from_entry (Slapi_Entry *e, char *errortext, PRBool is_add_operation { int rc = 0; Replica *r; - char *repl_name = NULL; if (e == NULL) { @@ -245,8 +244,7 @@ replica_new_from_entry (Slapi_Entry *e, char *errortext, PRBool is_add_operation /* ONREPL - the state update can occur before the entry is added to the DIT. In that case the updated would fail but nothing bad would happen. The next scheduled update would save the state */ - repl_name = slapi_ch_strdup (r->repl_name); - r->repl_eqcxt_rs = slapi_eq_repeat(replica_update_state, repl_name, + r->repl_eqcxt_rs = slapi_eq_repeat(replica_update_state, r->repl_name, current_time () + START_UPDATE_DELAY, RUV_SAVE_INTERVAL); if (r->tombstone_reap_interval > 0) @@ -255,8 +253,7 @@ replica_new_from_entry (Slapi_Entry *e, char *errortext, PRBool is_add_operation * Reap Tombstone should be started some time after the plugin started. * This will allow the server to fully start before consuming resources. */ - repl_name = slapi_ch_strdup (r->repl_name); - r->repl_eqcxt_tr = slapi_eq_repeat(eq_cb_reap_tombstones, repl_name, + r->repl_eqcxt_tr = slapi_eq_repeat(eq_cb_reap_tombstones, r->repl_name, current_time() + r->tombstone_reap_interval, 1000 * r->tombstone_reap_interval); } @@ -313,7 +310,6 @@ void replica_destroy(void **arg) { Replica *r; - void *repl_name; if (arg == NULL) return; @@ -333,16 +329,12 @@ replica_destroy(void **arg) if (r->repl_eqcxt_rs) { - repl_name = slapi_eq_get_arg (r->repl_eqcxt_rs); - slapi_ch_free (&repl_name); slapi_eq_cancel(r->repl_eqcxt_rs); r->repl_eqcxt_rs = NULL; } if (r->repl_eqcxt_tr) { - repl_name = slapi_eq_get_arg (r->repl_eqcxt_tr); - slapi_ch_free (&repl_name); slapi_eq_cancel(r->repl_eqcxt_tr); r->repl_eqcxt_tr = NULL; } @@ -1623,8 +1615,6 @@ consumer5_set_mapping_tree_state_for_replica(const Replica *r, RUV *supplierRuv) void replica_set_enabled (Replica *r, PRBool enable) { - char *repl_name = NULL; - PR_ASSERT (r); replica_lock(r->repl_lock); @@ -1633,8 +1623,7 @@ replica_set_enabled (Replica *r, PRBool enable) { if (r->repl_eqcxt_rs == NULL) /* event is not already registered */ { - repl_name = slapi_ch_strdup (r->repl_name); - r->repl_eqcxt_rs = slapi_eq_repeat(replica_update_state, repl_name, + r->repl_eqcxt_rs = slapi_eq_repeat(replica_update_state, r->repl_name, current_time() + START_UPDATE_DELAY, RUV_SAVE_INTERVAL); } } @@ -1642,8 +1631,6 @@ replica_set_enabled (Replica *r, PRBool enable) { if (r->repl_eqcxt_rs) /* event is still registerd */ { - repl_name = slapi_eq_get_arg (r->repl_eqcxt_rs); - slapi_ch_free ((void**)&repl_name); slapi_eq_cancel(r->repl_eqcxt_rs); r->repl_eqcxt_rs = NULL; } @@ -1898,7 +1885,7 @@ int replica_check_for_data_reload (Replica *r, void *arg) slapi_sdn_get_dn(r->repl_root)); rc = 0; } - } // slapi_disordely_shutdown + } /* slapi_disordely_shutdown */ object_release (ruv_obj); } @@ -3868,8 +3855,6 @@ replica_set_purge_delay(Replica *r, PRUint32 purge_delay) void replica_set_tombstone_reap_interval (Replica *r, long interval) { - char *repl_name; - replica_lock(r->repl_lock); /* @@ -3880,8 +3865,6 @@ replica_set_tombstone_reap_interval (Replica *r, long interval) { int found; - repl_name = slapi_eq_get_arg (r->repl_eqcxt_tr); - slapi_ch_free ((void**)&repl_name); found = slapi_eq_cancel (r->repl_eqcxt_tr); slapi_log_err(SLAPI_LOG_REPL, repl_plugin_name, "replica_set_tombstone_reap_interval - tombstone_reap event (interval=%ld) was %s\n", @@ -3891,8 +3874,7 @@ replica_set_tombstone_reap_interval (Replica *r, long interval) r->tombstone_reap_interval = interval; if ( interval > 0 && r->repl_eqcxt_tr == NULL ) { - repl_name = slapi_ch_strdup (r->repl_name); - r->repl_eqcxt_tr = slapi_eq_repeat (eq_cb_reap_tombstones, repl_name, + r->repl_eqcxt_tr = slapi_eq_repeat (eq_cb_reap_tombstones, r->repl_name, current_time() + r->tombstone_reap_interval, 1000 * r->tombstone_reap_interval); slapi_log_err(SLAPI_LOG_REPL, repl_plugin_name, diff --git a/ldap/servers/slapd/back-ldbm/ldbm_config.c b/ldap/servers/slapd/back-ldbm/ldbm_config.c index 686b232..bf1b9c7 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_config.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_config.c @@ -2251,6 +2251,12 @@ ldbm_config_destroy(struct ldbminfo *li) { } slapi_ch_free((void **) &(li->li_new_directory)); slapi_ch_free((void **) &(li->li_directory)); + /* Destroy the mutexes and cond var */ + PR_DestroyLock(li->li_dbcache_mutex); + PR_DestroyLock(li->li_shutdown_mutex); + PR_DestroyLock(li->li_config_mutex); + PR_DestroyCondVar(li->li_dbcache_cv); + /* Finally free the ldbminfo */ slapi_ch_free((void **)&li); } diff --git a/ldap/servers/slapd/backend_manager.c b/ldap/servers/slapd/backend_manager.c index e2b524c..c9b98e2 100644 --- a/ldap/servers/slapd/backend_manager.c +++ b/ldap/servers/slapd/backend_manager.c @@ -303,8 +303,20 @@ be_cleanupall() slapi_ch_free((void**)&backends); } +/* + * This ifdef is needed to resolve a gcc 6 issue which throws a false positive + * here. See also: https://bugzilla.redhat.com/show_bug.cgi?id=1386445 + * + * It's a good idea to run this in EL7 to check the overflows etc, but with + * GCC 6 and lsan to find memory leaks .... + */ void be_flushall() +#if defined(__has_feature) +# if __has_feature(address_sanitizer) && __GNUC__ == 6 +__attribute__((no_sanitize("address"))) +# endif +#endif { int i; Slapi_PBlock pb = {0}; diff --git a/ldap/servers/slapd/connection.c b/ldap/servers/slapd/connection.c index 9e68a59..7c83c66 100644 --- a/ldap/servers/slapd/connection.c +++ b/ldap/servers/slapd/connection.c @@ -155,6 +155,21 @@ connection_done(Connection *conn) } /* PAGED_RESULTS */ pagedresults_cleanup_all(conn, 0); + + /* + * WARNING: There is a memory leak here! During a shutdown, connections + * can still have events in ns add io timeout job because of post connection + * or closing. The issue is that we can track the *jobs*, we only have the + * connection, and we can have 1:N connection:jobs. So we can lose IO jobs + * here. Thankfully, it's only for existing connections, and they are closed + * anyway, so it's just a mem / mutex leak. + * + * To fix it, involves the rewrite of connection handling, which will happen + * soon anyway, so please be patient while I undertake this! + * + * - wibrown December 2016. + */ + } /* diff --git a/ldap/servers/slapd/pblock.c b/ldap/servers/slapd/pblock.c index 1c35148..f074394 100644 --- a/ldap/servers/slapd/pblock.c +++ b/ldap/servers/slapd/pblock.c @@ -113,9 +113,21 @@ if ( PBLOCK ->pb_plugin->plg_type != TYPE) return( -1 ) #define SLAPI_PBLOCK_GET_PLUGIN_RELATED_POINTER( pb, element ) \ ((pb)->pb_plugin == NULL ? NULL : (pb)->pb_plugin->element) +/* + * This ifdef is needed to resolve a gcc 6 issue which throws a false positive + * here. See also: https://bugzilla.redhat.com/show_bug.cgi?id=1386445 + * + * It's a good idea to run this in EL7 to check the overflows etc, but with + * GCC 6 and lsan to find memory leaks .... + */ int slapi_pblock_get( Slapi_PBlock *pblock, int arg, void *value ) +#if defined(__has_feature) +# if __has_feature(address_sanitizer) && __GNUC__ == 6 +__attribute__((no_sanitize("address"))) +# endif +#endif { char *authtype; Slapi_Backend *be; @@ -1985,8 +1997,21 @@ slapi_pblock_get( Slapi_PBlock *pblock, int arg, void *value ) return( 0 ); } +/* + * This ifdef is needed to resolve a gcc 6 issue which throws a false positive + * here. See also: https://bugzilla.redhat.com/show_bug.cgi?id=1386445 + * + * It's a good idea to run this in EL7 to check the overflows etc, but with + * GCC 6 and lsan to find memory leaks .... + */ + int slapi_pblock_set( Slapi_PBlock *pblock, int arg, void *value ) +#if defined(__has_feature) +# if __has_feature(address_sanitizer) && __GNUC__ == 6 +__attribute__((no_sanitize("address"))) +# endif +#endif { char *authtype; -- 1.8.3.1