389-commits July 2015

389-commits@lists.fedoraproject.org

3 participants
83 discussions

Branch '389-ds-base-1.3.4' - ldap/servers
by Mark Reynolds 20 Jul '15

20 Jul '15

ldap/servers/slapd/back-ldbm/ldbm_delete.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) New commits: commit 6d439887b99da557e8d7bc0c611d9afa909fdce7 Author: Mark Reynolds <mreynolds(a)redhat.com> Date: Mon Jul 20 14:22:05 2015 -0400 Ticket 48206 - Crash during retro changelog trimming Bug Description: If the retro changelog entry is small, its possible that during the trimming the reto changelog entry is not in the cache after the trim, but its tries to blindly unlock it from the cache, which leads to a crash. FIx Description: After we call the post op plugins and retrieve the entry from the cache, double check that it was found. If it is not found, do not unlock it. https://fedorahosted.org/389/ticket/48206 Reviewed by: nhosoi(Thanks!) (cherry picked from commit 2a8a8c8ced5849dada34ab28d79e87dd3636e413) diff --git a/ldap/servers/slapd/back-ldbm/ldbm_delete.c b/ldap/servers/slapd/back-ldbm/ldbm_delete.c index 59c1f76..f31d545 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_delete.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_delete.c @@ -1257,17 +1257,24 @@ ldbm_back_delete( Slapi_PBlock *pb ) CACHE_RETURN(&inst->inst_cache, &e); } } - if (cache_is_in_cache(&inst->inst_cache, e)) { - ep_id = e->ep_id; /* Otherwise, e might have been freed. */ - CACHE_REMOVE(&inst->inst_cache, e); - } - cache_unlock_entry(&inst->inst_cache, e); - CACHE_RETURN(&inst->inst_cache, &e); - /* - * e is unlocked and no longer in cache. - * It could be freed at any moment. + + /* + * e could have been replaced by cache_find_id(), recheck if it's NULL + * before trying to unlock it, etc. */ - e = NULL; + if (e) { + if (cache_is_in_cache(&inst->inst_cache, e)) { + ep_id = e->ep_id; /* Otherwise, e might have been freed. */ + CACHE_REMOVE(&inst->inst_cache, e); + } + cache_unlock_entry(&inst->inst_cache, e); + CACHE_RETURN(&inst->inst_cache, &e); + /* + * e is unlocked and no longer in cache. + * It could be freed at any moment. + */ + e = NULL; + } if (entryrdn_get_switch() && ep_id) { /* subtree-rename: on */ /* since the op was successful, delete the tombstone dn from the dn cache */

1 0

ldap/servers
by Mark Reynolds 20 Jul '15

20 Jul '15

ldap/servers/slapd/back-ldbm/ldbm_delete.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) New commits: commit 2a8a8c8ced5849dada34ab28d79e87dd3636e413 Author: Mark Reynolds <mreynolds(a)redhat.com> Date: Mon Jul 20 14:22:05 2015 -0400 Ticket 48206 - Crash during retro changelog trimming Bug Description: If the retro changelog entry is small, its possible that during the trimming the reto changelog entry is not in the cache after the trim, but its tries to blindly unlock it from the cache, which leads to a crash. FIx Description: After we call the post op plugins and retrieve the entry from the cache, double check that it was found. If it is not found, do not unlock it. https://fedorahosted.org/389/ticket/48206 Reviewed by: nhosoi(Thanks!) diff --git a/ldap/servers/slapd/back-ldbm/ldbm_delete.c b/ldap/servers/slapd/back-ldbm/ldbm_delete.c index 59c1f76..f31d545 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_delete.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_delete.c @@ -1257,17 +1257,24 @@ ldbm_back_delete( Slapi_PBlock *pb ) CACHE_RETURN(&inst->inst_cache, &e); } } - if (cache_is_in_cache(&inst->inst_cache, e)) { - ep_id = e->ep_id; /* Otherwise, e might have been freed. */ - CACHE_REMOVE(&inst->inst_cache, e); - } - cache_unlock_entry(&inst->inst_cache, e); - CACHE_RETURN(&inst->inst_cache, &e); - /* - * e is unlocked and no longer in cache. - * It could be freed at any moment. + + /* + * e could have been replaced by cache_find_id(), recheck if it's NULL + * before trying to unlock it, etc. */ - e = NULL; + if (e) { + if (cache_is_in_cache(&inst->inst_cache, e)) { + ep_id = e->ep_id; /* Otherwise, e might have been freed. */ + CACHE_REMOVE(&inst->inst_cache, e); + } + cache_unlock_entry(&inst->inst_cache, e); + CACHE_RETURN(&inst->inst_cache, &e); + /* + * e is unlocked and no longer in cache. + * It could be freed at any moment. + */ + e = NULL; + } if (entryrdn_get_switch() && ep_id) { /* subtree-rename: on */ /* since the op was successful, delete the tombstone dn from the dn cache */

1 0

ldap/admin
by Richard Allen Megginson 20 Jul '15

20 Jul '15

ldap/admin/src/logconv.pl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) New commits: commit 29043c5716a1bc8364689d518cb4e35722eaaf77 Author: Rich Megginson <rmeggins(a)redhat.com> Date: Mon Jul 20 10:31:46 2015 -0600 Ticket #48224 - redux 2 - logconv.pl should handle *.tar.xz, *.txz, *.xz log files https://fedorahosted.org/389/ticket/48224 Reviewed by: nhosoi (Thanks!) Branch: master Fix Description: Use $? instead of $! to get pipe errors. Platforms tested: Fedora 21, RHEL 7.2 candidate Flag Day: no Doc impact: no diff --git a/ldap/admin/src/logconv.pl b/ldap/admin/src/logconv.pl index 0038a03..3113f8a 100755 --- a/ldap/admin/src/logconv.pl +++ b/ldap/admin/src/logconv.pl @@ -437,9 +437,9 @@ sub doUncompress { # so use the xz command directly # NOTE: This doesn't work if the argument is a file handle e.g. from # Archive::Tar - $! = 0; # clear - if (!open($TARFH, "xz -dc $_ |") or $!) { - openFailed($!, $_); + $? = 0; # clear + if (!open($TARFH, "xz -dc $_ |") or $?) { + openFailed($?, $_); return; } } else {

