From 5b2efd34c07c65e24f4129430064f7299803dbf8 Mon Sep 17 00:00:00 2001 From: Noriko Hosoi 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