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!)

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));
 }
 

ldap/servers/plugins/replication/repl5_mtnode_ext.c

@@ -34,11 +34,31 @@ multimaster_mtnode_extension_init ()
 	dl_init (root_list, 0);
 }
 
+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));

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,

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);
 }

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};

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.
+     */
+
 }
 
 /*

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;
+    }
 }
 
 

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;
 

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);