1 0

Branch '389-ds-base-1.3.3' - ldap/admin
by Richard Allen Megginson 20 Jul '15

20 Jul '15

ldap/admin/src/logconv.pl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) New commits: commit c81f40ad42c7def0b6da612d4ddf44e877d601b8 Author: Rich Megginson <rmeggins(a)redhat.com> Date: Mon Jul 20 10:31:46 2015 -0600 Ticket #48224 - redux 2 - logconv.pl should handle *.tar.xz, *.txz, *.xz log files https://fedorahosted.org/389/ticket/48224 Reviewed by: nhosoi (Thanks!) Branch: 389-ds-base-1.3.3 Fix Description: Use $? instead of $! to get pipe errors. Platforms tested: Fedora 21, RHEL 7.2 candidate Flag Day: no Doc impact: no (cherry picked from commit 29043c5716a1bc8364689d518cb4e35722eaaf77) (cherry picked from commit 0e31d818366e846f45c60b2c24bdb8026a82c048) diff --git a/ldap/admin/src/logconv.pl b/ldap/admin/src/logconv.pl index 4842ec7..37862a6 100755 --- a/ldap/admin/src/logconv.pl +++ b/ldap/admin/src/logconv.pl @@ -451,9 +451,9 @@ sub doUncompress { # so use the xz command directly # NOTE: This doesn't work if the argument is a file handle e.g. from # Archive::Tar - $! = 0; # clear - if (!open($TARFH, "xz -dc $_ |") or $!) { - openFailed($!, $_); + $? = 0; # clear + if (!open($TARFH, "xz -dc $_ |") or $?) { + openFailed($?, $_); return; } } else {

1 0

Branch '389-ds-base-1.3.4' - ldap/admin
by Richard Allen Megginson 20 Jul '15

20 Jul '15

ldap/admin/src/logconv.pl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) New commits: commit 0e31d818366e846f45c60b2c24bdb8026a82c048 Author: Rich Megginson <rmeggins(a)redhat.com> Date: Mon Jul 20 10:31:46 2015 -0600 Ticket #48224 - redux 2 - logconv.pl should handle *.tar.xz, *.txz, *.xz log files https://fedorahosted.org/389/ticket/48224 Reviewed by: nhosoi (Thanks!) Branch: 389-ds-base-1.3.4 Fix Description: Use $? instead of $! to get pipe errors. Platforms tested: Fedora 21, RHEL 7.2 candidate Flag Day: no Doc impact: no (cherry picked from commit 29043c5716a1bc8364689d518cb4e35722eaaf77) diff --git a/ldap/admin/src/logconv.pl b/ldap/admin/src/logconv.pl index 0038a03..3113f8a 100755 --- a/ldap/admin/src/logconv.pl +++ b/ldap/admin/src/logconv.pl @@ -437,9 +437,9 @@ sub doUncompress { # so use the xz command directly # NOTE: This doesn't work if the argument is a file handle e.g. from # Archive::Tar - $! = 0; # clear - if (!open($TARFH, "xz -dc $_ |") or $!) { - openFailed($!, $_); + $? = 0; # clear + if (!open($TARFH, "xz -dc $_ |") or $?) { + openFailed($?, $_); return; } } else {

1 0

Branch '389-ds-base-1.3.4' - ldap/admin
by Mark Reynolds 20 Jul '15

20 Jul '15

ldap/admin/src/logconv.pl | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) New commits: commit 16b95b12d26fb293d68be154c602398798353bb8 Author: Mark Reynolds <mreynolds(a)redhat.com> Date: Mon Jul 20 11:18:12 2015 -0400 Ticket 47910 - logconv.pl - check that the end time is greater than the start time Bug Description: There is no check if the end time is greater than the start time. This leads to an empty report being generated, when an error should be returned instead. Fix Description: If start and end time are used, validate that the end time is greater than the start time. Also, improved an error message when the tool options are not correctly used. https://fedorahosted.org/389/ticket/47910 Reviewed by: nhosoi(Thanks!) (cherry picked from commit 34ffa6c44734b99c252e7585bb499089ac8e6a67) diff --git a/ldap/admin/src/logconv.pl b/ldap/admin/src/logconv.pl index d26e91e..0038a03 100755 --- a/ldap/admin/src/logconv.pl +++ b/ldap/admin/src/logconv.pl @@ -148,14 +148,13 @@ while($arg_count <= $#ARGV){ } if($file_count == 0){ - if($reportStatsSecFile or $reportStatsMinFile){ - print "Usage error for option -m or -M, either the output file or access log is missing!\n\n"; - } else { - print "There are no access logs specified!\n\n"; - } + print "There are no access logs specified, or the tool options have not been used correctly!\n"; exit 1; } +# +# Initialize the statistic blocks +# if ($reportStatsSecFile) { $s_stats = new_stats_block($reportStatsSecFile); $reportStats = "-m"; @@ -357,6 +356,19 @@ my %monthname = ( ); +# +# Validate start/end times (if specified) +# +if ($startTime and $endTime){ + # Make sure the end time is not earlier than the start time + my $testStart = convertTimeToSeconds($startTime); + my $testEnd = convertTimeToSeconds($endTime); + if ($testStart > $testEnd){ + print "Start time ($startTime) is greater than end time ($endTime)!\n"; + exit 1; + } +} + my $linesProcessed; my $lineBlockCount; my $cursize = 0;

1 0

ldap/admin
by Mark Reynolds 20 Jul '15

20 Jul '15

