Blob Blame Raw
From 5b2efd34c07c65e24f4129430064f7299803dbf8 Mon Sep 17 00:00:00 2001
From: Noriko Hosoi <nhosoi@redhat.com>
Date: Fri, 2 Oct 2015 11:38:01 -0700
Subject: [PATCH 68/68] Ticket #48298 - ns-slapd crash during
 ipa-replica-manage del

Bug Description: The cause of the problem is rather not a race condition but
accessing an already freed agreement in a plug-in:
> The crashed thread is deleting an agreement object, which calls mep_pre_op.
> It eventually calls op_shared_search with the deleted agreement object with
> base scope and filter "(|(objectclass=*)(objectclass=ldapsubentry))"
> Since it is a DSE entry it goes to dse_search, in which it calls agmt_get_
> replarea and crashes in slapi_sdn_copy by NULL dereference in from SDN...

Fix Description: This patch adds the check to agmt_get_replarea, in which if
the agreement is not in the agreement list, it returnes NULL repl area.  When
the NULL repl area is returned the callers back off with an error.

https://fedorahosted.org/389/ticket/48298

Reviewed by rmeggins@redhat.com (Thanks, Rich!)

(cherry picked from commit 3cbdfa613ed8668337213fe9c3c15cf54ce798aa)
(cherry picked from commit f09eb8c0f8ee315b2a20d6460c975a546207411e)
---
 ldap/servers/plugins/replication/repl5.h           |  1 +
 ldap/servers/plugins/replication/repl5_agmt.c      | 17 +++++++++--
 ldap/servers/plugins/replication/repl5_agmtlist.c  | 34 ++++++++++++++++++----
 .../plugins/replication/repl5_inc_protocol.c       | 10 ++++++-
 .../plugins/replication/repl5_replica_config.c     |  6 +++-
 .../plugins/replication/repl5_tot_protocol.c       | 12 ++++++--
 .../plugins/replication/repl_session_plugin.c      | 22 +++++++++++---
 .../plugins/replication/windows_protocol_util.c    | 12 ++++++++
 8 files changed, 98 insertions(+), 16 deletions(-)

diff --git a/ldap/servers/plugins/replication/repl5.h b/ldap/servers/plugins/replication/repl5.h
index 17282bb..df92ca0 100644
--- a/ldap/servers/plugins/replication/repl5.h
+++ b/ldap/servers/plugins/replication/repl5.h
@@ -390,6 +390,7 @@ void agmtlist_shutdown();
 void agmtlist_notify_all(Slapi_PBlock *pb);
 Object* agmtlist_get_first_agreement_for_replica (Replica *r);
 Object* agmtlist_get_next_agreement_for_replica (Replica *r, Object *prev);
+int agmtlist_agmt_exists(const Repl_Agmt *ra);
 
 /* In repl5_backoff.c */
 typedef struct backoff_timer Backoff_Timer;
diff --git a/ldap/servers/plugins/replication/repl5_agmt.c b/ldap/servers/plugins/replication/repl5_agmt.c
index f84eacb..76d26a1 100644
--- a/ldap/servers/plugins/replication/repl5_agmt.c
+++ b/ldap/servers/plugins/replication/repl5_agmt.c
@@ -696,6 +696,12 @@ agmt_start(Repl_Agmt *ra)
      * index.
      */
     repl_sdn = agmt_get_replarea(ra);
+    if (!repl_sdn) {
+        slapi_log_error(SLAPI_LOG_REPL, repl_plugin_name,
+                        "agmt_start: failed to get repl area.  Please check agreement.\n");
+        prot_free(&prot);
+        return -1;
+    }
 
     pb = slapi_pblock_new();
     attrs[0] = (char*)type_agmtMaxCSN;
