ldap/servers/plugins/replication/cl5_api.c | 5 +++
ldap/servers/plugins/replication/repl5_mtnode_ext.c | 28 +++++++++++++++++---
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/mapping_tree.c | 13 ++++++---
ldap/servers/slapd/pblock.c | 25 +++++++++++++++++
ldap/servers/slapd/plugin.c | 12 +++++---
9 files changed, 109 insertions(+), 35 deletions(-)
New commits:
commit c87c57e024646c1f51bc2f04aa8af559557c0f4c
Author: William Brown <firstyear(a)redhat.com>
Date: Tue Dec 20 16:26:58 2016 +1000
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.
This also resolves a schema issue in plugin configuration creation, and a
segfault in server shutdown when replication has been disabled.
https://fedorahosted.org/389/ticket/49066
Author: wibrown
Review by: mreynolds, tbordaz (Thanks!)
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..70fe2cc 100644
--- a/ldap/servers/plugins/replication/repl5_mtnode_ext.c
+++ b/ldap/servers/plugins/replication/repl5_mtnode_ext.c
@@ -35,10 +35,30 @@ multimaster_mtnode_extension_init ()
}
void
+multimaster_mtnode_free_replica_object(const Slapi_DN *root) {
+ mapping_tree_node *mtnode;
+ multimaster_mtnode_extension *ext;
+
+ /* In some cases, root can be an empty SDN */
+ /* Othertimes, a bug is setting root to 0x8, and I can't see where... */
+ if (root != NULL) {
+ mtnode = slapi_get_mapping_tree_node_by_dn(root);
+ if (mtnode != NULL) {
+ 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 +79,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,
@@ -110,7 +130,7 @@ multimaster_mtnode_extension_constructor (void *object, void *parent)
we can't fully initialize replica here since backends
are not yet started. Instead, replica objects are created
during replication plugin startup */
- if (root)
+ if (root != NULL && !slapi_sdn_isempty(root))
{
/* for now just store node root in the root list */
dl_add (root_list, slapi_sdn_dup (root));
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/mapping_tree.c b/ldap/servers/slapd/mapping_tree.c
index 4e91757..1b8d2d9 100644
--- a/ldap/servers/slapd/mapping_tree.c
+++ b/ldap/servers/slapd/mapping_tree.c
@@ -2064,7 +2064,11 @@ done:
static int sdn_is_nulldn(const Slapi_DN *sdn){
if(sdn){
- const char *dn= slapi_sdn_get_ndn(sdn);
+ /*
+ * Use get_dn rather than get_ndn, because an issue in get_ndn exists
+ * where ndn can be set to 0x8
+ */
+ const char *dn= slapi_sdn_get_dn(sdn);
if(dn && ( '\0' == *dn)){
return 1;
}
@@ -2900,7 +2904,7 @@ slapi_get_mapping_tree_node_by_dn(const Slapi_DN *dn)
* it has been assigned to a different backend.
* e.g: a container backend
*/
- if ( sdn_is_nulldn(dn) && mapping_tree_root &&
mapping_tree_root->mtn_be[0] &&
+ if (sdn_is_nulldn(dn) && mapping_tree_root &&
mapping_tree_root->mtn_be[0] &&
mapping_tree_root->mtn_be[0] !=
slapi_be_select_by_instance_name(DSE_BACKEND)) {
return( mapping_tree_root );
}
@@ -2911,10 +2915,11 @@ slapi_get_mapping_tree_node_by_dn(const Slapi_DN *dn)
next_best_match = best_matching_child(current_best_match, dn);
}
- if (current_best_match == mapping_tree_root)
+ if (current_best_match == mapping_tree_root) {
return NULL;
- else
+ } else {
return current_best_match;
+ }
}
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;
diff --git a/ldap/servers/slapd/plugin.c b/ldap/servers/slapd/plugin.c
index 0416ff8..fea1a2a 100644
--- a/ldap/servers/slapd/plugin.c
+++ b/ldap/servers/slapd/plugin.c
@@ -301,10 +301,14 @@ slapi_register_plugin_ext(
slapi_entry_init_ext(e, sdn, NULL);
slapi_sdn_free(&sdn);
- slapi_entry_attr_set_charptr(e, "cn", name);
- slapi_entry_attr_set_charptr(e, ATTR_PLUGIN_TYPE, plugintype);
- if (!enabled)
- slapi_entry_attr_set_charptr(e, ATTR_PLUGIN_ENABLED, "off");
+ slapi_entry_attr_set_charptr(e, "cn", name);
+ /* Need a valid objectClass! No plugin OC so just use extensible :( */
+ slapi_entry_add_string(e, "objectclass", "top");
+ slapi_entry_add_string(e, "objectclass", "extensibleObject");
+ slapi_entry_attr_set_charptr(e, ATTR_PLUGIN_TYPE, plugintype);
+ if (!enabled) {
+ slapi_entry_attr_set_charptr(e, ATTR_PLUGIN_ENABLED, "off");
+ }
slapi_entry_attr_set_charptr(e, ATTR_PLUGIN_INITFN, initsymbol);