ldap/admin/src/logconv.pl | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) New commits: commit 34ffa6c44734b99c252e7585bb499089ac8e6a67 Author: Mark Reynolds <mreynolds(a)redhat.com> Date: Mon Jul 20 11:18:12 2015 -0400 Ticket 47910 - logconv.pl - check that the end time is greater than the start time Bug Description: There is no check if the end time is greater than the start time. This leads to an empty report being generated, when an error should be returned instead. Fix Description: If start and end time are used, validate that the end time is greater than the start time. Also, improved an error message when the tool options are not correctly used. https://fedorahosted.org/389/ticket/47910 Reviewed by: nhosoi(Thanks!) diff --git a/ldap/admin/src/logconv.pl b/ldap/admin/src/logconv.pl index d26e91e..0038a03 100755 --- a/ldap/admin/src/logconv.pl +++ b/ldap/admin/src/logconv.pl @@ -148,14 +148,13 @@ while($arg_count <= $#ARGV){ } if($file_count == 0){ - if($reportStatsSecFile or $reportStatsMinFile){ - print "Usage error for option -m or -M, either the output file or access log is missing!\n\n"; - } else { - print "There are no access logs specified!\n\n"; - } + print "There are no access logs specified, or the tool options have not been used correctly!\n"; exit 1; } +# +# Initialize the statistic blocks +# if ($reportStatsSecFile) { $s_stats = new_stats_block($reportStatsSecFile); $reportStats = "-m"; @@ -357,6 +356,19 @@ my %monthname = ( ); +# +# Validate start/end times (if specified) +# +if ($startTime and $endTime){ + # Make sure the end time is not earlier than the start time + my $testStart = convertTimeToSeconds($startTime); + my $testEnd = convertTimeToSeconds($endTime); + if ($testStart > $testEnd){ + print "Start time ($startTime) is greater than end time ($endTime)!\n"; + exit 1; + } +} + my $linesProcessed; my $lineBlockCount; my $cursize = 0;

1 0

Branch '389-ds-base-1.3.4' - ldap/servers
by Mark Reynolds 17 Jul '15

17 Jul '15