@@ -770,7 +776,7 @@ agmt_start(Repl_Agmt *ra)
                                 slapi_rdn_get_value_by_ref(slapi_rdn_get_rdn(ra->rdn)),
                                 ra->hostname, ra->port);
                     if(strstr(maxcsns[i], buf) || strstr(maxcsns[i], unavail_buf)){
-                    	/* Set the maxcsn */
+                        /* Set the maxcsn */
                         slapi_ch_free_string(&ra->maxcsn);
                         ra->maxcsn = slapi_ch_strdup(maxcsns[i]);
                         ra->consumerRID = agmt_maxcsn_get_rid(maxcsns[i]);
@@ -976,8 +982,11 @@ agmt_get_bindmethod(const Repl_Agmt *ra)
 Slapi_DN *
 agmt_get_replarea(const Repl_Agmt *ra)
 {
-	Slapi_DN *return_value;
+	Slapi_DN *return_value = NULL;
 	PR_ASSERT(NULL != ra);
+	if (!agmtlist_agmt_exists(ra)) {
+		return return_value;
+	}
 	PR_Lock(ra->lock);
 	return_value = slapi_sdn_new();
 	slapi_sdn_copy(ra->replarea, return_value);
@@ -2690,6 +2699,9 @@ get_agmt_status(Slapi_PBlock *pb, Slapi_Entry* e, Slapi_Entry* entryAfter,
 		Object *repl_obj = NULL;
 
 		replarea_sdn = agmt_get_replarea(ra);
+		if (!replarea_sdn) {
+			goto bail;
+		}
 		repl_obj = replica_get_replica_from_dn(replarea_sdn);
 		slapi_sdn_free(&replarea_sdn);
 		if (repl_obj) {
@@ -2748,6 +2760,7 @@ get_agmt_status(Slapi_PBlock *pb, Slapi_Entry* e, Slapi_Entry* entryAfter,
 			slapi_entry_add_string(e, "nsds5replicaLastInitStatus", ra->last_init_status);
 		}
 	}
+bail:
 	return SLAPI_DSE_CALLBACK_OK;
 }
 
diff --git a/ldap/servers/plugins/replication/repl5_agmtlist.c b/ldap/servers/plugins/replication/repl5_agmtlist.c
index 34650b4..f50862f 100644
--- a/ldap/servers/plugins/replication/repl5_agmtlist.c
+++ b/ldap/servers/plugins/replication/repl5_agmtlist.c
@@ -109,6 +109,24 @@ agmtlist_release_agmt(Repl_Agmt *ra)
 	}
 }
 
+int
+agmtlist_agmt_exists(const Repl_Agmt *ra)
+{
+	Object *ro;
+	int exists = 0;
+
+	PR_ASSERT(NULL != agmt_set);
+	if (!ra) {
+		return exists;
+	}
+	ro = objset_find(agmt_set, agmt_ptr_cmp, (const void *)ra);
+	if (ro) {
+		exists = 1;
+		object_release(ro);
+	}
+	return exists;
+}
+
 
 /*
  * Note: when we add the new object, we have a reference to it. We hold
@@ -135,6 +153,9 @@ add_new_agreement(Slapi_Entry *e)
 
     /* get the replica for this agreement */
     replarea_sdn = agmt_get_replarea(ra);