ldap/servers/plugins/replication/repl5.h | 1 ldap/servers/plugins/replication/repl5_agmt.c | 211 ++++++++++++-------------- 2 files changed, 101 insertions(+), 111 deletions(-) New commits: commit 23a3ff6082cba3eb749401eff44942b16dc30538 Author: Mark Reynolds <mreynolds(a)redhat.com> Date: Fri Jul 17 15:08:00 2015 -0400 Ticket 48179 - Starting a replica agreement can lead to deadlock Bug Description: When starting a replica agreement and setting the agmt maxcsn a deadlock can occur with another op updating nsuniqueid index. When setting the agmt maxcsn the server searches for the tombstone ruv which uses the nsuniqueid index, and it does this while holding the repl agmt lock. If another thread is doing a delete and writing to the change log, it can also grab a write lock on the nsuniqueid index, before it attempts to grab the agmt lock. This can lead to a deadlock if the timing is right. Fix Description: When starting the agmt and setting the agmt maxcsn, search/get the tombstone ruv before we take the repl agmt lock. https://fedorahosted.org/389/ticket/48179 Reviewed by: nhosoi(Thanks!) (cherry picked from commit eb3086dcb0c56a23d6cee00a12f38b2584fe59a2) diff --git a/ldap/servers/plugins/replication/repl5.h b/ldap/servers/plugins/replication/repl5.h index 4a5d859..0b0f26b 100644 --- a/ldap/servers/plugins/replication/repl5.h +++ b/ldap/servers/plugins/replication/repl5.h @@ -380,7 +380,6 @@ PRUint64 agmt_get_protocol_timeout(Repl_Agmt *agmt); void agmt_set_protocol_timeout(Repl_Agmt *agmt, PRUint64 timeout); void agmt_update_maxcsn(Replica *r, Slapi_DN *sdn, int op, LDAPMod **mods, CSN *csn); void add_agmt_maxcsns(Slapi_Entry *e, Replica *r); -void agmt_set_maxcsn(Repl_Agmt *ra); void agmt_remove_maxcsn(Repl_Agmt *ra); int agmt_maxcsn_to_smod (Replica *r, Slapi_Mod *smod); int agmt_set_WaitForAsyncResults(Repl_Agmt *ra, const Slapi_Entry *e); diff --git a/ldap/servers/plugins/replication/repl5_agmt.c b/ldap/servers/plugins/replication/repl5_agmt.c index 9d1a8f2..f84eacb 100644 --- a/ldap/servers/plugins/replication/repl5_agmt.c +++ b/ldap/servers/plugins/replication/repl5_agmt.c @@ -668,43 +668,127 @@ int agmt_start(Repl_Agmt *ra) { Repl_Protocol *prot = NULL; + Slapi_PBlock *pb = NULL; + Slapi_Entry **entries = NULL; + Slapi_DN *repl_sdn = NULL; + char *attrs[2]; + int protocol_state; + int found_ruv = 0; + int rc = 0; - int protocol_state; - - /* To Allow Consumer Initialisation when adding an agreement: */ - if (ra->auto_initialize == STATE_PERFORMING_TOTAL_UPDATE) - { - protocol_state = STATE_PERFORMING_TOTAL_UPDATE; - } - else - { - protocol_state = STATE_PERFORMING_INCREMENTAL_UPDATE; - } + /* To Allow Consumer Initialisation when adding an agreement: */ + if (ra->auto_initialize == STATE_PERFORMING_TOTAL_UPDATE){ + protocol_state = STATE_PERFORMING_TOTAL_UPDATE; + } else { + protocol_state = STATE_PERFORMING_INCREMENTAL_UPDATE; + } /* First, create a new protocol object */ if ((prot = prot_new(ra, protocol_state)) == NULL) { return -1; } - /* Now it is safe to own the agreement lock */ + /* + * Set the agmt maxcsn + * + * We need to get the replica ruv before we take the + * agmt lock to avoid potential deadlocks on the nsuniqueid + * index. + */ + repl_sdn = agmt_get_replarea(ra); + + pb = slapi_pblock_new(); + attrs[0] = (char*)type_agmtMaxCSN; + attrs[1] = NULL; + slapi_search_internal_set_pb_ext( + pb, + repl_sdn, + LDAP_SCOPE_BASE, + "objectclass=*", + attrs, + 0, + NULL, + RUV_STORAGE_ENTRY_UNIQUEID, + repl_get_plugin_identity (PLUGIN_MULTIMASTER_REPLICATION), + OP_FLAG_REPLICATED); + slapi_search_internal_pb (pb); + + slapi_pblock_get(pb, SLAPI_PLUGIN_INTOP_RESULT, &rc); + if (rc == LDAP_SUCCESS){ + slapi_pblock_get(pb, SLAPI_PLUGIN_INTOP_SEARCH_ENTRIES, &entries); + if (NULL == entries || NULL == entries[0]){ + slapi_log_error(SLAPI_LOG_FATAL, repl_plugin_name, + "agmt_start: replica ruv tombstone entry for " + "replica %s not found\n", + slapi_sdn_get_dn(ra->replarea)); + } else { + found_ruv = 1; + } + } + + /* + * Now it is safe to own the agreement lock + */ PR_Lock(ra->lock); /* Check that replication is not already started */ if (ra->protocol != NULL) { slapi_log_error(SLAPI_LOG_REPL, repl_plugin_name, "replication already started for agreement \"%s\"\n", agmt_get_long_name(ra)); - PR_Unlock(ra->lock); prot_free(&prot); - return 0; + goto done; } + /* Set and start the protocol */ ra->protocol = prot; - - /* Start the protocol thread */ prot_start(ra->protocol); - agmt_set_maxcsn(ra); + /* + * If we found the repl ruv, set the agmt maxcsn... + */ + if (found_ruv){ + Replica *r; + Object *repl_obj; + char **maxcsns = NULL; + int i; + maxcsns = slapi_entry_attr_get_charray(entries[0], type_agmtMaxCSN); + repl_obj = prot_get_replica_object(ra->protocol); + if(repl_obj && maxcsns){ + r = (Replica *)object_get_data(repl_obj); + if(r){ + /* + * Loop over all the agmt maxcsns and find ours... + */ + for(i = 0; maxcsns[i]; i++){ + char buf[BUFSIZ]; + char unavail_buf[BUFSIZ]; + + PR_snprintf(buf,BUFSIZ,"%s;%s;%s;%d;",slapi_sdn_get_dn(repl_sdn), + slapi_rdn_get_value_by_ref(slapi_rdn_get_rdn(ra->rdn)), + ra->hostname, ra->port); + PR_snprintf(unavail_buf, BUFSIZ,"%s;%s;%s;%d;unavailable", slapi_sdn_get_dn(repl_sdn), + 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 */ + slapi_ch_free_string(&ra->maxcsn); + ra->maxcsn = slapi_ch_strdup(maxcsns[i]); + ra->consumerRID = agmt_maxcsn_get_rid(maxcsns[i]); + ra->tmpConsumerRID = 1; + break; + } + } + } + } + slapi_ch_array_free(maxcsns); + } + +done: PR_Unlock(ra->lock); + slapi_free_search_results_internal(pb); + slapi_pblock_destroy (pb); + slapi_sdn_free(&repl_sdn); + return 0; } @@ -3052,99 +3136,6 @@ agmt_maxcsn_to_smod (Replica *r, Slapi_Mod *smod) } /* - * Called when we start a repl agmt - */ -void -agmt_set_maxcsn(Repl_Agmt *ra) -{ - Slapi_PBlock *pb = NULL; - Slapi_Entry **entries = NULL; - Replica *r = NULL; - Object *repl_obj; - const Slapi_DN *tombstone_sdn = NULL; - char *attrs[2]; - int rc; - - /* read ruv state from the ruv tombstone entry */ - pb = slapi_pblock_new(); - if (!pb) { - slapi_log_error(SLAPI_LOG_FATAL, repl_plugin_name, "agmt_set_maxcsn: Out of memory\n"); - goto done; - } - repl_obj = prot_get_replica_object(ra->protocol); - if(repl_obj){ - r = (Replica *)object_get_data(repl_obj); - tombstone_sdn = replica_get_root(r); - } - ra->maxcsn = NULL; - attrs[0] = (char*)type_agmtMaxCSN; - attrs[1] = NULL; - slapi_search_internal_set_pb_ext( - pb, - (Slapi_DN *)tombstone_sdn, - LDAP_SCOPE_BASE, - "objectclass=*", - attrs, - 0, /* attrsonly */ - NULL, /* controls */ - RUV_STORAGE_ENTRY_UNIQUEID, - repl_get_plugin_identity (PLUGIN_MULTIMASTER_REPLICATION), - OP_FLAG_REPLICATED); /* flags */ - slapi_search_internal_pb (pb); - - slapi_pblock_get(pb, SLAPI_PLUGIN_INTOP_RESULT, &rc); - if (rc == LDAP_SUCCESS){ - Replica *r; - Object *repl_obj; - char **maxcsns; - int i; - - slapi_pblock_get(pb, SLAPI_PLUGIN_INTOP_SEARCH_ENTRIES, &entries); - if (NULL == entries || NULL == entries[0]){ - slapi_log_error(SLAPI_LOG_FATAL, repl_plugin_name, - "agmt_set_maxcsn: replica ruv tombstone entry for " - "replica %s not found\n", - slapi_sdn_get_dn(ra->replarea)); - goto done; - } - maxcsns = slapi_entry_attr_get_charray(entries[0], type_agmtMaxCSN); - repl_obj = prot_get_replica_object(ra->protocol); - if(repl_obj && maxcsns){ - r = (Replica *)object_get_data(repl_obj); - if(r){ - /* - * Loop over all the agmt maxcsns and find ours - */ - for(i = 0; maxcsns[i]; i++){ - char buf[BUFSIZ]; - char unavail_buf[BUFSIZ]; - - PR_snprintf(buf,BUFSIZ,"%s;%s;%s;%d;",slapi_sdn_get_dn(ra->replarea), - slapi_rdn_get_value_by_ref(slapi_rdn_get_rdn(ra->rdn)), - ra->hostname, ra->port); - PR_snprintf(unavail_buf, BUFSIZ,"%s;%s;%s;%d;unavailable", slapi_sdn_get_dn(ra->replarea), - 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)){ - slapi_ch_free_string(&ra->maxcsn); - ra->maxcsn = slapi_ch_strdup(maxcsns[i]); - ra->consumerRID = agmt_maxcsn_get_rid(maxcsns[i]); - ra->tmpConsumerRID = 1; - break; - } - } - } - } - slapi_ch_array_free(maxcsns); - } -done: - if (NULL != pb){ - slapi_free_search_results_internal(pb); - slapi_pblock_destroy (pb); - } -} - -/* * Parse out the consumer replicaID from the agmt maxcsn * * "repl area;agmt_rdn;hostname;port;consumer_rid;maxcsn"

1 0

ldap/servers
by Mark Reynolds 17 Jul '15

17 Jul '15

ldap/servers/plugins/replication/repl5.h | 1 ldap/servers/plugins/replication/repl5_agmt.c | 211 ++++++++++++-------------- 2 files changed, 101 insertions(+), 111 deletions(-) New commits: commit eb3086dcb0c56a23d6cee00a12f38b2584fe59a2 Author: Mark Reynolds <mreynolds(a)redhat.com> Date: Fri Jul 17 15:08:00 2015 -0400 Ticket 48179 - Starting a replica agreement can lead to deadlock Bug Description: When starting a replica agreement and setting the agmt maxcsn a deadlock can occur with another op updating nsuniqueid index. When setting the agmt maxcsn the server searches for the tombstone ruv which uses the nsuniqueid index, and it does this while holding the repl agmt lock. If another thread is doing a delete and writing to the change log, it can also grab a write lock on the nsuniqueid index, before it attempts to grab the agmt lock. This can lead to a deadlock if the timing is right. Fix Description: When starting the agmt and setting the agmt maxcsn, search/get the tombstone ruv before we take the repl agmt lock. https://fedorahosted.org/389/ticket/48179 Reviewed by: nhosoi(Thanks!) diff --git a/ldap/servers/plugins/replication/repl5.h b/ldap/servers/plugins/replication/repl5.h index 4a5d859..0b0f26b 100644 --- a/ldap/servers/plugins/replication/repl5.h +++ b/ldap/servers/plugins/replication/repl5.h @@ -380,7 +380,6 @@ PRUint64 agmt_get_protocol_timeout(Repl_Agmt *agmt); void agmt_set_protocol_timeout(Repl_Agmt *agmt, PRUint64 timeout); void agmt_update_maxcsn(Replica *r, Slapi_DN *sdn, int op, LDAPMod **mods, CSN *csn); void add_agmt_maxcsns(Slapi_Entry *e, Replica *r); -void agmt_set_maxcsn(Repl_Agmt *ra); void agmt_remove_maxcsn(Repl_Agmt *ra); int agmt_maxcsn_to_smod (Replica *r, Slapi_Mod *smod); int agmt_set_WaitForAsyncResults(Repl_Agmt *ra, const Slapi_Entry *e); diff --git a/ldap/servers/plugins/replication/repl5_agmt.c b/ldap/servers/plugins/replication/repl5_agmt.c index 9d1a8f2..f84eacb 100644 --- a/ldap/servers/plugins/replication/repl5_agmt.c +++ b/ldap/servers/plugins/replication/repl5_agmt.c @@ -668,43 +668,127 @@ int agmt_start(Repl_Agmt *ra) { Repl_Protocol *prot = NULL; + Slapi_PBlock *pb = NULL; + Slapi_Entry **entries = NULL; + Slapi_DN *repl_sdn = NULL; + char *attrs[2]; + int protocol_state; + int found_ruv = 0; + int rc = 0; - int protocol_state; - - /* To Allow Consumer Initialisation when adding an agreement: */ - if (ra->auto_initialize == STATE_PERFORMING_TOTAL_UPDATE) - { - protocol_state = STATE_PERFORMING_TOTAL_UPDATE; - } - else - { - protocol_state = STATE_PERFORMING_INCREMENTAL_UPDATE; - } + /* To Allow Consumer Initialisation when adding an agreement: */ + if (ra->auto_initialize == STATE_PERFORMING_TOTAL_UPDATE){ + protocol_state = STATE_PERFORMING_TOTAL_UPDATE; + } else { + protocol_state = STATE_PERFORMING_INCREMENTAL_UPDATE; + } /* First, create a new protocol object */ if ((prot = prot_new(ra, protocol_state)) == NULL) { return -1; } - /* Now it is safe to own the agreement lock */ + /* + * Set the agmt maxcsn + * + * We need to get the replica ruv before we take the + * agmt lock to avoid potential deadlocks on the nsuniqueid + * index. + */ + repl_sdn = agmt_get_replarea(ra); + + pb = slapi_pblock_new(); + attrs[0] = (char*)type_agmtMaxCSN; + attrs[1] = NULL; + slapi_search_internal_set_pb_ext( + pb, + repl_sdn, + LDAP_SCOPE_BASE, + "objectclass=*", + attrs, + 0, + NULL, + RUV_STORAGE_ENTRY_UNIQUEID, + repl_get_plugin_identity (PLUGIN_MULTIMASTER_REPLICATION), + OP_FLAG_REPLICATED); + slapi_search_internal_pb (pb); + + slapi_pblock_get(pb, SLAPI_PLUGIN_INTOP_RESULT, &rc); + if (rc == LDAP_SUCCESS){ + slapi_pblock_get(pb, SLAPI_PLUGIN_INTOP_SEARCH_ENTRIES, &entries); + if (NULL == entries || NULL == entries[0]){ + slapi_log_error(SLAPI_LOG_FATAL, repl_plugin_name, + "agmt_start: replica ruv tombstone entry for " + "replica %s not found\n", + slapi_sdn_get_dn(ra->replarea)); + } else { + found_ruv = 1; + } + } + + /* + * Now it is safe to own the agreement lock + */ PR_Lock(ra->lock); /* Check that replication is not already started */ if (ra->protocol != NULL) { slapi_log_error(SLAPI_LOG_REPL, repl_plugin_name, "replication already started for agreement \"%s\"\n", agmt_get_long_name(ra)); - PR_Unlock(ra->lock); prot_free(&prot); - return 0; + goto done; } + /* Set and start the protocol */ ra->protocol = prot; - - /* Start the protocol thread */ prot_start(ra->protocol); - agmt_set_maxcsn(ra); + /* + * If we found the repl ruv, set the agmt maxcsn... + */ + if (found_ruv){ + Replica *r; + Object *repl_obj; + char **maxcsns = NULL; + int i; + maxcsns = slapi_entry_attr_get_charray(entries[0], type_agmtMaxCSN); + repl_obj = prot_get_replica_object(ra->protocol); + if(repl_obj && maxcsns){ + r = (Replica *)object_get_data(repl_obj); + if(r){ + /* + * Loop over all the agmt maxcsns and find ours... + */ + for(i = 0; maxcsns[i]; i++){ + char buf[BUFSIZ]; + char unavail_buf[BUFSIZ]; + + PR_snprintf(buf,BUFSIZ,"%s;%s;%s;%d;",slapi_sdn_get_dn(repl_sdn), + slapi_rdn_get_value_by_ref(slapi_rdn_get_rdn(ra->rdn)), + ra->hostname, ra->port); + PR_snprintf(unavail_buf, BUFSIZ,"%s;%s;%s;%d;unavailable", slapi_sdn_get_dn(repl_sdn), + 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 */ + slapi_ch_free_string(&ra->maxcsn); + ra->maxcsn = slapi_ch_strdup(maxcsns[i]); + ra->consumerRID = agmt_maxcsn_get_rid(maxcsns[i]); + ra->tmpConsumerRID = 1; + break; + } + } + } + } + slapi_ch_array_free(maxcsns); + } + +done: PR_Unlock(ra->lock); + slapi_free_search_results_internal(pb); + slapi_pblock_destroy (pb); + slapi_sdn_free(&repl_sdn); + return 0; } @@ -3052,99 +3136,6 @@ agmt_maxcsn_to_smod (Replica *r, Slapi_Mod *smod) } /* - * Called when we start a repl agmt - */ -void -agmt_set_maxcsn(Repl_Agmt *ra) -{ - Slapi_PBlock *pb = NULL; - Slapi_Entry **entries = NULL; - Replica *r = NULL; - Object *repl_obj; - const Slapi_DN *tombstone_sdn = NULL; - char *attrs[2]; - int rc; - - /* read ruv state from the ruv tombstone entry */ - pb = slapi_pblock_new(); - if (!pb) { - slapi_log_error(SLAPI_LOG_FATAL, repl_plugin_name, "agmt_set_maxcsn: Out of memory\n"); - goto done; - } - repl_obj = prot_get_replica_object(ra->protocol); - if(repl_obj){ - r = (Replica *)object_get_data(repl_obj); - tombstone_sdn = replica_get_root(r); - } - ra->maxcsn = NULL; - attrs[0] = (char*)type_agmtMaxCSN; - attrs[1] = NULL; - slapi_search_internal_set_pb_ext( - pb, - (Slapi_DN *)tombstone_sdn, - LDAP_SCOPE_BASE, - "objectclass=*", - attrs, - 0, /* attrsonly */ - NULL, /* controls */ - RUV_STORAGE_ENTRY_UNIQUEID, - repl_get_plugin_identity (PLUGIN_MULTIMASTER_REPLICATION), - OP_FLAG_REPLICATED); /* flags */ - slapi_search_internal_pb (pb); - - slapi_pblock_get(pb, SLAPI_PLUGIN_INTOP_RESULT, &rc); - if (rc == LDAP_SUCCESS){ - Replica *r; - Object *repl_obj; - char **maxcsns; - int i; - - slapi_pblock_get(pb, SLAPI_PLUGIN_INTOP_SEARCH_ENTRIES, &entries); - if (NULL == entries || NULL == entries[0]){ - slapi_log_error(SLAPI_LOG_FATAL, repl_plugin_name, - "agmt_set_maxcsn: replica ruv tombstone entry for " - "replica %s not found\n", - slapi_sdn_get_dn(ra->replarea)); - goto done; - } - maxcsns = slapi_entry_attr_get_charray(entries[0], type_agmtMaxCSN); - repl_obj = prot_get_replica_object(ra->protocol); - if(repl_obj && maxcsns){ - r = (Replica *)object_get_data(repl_obj); - if(r){ - /* - * Loop over all the agmt maxcsns and find ours - */ - for(i = 0; maxcsns[i]; i++){ - char buf[BUFSIZ]; - char unavail_buf[BUFSIZ]; - - PR_snprintf(buf,BUFSIZ,"%s;%s;%s;%d;",slapi_sdn_get_dn(ra->replarea), - slapi_rdn_get_value_by_ref(slapi_rdn_get_rdn(ra->rdn)), - ra->hostname, ra->port); - PR_snprintf(unavail_buf, BUFSIZ,"%s;%s;%s;%d;unavailable", slapi_sdn_get_dn(ra->replarea), - 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)){ - slapi_ch_free_string(&ra->maxcsn); - ra->maxcsn = slapi_ch_strdup(maxcsns[i]); - ra->consumerRID = agmt_maxcsn_get_rid(maxcsns[i]); - ra->tmpConsumerRID = 1; - break; - } - } - } - } - slapi_ch_array_free(maxcsns); - } -done: - if (NULL != pb){ - slapi_free_search_results_internal(pb); - slapi_pblock_destroy (pb); - } -} - -/* * Parse out the consumer replicaID from the agmt maxcsn * * "repl area;agmt_rdn;hostname;port;consumer_rid;maxcsn"