+    if (!replarea_sdn) {
+        return 1;
+    }
     repl_obj = replica_get_replica_from_dn(replarea_sdn);
     slapi_sdn_free(&replarea_sdn);
     if (repl_obj) {
@@ -841,13 +862,16 @@ Object* agmtlist_get_next_agreement_for_replica (Replica *r, Object *prev)
     else
         obj = objset_first_obj(agmt_set);
         
-    while (obj)
-    {
+    for ( ; obj; obj = objset_next_obj(agmt_set, obj)) {
         agmt = (Repl_Agmt*)object_get_data (obj);
-        PR_ASSERT (agmt);
+        if (!agmt) {
+            continue;
+        }
 
         agmt_root = agmt_get_replarea(agmt);
-        PR_ASSERT (agmt_root);
+        if (!agmt_root) {
+            continue;
+        }
 
         if (slapi_sdn_compare (replica_root, agmt_root) == 0)
         {
@@ -856,7 +880,7 @@ Object* agmtlist_get_next_agreement_for_replica (Replica *r, Object *prev)
         }
 
         slapi_sdn_free (&agmt_root);
-        obj = objset_next_obj(agmt_set, obj);
+
     }
 
     return NULL;
diff --git a/ldap/servers/plugins/replication/repl5_inc_protocol.c b/ldap/servers/plugins/replication/repl5_inc_protocol.c
index 7680340..244bbb2 100644
--- a/ldap/servers/plugins/replication/repl5_inc_protocol.c
+++ b/ldap/servers/plugins/replication/repl5_inc_protocol.c
@@ -1906,6 +1906,7 @@ send_updates(Private_Repl_Protocol *prp, RUV *remote_update_vector, PRUint32 *nu
 		{
 			Replica *replica;
 			ReplicaId rid = -1; /* Used to create the replica keep alive subentry */
+			Slapi_DN *replarea_sdn = NULL;
 			replica = (Replica*) object_get_data(prp->replica_object);
 			if (replica)
 			{
@@ -1914,7 +1915,14 @@ send_updates(Private_Repl_Protocol *prp, RUV *remote_update_vector, PRUint32 *nu
 			slapi_log_error(SLAPI_LOG_REPL, repl_plugin_name,
 					"%s: skipped updates was definitely too high (%d) update the subentry now\n",
 					agmt_get_long_name(prp->agmt), skipped_updates);
-			replica_subentry_update(agmt_get_replarea(prp->agmt), rid);
+			replarea_sdn = agmt_get_replarea(prp->agmt);
+			if (!replarea_sdn) {
+				slapi_log_error(SLAPI_LOG_FATAL, repl_plugin_name,
+				                "send_updates: Unknown replication area due to agreement not found.");
+				return_value = UPDATE_FATAL_ERROR;
+			} else {
+				replica_subentry_update(replarea_sdn, rid);
+			}
 		}
 		/* Terminate the results reading thread */
 		if (!prp->repl50consumer) 
diff --git a/ldap/servers/plugins/replication/repl5_replica_config.c b/ldap/servers/plugins/replication/repl5_replica_config.c
index 8d3c481..e85ae3e 100644
--- a/ldap/servers/plugins/replication/repl5_replica_config.c
+++ b/ldap/servers/plugins/replication/repl5_replica_config.c
@@ -2368,13 +2368,17 @@ replica_send_cleanruv_task(Repl_Agmt *agmt, cleanruv_data *clean_data)
         conn_delete_internal_ext(conn);
         return;
     }
-    val.bv_len = PR_snprintf(data, sizeof(data), "CLEANRUV%d", clean_data->rid);
     sdn = agmt_get_replarea(agmt);
+    if (!sdn) {
+        conn_delete_internal_ext(conn);
+        return;
+    }
     mod.mod_op  = LDAP_MOD_ADD|LDAP_MOD_BVALUES;
     mod.mod_type = "nsds5task";
     mod.mod_bvalues = vals;
     vals [0] = &val;
     vals [1] = NULL;
+    val.bv_len = PR_snprintf(data, sizeof(data), "CLEANRUV%d", clean_data->rid);
     val.bv_val = data;
     mods[0] = &mod;
     mods[1] = NULL;
diff --git a/ldap/servers/plugins/replication/repl5_tot_protocol.c b/ldap/servers/plugins/replication/repl5_tot_protocol.c
index 7da893a..16b51b5 100644
--- a/ldap/servers/plugins/replication/repl5_tot_protocol.c
+++ b/ldap/servers/plugins/replication/repl5_tot_protocol.c
@@ -329,6 +329,13 @@ repl5_tot_run(Private_Repl_Protocol *prp)
 		goto done;
 	}
 
+	area_sdn = agmt_get_replarea(prp->agmt);
+	if (!area_sdn) {
+		slapi_log_error(SLAPI_LOG_FATAL, repl_plugin_name, "Warning: unable to "
+		                "get repl area.  Please check agreement.\n");
+		goto done;
+	}
+
 	conn_set_timeout(prp->conn, agmt_get_timeout(prp->agmt));
 
     /* acquire remote replica */
@@ -387,11 +394,10 @@ repl5_tot_run(Private_Repl_Protocol *prp)
     slapi_log_error(SLAPI_LOG_FATAL, repl_plugin_name, "Beginning total update of replica "
 		"\"%s\".\n", agmt_get_long_name(prp->agmt));
 
-    pb = slapi_pblock_new ();
+    /* RMREPL - need to send schema here */
 
-	/* RMREPL - need to send schema here */
+    pb = slapi_pblock_new ();
 
-	area_sdn = agmt_get_replarea(prp->agmt);
     /* we need to provide managedsait control so that referral entries can
        be replicated */
     ctrls = (LDAPControl **)slapi_ch_calloc (3, sizeof (LDAPControl *));
diff --git a/ldap/servers/plugins/replication/repl_session_plugin.c b/ldap/servers/plugins/replication/repl_session_plugin.c
index 1c04089..2fa993d 100644
--- a/ldap/servers/plugins/replication/repl_session_plugin.c
+++ b/ldap/servers/plugins/replication/repl_session_plugin.c
@@ -48,6 +48,10 @@ repl_session_plugin_call_agmt_init_cb(Repl_Agmt *ra)
     }
     if (initfunc) {
         replarea = agmt_get_replarea(ra);
+        if (!replarea) {
+            LDAPDebug0Args(LDAP_DEBUG_ANY, "repl_session_plugin_call_agmt_init_cb -- Aborted -- No replication area\n");
+            return;
+        }
         cookie = (*initfunc)(replarea);
         slapi_sdn_free(&replarea);
     }
@@ -73,8 +77,11 @@ repl_session_plugin_call_pre_acquire_cb(const Repl_Agmt *ra, int is_total,
 
     if (thefunc) {
         replarea = agmt_get_replarea(ra);
-        rc = (*thefunc)(agmt_get_priv(ra), replarea, is_total,
-                data_guid, data);
+        if (!replarea) {
+            LDAPDebug0Args(LDAP_DEBUG_ANY, "repl_session_plugin_call_pre_acquire_cb -- Aborted -- No replication area\n");
+            return 1;
+        }
+        rc = (*thefunc)(agmt_get_priv(ra), replarea, is_total, data_guid, data);
         slapi_sdn_free(&replarea);
     }
 
@@ -95,8 +102,11 @@ repl_session_plugin_call_post_acquire_cb(const Repl_Agmt *ra, int is_total,
 
     if (thefunc) {
         replarea = agmt_get_replarea(ra);
-        rc = (*thefunc)(agmt_get_priv(ra), replarea,
-		is_total, data_guid, data);
+        if (!replarea) {
+            LDAPDebug0Args(LDAP_DEBUG_ANY, "repl_session_plugin_call_post_acquire_cb -- Aborted -- No replication area\n");
+            return 1;
+        }
+        rc = (*thefunc)(agmt_get_priv(ra), replarea, is_total, data_guid, data);
         slapi_sdn_free(&replarea);
     }
 
@@ -151,6 +161,10 @@ repl_session_plugin_call_destroy_agmt_cb(const Repl_Agmt *ra)
 
     if (thefunc) {
         replarea = agmt_get_replarea(ra);
+        if (!replarea) {
+            LDAPDebug0Args(LDAP_DEBUG_ANY, "repl_session_plugin_call_destroy_agmt_cb -- Aborted -- No replication area\n");
+            return;
+        }
         (*thefunc)(agmt_get_priv(ra), replarea);
         slapi_sdn_free(&replarea);
     }
diff --git a/ldap/servers/plugins/replication/windows_protocol_util.c b/ldap/servers/plugins/replication/windows_protocol_util.c
index 5c12af7..084b520 100644
--- a/ldap/servers/plugins/replication/windows_protocol_util.c
+++ b/ldap/servers/plugins/replication/windows_protocol_util.c
@@ -5319,6 +5319,13 @@ windows_update_local_entry(Private_Repl_Protocol *prp,Slapi_Entry *remote_entry,
 		 * in the groups caused by moving member entries.
 		 * We need to update the local groups manually... */
 		local_subtree = agmt_get_replarea(prp->agmt); 
+		if (!local_subtree) {
+			slapi_log_error(SLAPI_LOG_FATAL, windows_repl_plugin_name,
+			                "failed to get local subtree from agreement\n");
+			local_entry = orig_local_entry;
+			orig_local_entry = NULL;
+			goto bail;
+		}
 		local_subtree_sdn = local_subtree;
 		orig_local_sdn = slapi_entry_get_sdn_const(orig_local_entry);
 		escaped_filter_val = slapi_escape_filter_value((char *)slapi_sdn_get_ndn(orig_local_sdn),
@@ -5651,6 +5658,11 @@ windows_search_local_entry_by_uniqueid(Private_Repl_Protocol *prp,
 	*ret_entry = NULL;
 	if (is_global) { /* Search from the suffix (rename case) */
 		local_subtree = agmt_get_replarea(prp->agmt); 
+		if (!local_subtree) {
+			slapi_log_error(SLAPI_LOG_FATAL, windows_repl_plugin_name,
+			                "failed to get local subtree from agreement\n");
+			return LDAP_PARAM_ERROR;
+		}
 		local_subtree_sdn = local_subtree;
 	} else {
 		local_subtree_sdn = windows_private_get_directory_treetop(prp->agmt);
-- 
1.9.3