1 0

Branch '389-ds-base-1.3.3' - 2 commits - dirsrvtests/tickets ldap/servers
by Noriko Hosoi 16 Jul '15

16 Jul '15

dirsrvtests/tickets/ticket48226_test.py | 239 ++++++++++++++++++++++++++++++++ ldap/servers/slapd/valueset.c | 5 2 files changed, 242 insertions(+), 2 deletions(-) New commits: commit dfcfa5521e44604991c48808ff613394712b34ee Author: Noriko Hosoi <nhosoi(a)redhat.com> Date: Thu Jul 16 10:41:53 2015 -0700 Ticket #48226 - CI test: added test cases for ticket 48226 Description: In MMR, double free coould occur under some special condition This test script was written by thierry bordaz <tbordaz(a)redhat.com>. A small modification to check the memory leak was added. (cherry picked from commit f5d24450477f8341261c3e5cb5c54ec1ab83328f) (cherry picked from commit 8600a5eabc78848ad1bf0a9c2014823d0cd6cedc) diff --git a/dirsrvtests/tickets/ticket48226_test.py b/dirsrvtests/tickets/ticket48226_test.py new file mode 100644 index 0000000..87814e7 --- /dev/null +++ b/dirsrvtests/tickets/ticket48226_test.py @@ -0,0 +1,239 @@ +# --- BEGIN COPYRIGHT BLOCK --- +# Copyright (C) 2015 Red Hat, Inc. +# All rights reserved. +# +# License: GPL (version 3 or any later version). +# See LICENSE for details. +# --- END COPYRIGHT BLOCK --- +# +import os +import sys +import time +import ldap +import logging +import pytest +from lib389 import DirSrv, Entry, tools, tasks +from lib389.tools import DirSrvTools +from lib389._constants import * +from lib389.properties import * +from lib389.tasks import * +from lib389.utils import * + +logging.getLogger(__name__).setLevel(logging.DEBUG) +log = logging.getLogger(__name__) + +installation1_prefix = None + + +class TopologyReplication(object): + def __init__(self, master1, master2): + master1.open() + self.master1 = master1 + master2.open() + self.master2 = master2 + + +(a)pytest.fixture(scope="module") +def topology(request): + global installation1_prefix + os.environ['USE_VALGRIND'] = '1' + if installation1_prefix: + args_instance[SER_DEPLOYED_DIR] = installation1_prefix + + # Creating master 1... + master1 = DirSrv(verbose=False) + if installation1_prefix: + args_instance[SER_DEPLOYED_DIR] = installation1_prefix + args_instance[SER_HOST] = HOST_MASTER_1 + args_instance[SER_PORT] = PORT_MASTER_1 + args_instance[SER_SERVERID_PROP] = SERVERID_MASTER_1 + args_instance[SER_CREATION_SUFFIX] = DEFAULT_SUFFIX + args_master = args_instance.copy() + master1.allocate(args_master) + instance_master1 = master1.exists() + if instance_master1: + master1.delete() + master1.create() + master1.open() + master1.replica.enableReplication(suffix=SUFFIX, role=REPLICAROLE_MASTER, replicaId=REPLICAID_MASTER_1) + + # Creating master 2... + master2 = DirSrv(verbose=False) + if installation1_prefix: + args_instance[SER_DEPLOYED_DIR] = installation1_prefix + args_instance[SER_HOST] = HOST_MASTER_2 + args_instance[SER_PORT] = PORT_MASTER_2 + args_instance[SER_SERVERID_PROP] = SERVERID_MASTER_2 + args_instance[SER_CREATION_SUFFIX] = DEFAULT_SUFFIX + args_master = args_instance.copy() + master2.allocate(args_master) + instance_master2 = master2.exists() + if instance_master2: + master2.delete() + master2.create() + master2.open() + master2.replica.enableReplication(suffix=SUFFIX, role=REPLICAROLE_MASTER, replicaId=REPLICAID_MASTER_2) + + # + # Create all the agreements + # + # Creating agreement from master 1 to master 2 + properties = {RA_NAME: r'meTo_$host:$port', + RA_BINDDN: defaultProperties[REPLICATION_BIND_DN], + RA_BINDPW: defaultProperties[REPLICATION_BIND_PW], + RA_METHOD: defaultProperties[REPLICATION_BIND_METHOD], + RA_TRANSPORT_PROT: defaultProperties[REPLICATION_TRANSPORT]} + m1_m2_agmt = master1.agreement.create(suffix=SUFFIX, host=master2.host, port=master2.port, properties=properties) + if not m1_m2_agmt: + log.fatal("Fail to create a master -> master replica agreement") + sys.exit(1) + log.debug("%s created" % m1_m2_agmt) + + # Creating agreement from master 2 to master 1 + properties = {RA_NAME: r'meTo_$host:$port', + RA_BINDDN: defaultProperties[REPLICATION_BIND_DN], + RA_BINDPW: defaultProperties[REPLICATION_BIND_PW], + RA_METHOD: defaultProperties[REPLICATION_BIND_METHOD], + RA_TRANSPORT_PROT: defaultProperties[REPLICATION_TRANSPORT]} + m2_m1_agmt = master2.agreement.create(suffix=SUFFIX, host=master1.host, port=master1.port, properties=properties) + if not m2_m1_agmt: + log.fatal("Fail to create a master -> master replica agreement") + sys.exit(1) + log.debug("%s created" % m2_m1_agmt) + + # Allow the replicas to get situated with the new agreements... + time.sleep(5) + + # + # Initialize all the agreements + # + master1.agreement.init(SUFFIX, HOST_MASTER_2, PORT_MASTER_2) + master1.waitForReplInit(m1_m2_agmt) + + # Check replication is working... + if master1.testReplication(DEFAULT_SUFFIX, master2): + log.info('Replication is working.') + else: + log.fatal('Replication is not working.') + assert False + + # Clear out the tmp dir + master1.clearTmpDir(__file__) + + return TopologyReplication(master1, master2) + +def test_ticket11111_set_purgedelay(topology): + args = {REPLICA_PURGE_DELAY: '5', + REPLICA_PURGE_INTERVAL: '5'} + try: + topology.master1.replica.setProperties(DEFAULT_SUFFIX, None, None, args) + except: + log.fatal('Failed to configure replica') + assert False + try: + topology.master2.replica.setProperties(DEFAULT_SUFFIX, None, None, args) + except: + log.fatal('Failed to configure replica') + assert False + topology.master1.modify_s(DN_CONFIG, [(ldap.MOD_REPLACE, 'nsslapd-auditlog-logging-enabled', 'on')]) + topology.master2.modify_s(DN_CONFIG, [(ldap.MOD_REPLACE, 'nsslapd-auditlog-logging-enabled', 'on')]) + topology.master1.restart(10) + topology.master2.restart(10) + + +def test_ticket11111_1(topology): + name = 'test_entry' + dn = "cn=%s,%s" % (name, SUFFIX) + + topology.master1.add_s(Entry((dn , { + 'objectclass': "top person".split(), + 'sn': name, + 'cn': name}))) + + # First do an update that is replicated + mods = [(ldap.MOD_ADD, 'description', '5')] + topology.master1.modify_s(dn, mods) + + nbtry = 0 + while (nbtry <= 10): + try: + ent = topology.master2.getEntry(dn, ldap.SCOPE_BASE, "(objectclass=*)", ['description']) + if ent.hasAttr('description') and ent.getValue('description') == '5': + break + except ldap.NO_SUCH_OBJECT: + pass + nbtry = nbtry + 1 + time.sleep(1) + assert nbtry <= 10 + + # Stop M2 so that it will not receive the next update + topology.master2.stop(10) + + # ADD a new value that is not replicated + mods = [(ldap.MOD_DELETE, 'description', '5')] + topology.master1.modify_s(dn, mods) + + # Stop M1 so that it will keep del '5' that is unknown from master2 + topology.master1.stop(10) + + # Get the sbin directory so we know where to replace 'ns-slapd' + sbin_dir = get_sbin_dir(prefix=topology.master2.prefix) + + # Enable valgrind + valgrind_enable(sbin_dir) + + # start M2 to do the next updates + topology.master2.start(10) + + # ADD 'description' by '5' + mods = [(ldap.MOD_DELETE, 'description', '5')] + topology.master2.modify_s(dn, mods) + + # DEL 'description' by '5' + mods = [(ldap.MOD_ADD, 'description', '5')] + topology.master2.modify_s(dn, mods) + + # sleep of purgedelay so that the next update will purge the CSN_7 + time.sleep(6) + + # ADD 'description' by '8' that purge the state info + mods = [(ldap.MOD_ADD, 'description', '6')] + topology.master2.modify_s(dn, mods) + + if valgrind_check_leak(topology.master2, 'csnset_dup'): + log.error('test_csnset_dup: Memory leak is present!') + else: + log.info('test_csnset_dup: No leak is present!') + + if valgrind_check_leak(topology.master2, 'Invalid'): + log.info('Valgrind reported invalid!') + else: + log.info('Valgrind is happy!') + + #log.info("You can attach yourself") + #time.sleep(60) + + # Enable valgrind + valgrind_disable(sbin_dir) + + topology.master1.start(10) + + +def test_ticket11111_final(topology): + topology.master1.delete() + topology.master2.delete() + log.info('Testcase PASSED') + + +def run_isolated(): + global installation1_prefix + installation1_prefix = None + + topo = topology(True) + test_ticket11111_set_purgedelay(topo) + test_ticket11111_1(topo) + + +if __name__ == '__main__': + run_isolated() + commit de1b027c39e6b80bdfdf02f1a12854587730aff8 Author: Noriko Hosoi <nhosoi(a)redhat.com> Date: Thu Jul 16 10:34:47 2015 -0700 Ticket #48226 - In MMR, double free coould occur under some special condition Bug description: In a replicated topology, a authenticated user that have write access on an entry can send a series of operations that crash the server. The crash is due to an access to a already freed buffer. Fix description: To avoid the double free, duplicate a CSNSet and assign it to the Slapi_Value. https://fedorahosted.org/389/ticket/48226 Reviewed by rmeggins(a)redhat.com (Thank you, Rich!!) (cherry picked from commit a0f8e0f981a046882db299a7a6d6d1c01bc19571) (cherry picked from commit bdbc81e62eb8d7b8dfb298c7ba983cf86353fe66) diff --git a/ldap/servers/slapd/valueset.c b/ldap/servers/slapd/valueset.c index 9d77b0c..fb7a99b 100644 --- a/ldap/servers/slapd/valueset.c +++ b/ldap/servers/slapd/valueset.c @@ -1444,8 +1444,9 @@ valueset_update_csn_for_valuearray_ext(Slapi_ValueSet *vs, const Slapi_Attr *a, if(v) { value_update_csn(v,t,csn); - if (csnref_updated) - valuestoupdate[i]->v_csnset = (CSNSet *)value_get_csnset(v); + if (csnref_updated) { + valuestoupdate[i]->v_csnset = csnset_dup(value_get_csnset(v)); + } valuearrayfast_add_value_passin(&vaf_valuesupdated,valuestoupdate[i]); valuestoupdate[i]= NULL; del_count++;

1 0

← Newer
1
2
3
4
5
6
7
8
9
Older →

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

389-commits July 2015