From 233b64f26df76aa50f4b37aaf6b3804d208fdc1b Mon Sep 17 00:00:00 2001
From: Ludwig Krispenz <lkrispen@redhat.com>
Date: Mon, 12 Feb 2018 09:24:25 +0100
Subject: [PATCH] Ticket 49551 - v3 - correct handling of numsubordinates for
cenotaphs and tombstone delete
Bug: The ticket exposed several problems with tombstone handling.
- tombstone entries of conflicts were not purged in tombstone purging
- cenotaphs are tombstone, but the subordinate count was not managed properly
- direct delete of tombstones failed with err=1
- delete of entry with only conflict children failed correctly, but gave no hint why
Fix: update the correct numsobordinates attribut for cenotaphs
set proper flag in directly deleting a tombstone
change search filter for tombstone purging to include ldapsubentries
check for conflict children if a delete is rejected and add a message to the response
Reviewed by; Thierry, William - thanks
---
ldap/servers/plugins/replication/repl5_replica.c | 14 +++++++++--
ldap/servers/plugins/replication/urp.c | 8 +++---
ldap/servers/slapd/back-ldbm/ldbm_add.c | 8 +++---
ldap/servers/slapd/back-ldbm/ldbm_delete.c | 14 ++++++++---
ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c | 3 ++-
ldap/servers/slapd/back-ldbm/parents.c | 12 ++++++---
ldap/servers/slapd/entry.c | 31 ++++++++++++++++++++++++
ldap/servers/slapd/slapi-plugin.h | 2 ++
ldap/servers/slapd/slapi-private.h | 1 +
ldap/servers/slapd/task.c | 4 +--
10 files changed, 78 insertions(+), 19 deletions(-)
diff --git a/ldap/servers/plugins/replication/repl5_replica.c b/ldap/servers/plugins/replication/repl5_replica.c
index bdb8a5167..628fb9ceb 100644
--- a/ldap/servers/plugins/replication/repl5_replica.c
+++ b/ldap/servers/plugins/replication/repl5_replica.c
@@ -3017,6 +3017,16 @@ process_reap_entry(Slapi_Entry *entry, void *cb_data)
search in the future, see _replica_reap_tombstones below and add more to the
attrs array */
deletion_csn = entry_get_deletion_csn(entry);
+ if (deletion_csn == NULL) {
+ /* this might be a tombstone which was directly added, eg a cenotaph
+ * check if a tombstonecsn exist and use it
+ */
+ char *tombstonecsn = slapi_entry_attr_get_charptr(entry, SLAPI_ATTR_TOMBSTONE_CSN);
+ if (tombstonecsn) {
+ deletion_csn = csn_new_by_string(tombstonecsn);
+ slapi_ch_free_string(&tombstonecsn);
+ }
+ }
if ((NULL == deletion_csn || csn_compare(deletion_csn, purge_csn) < 0) &&
(!is_ruv_tombstone_entry(entry))) {
@@ -3116,11 +3126,11 @@ _replica_reap_tombstones(void *arg)
*/
csn_as_string(purge_csn, PR_FALSE, deletion_csn_str);
PR_snprintf(tombstone_filter, 128,
- "(&(%s<=%s)(objectclass=nsTombstone))", SLAPI_ATTR_TOMBSTONE_CSN,
+ "(&(%s<=%s)(objectclass=nsTombstone)(|(objectclass=*)(objectclass=ldapsubentry)))", SLAPI_ATTR_TOMBSTONE_CSN,
csn_as_string(purge_csn, PR_FALSE, deletion_csn_str));
} else {
/* Use the old inefficient filter */
- PR_snprintf(tombstone_filter, 128, "(objectclass=nsTombstone)");
+ PR_snprintf(tombstone_filter, 128, "(&(objectclass=nsTombstone)(|(objectclass=*)(objectclass=ldapsubentry)))");
}
/* we just need the objectclass - for the deletion csn
diff --git a/ldap/servers/plugins/replication/urp.c b/ldap/servers/plugins/replication/urp.c
index d4556d7fd..11c5da7cf 100644
--- a/ldap/servers/plugins/replication/urp.c
+++ b/ldap/servers/plugins/replication/urp.c
@@ -911,7 +911,7 @@ urp_fixup_add_cenotaph (Slapi_PBlock *pb, char *sessionid, CSN *opcsn)
cenotaph,
NULL,
repl_get_plugin_identity(PLUGIN_MULTIMASTER_REPLICATION),
- OP_FLAG_REPL_FIXUP|OP_FLAG_NOOP);
+ OP_FLAG_REPL_FIXUP|OP_FLAG_NOOP|OP_FLAG_CENOTAPH_ENTRY);
slapi_add_internal_pb(add_pb);
slapi_pblock_get(add_pb, SLAPI_PLUGIN_INTOP_RESULT, &ret);
@@ -1922,7 +1922,7 @@ done:
newpb = NULL;
slapi_log_err(SLAPI_LOG_REPL, sessionid,
- "urp_get_min_naming_conflict_entry - Found %d entries\n", i);
+ "urp_get_min_naming_conflict_entry - Found %d entries\n", min_csn?1:0);
return min_naming_conflict_entry;
}
@@ -2172,8 +2172,8 @@ mod_objectclass_attr(const char *uniqueid, const Slapi_DN *entrysdn, const Slapi
char csnstr[CSN_STRSIZE+1] = {0};
slapi_mods_init(&smods, 3);
- slapi_mods_add(&smods, LDAP_MOD_ADD, "objectclass", strlen("ldapsubentry"),"ldapsubentry");
- slapi_mods_add(&smods, LDAP_MOD_REPLACE, "conflictcsn", CSN_STRSIZE, csn_as_string(opcsn, PR_FALSE, csnstr));
+ slapi_mods_add_string(&smods, LDAP_MOD_ADD, "objectclass", "ldapsubentry");
+ slapi_mods_add_string(&smods, LDAP_MOD_REPLACE, "conflictcsn", csn_as_string(opcsn, PR_FALSE, csnstr));
op_result = urp_fixup_modify_entry(uniqueid, entrysdn, opcsn, &smods, 0);
slapi_mods_done(&smods);
if (op_result == LDAP_TYPE_OR_VALUE_EXISTS) {
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_add.c b/ldap/servers/slapd/back-ldbm/ldbm_add.c
index c93d44a65..f0a3262ec 100644
--- a/ldap/servers/slapd/back-ldbm/ldbm_add.c
+++ b/ldap/servers/slapd/back-ldbm/ldbm_add.c
@@ -81,6 +81,7 @@ ldbm_back_add(Slapi_PBlock *pb)
Slapi_Operation *operation;
int is_replicated_operation = 0;
int is_resurect_operation = 0;
+ int is_cenotaph_operation = 0;
int is_tombstone_operation = 0;
int is_fixup_operation = 0;
int is_remove_from_cache = 0;
@@ -116,6 +117,7 @@ ldbm_back_add(Slapi_PBlock *pb)
}
is_resurect_operation = operation_is_flag_set(operation, OP_FLAG_RESURECT_ENTRY);
+ is_cenotaph_operation = operation_is_flag_set(operation, OP_FLAG_CENOTAPH_ENTRY);
is_tombstone_operation = operation_is_flag_set(operation, OP_FLAG_TOMBSTONE_ENTRY);
is_fixup_operation = operation_is_flag_set(operation, OP_FLAG_REPL_FIXUP);
is_ruv = operation_is_flag_set(operation, OP_FLAG_REPL_RUV);
@@ -846,9 +848,9 @@ ldbm_back_add(Slapi_PBlock *pb)
the in-memory state of the parent to reflect the new child (update
subordinate count specifically */
if (parententry) {
- retval = parent_update_on_childchange(&parent_modify_c,
- is_resurect_operation ? PARENTUPDATE_RESURECT : PARENTUPDATE_ADD,
- NULL);
+ int op = is_resurect_operation ? PARENTUPDATE_RESURECT : PARENTUPDATE_ADD;
+ if (is_cenotaph_operation ) op |= PARENTUPDATE_CREATE_TOMBSTONE;
+ retval = parent_update_on_childchange(&parent_modify_c, op, NULL);
slapi_log_err(SLAPI_LOG_BACKLDBM, "ldbm_back_add",
"conn=%lu op=%d parent_update_on_childchange: old_entry=0x%p, new_entry=0x%p, rc=%d\n",
conn_id, op_id, parent_modify_c.old_entry, parent_modify_c.new_entry, retval);
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_delete.c b/ldap/servers/slapd/back-ldbm/ldbm_delete.c
index be0db1bd0..bc0a3654e 100644
--- a/ldap/servers/slapd/back-ldbm/ldbm_delete.c
+++ b/ldap/servers/slapd/back-ldbm/ldbm_delete.c
@@ -291,9 +291,16 @@ replace_entry:
retval = slapi_entry_has_children(e->ep_entry);
if (retval && !is_replicated_operation) {
ldap_result_code= LDAP_NOT_ALLOWED_ON_NONLEAF;
- slapi_log_err(SLAPI_LOG_BACKLDBM, "ldbm_back_delete",
- "conn=%lu op=%d Deleting entry %s has %d children.\n",
- conn_id, op_id, slapi_entry_get_dn(e->ep_entry), retval);
+ if (slapi_entry_has_conflict_children(e->ep_entry, (void *)li->li_identity) > 0) {
+ ldap_result_message = "Entry has replication conflicts as children";
+ slapi_log_err(SLAPI_LOG_ERR, "ldbm_back_delete",
+ "conn=%lu op=%d Deleting entry %s has replication conflicts as children.\n",
+ conn_id, op_id, slapi_entry_get_dn(e->ep_entry));
+ } else {
+ slapi_log_err(SLAPI_LOG_BACKLDBM, "ldbm_back_delete",
+ "conn=%lu op=%d Deleting entry %s has %d children.\n",
+ conn_id, op_id, slapi_entry_get_dn(e->ep_entry), retval);
+ }
retval = -1;
goto error_return;
}
@@ -431,6 +438,7 @@ replace_entry:
slapi_log_err(SLAPI_LOG_WARNING, "ldbm_back_delete",
"Attempt to Tombstone again a tombstone entry %s\n", dn);
delete_tombstone_entry = 1;
+ operation_set_flag(operation, OP_FLAG_TOMBSTONE_ENTRY);
}
}
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c b/ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c
index b41a2d241..5797dd779 100644
--- a/ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c
+++ b/ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c
@@ -2824,7 +2824,8 @@ _entryrdn_delete_key(backend *be,
break;
}
childelem = (rdn_elem *)dataret.data;
- if (!slapi_is_special_rdn(childelem->rdn_elem_nrdn_rdn, RDN_IS_TOMBSTONE)) {
+ if (!slapi_is_special_rdn(childelem->rdn_elem_nrdn_rdn, RDN_IS_TOMBSTONE) &&
+ !strcasestr(childelem->rdn_elem_nrdn_rdn, "cenotaphid")) {
/* there's at least one live child */
slapi_log_err(SLAPI_LOG_ERR, "_entryrdn_delete_key",
"Failed to remove %s; has a child %s\n", nrdn,
diff --git a/ldap/servers/slapd/back-ldbm/parents.c b/ldap/servers/slapd/back-ldbm/parents.c
index 79e66451e..1afc795c0 100644
--- a/ldap/servers/slapd/back-ldbm/parents.c
+++ b/ldap/servers/slapd/back-ldbm/parents.c
@@ -89,7 +89,11 @@ parent_update_on_childchange(modify_context *mc, int op, size_t *new_sub_count)
}
}
- if (PARENTUPDATE_DELETE_TOMBSTONE != repl_op) {
+ if ((PARENTUPDATE_ADD == op) && (PARENTUPDATE_CREATE_TOMBSTONE == repl_op)) {
+ /* we are directly adding a tombstone entry, only need to
+ * update the tombstone subordinates
+ */
+ } else if (PARENTUPDATE_DELETE_TOMBSTONE != repl_op) {
/* are we adding ? */
if (((PARENTUPDATE_ADD == op) || (PARENTUPDATE_RESURECT == op)) && !already_present) {
/* If so, and the parent entry does not already have a subcount
@@ -136,10 +140,10 @@ parent_update_on_childchange(modify_context *mc, int op, size_t *new_sub_count)
}
}
- /* tombstoneNumSubordinates is needed only when this is repl op
- * and a child is being deleted */
+ /* tombstoneNumSubordinates has to be updated if a tombstone child has been
+ * deleted or a tombstone has been directly added (cenotaph) */
current_sub_count = LDAP_MAXINT;
- if ((repl_op && (PARENTUPDATE_DEL == op)) || (PARENTUPDATE_RESURECT == op)) {
+ if (repl_op) {
ret = slapi_entry_attr_find(mc->old_entry->ep_entry,
tombstone_numsubordinates, &read_attr);
if (0 == ret) {
diff --git a/ldap/servers/slapd/entry.c b/ldap/servers/slapd/entry.c
index 32828b4e2..b85e9f5b0 100644
--- a/ldap/servers/slapd/entry.c
+++ b/ldap/servers/slapd/entry.c
@@ -3238,6 +3238,37 @@ slapi_entry_has_children(const Slapi_Entry *entry)
return slapi_entry_has_children_ext(entry, 0);
}
+int
+slapi_entry_has_conflict_children(const Slapi_Entry *entry, void *plg_id)
+{
+ Slapi_PBlock *search_pb = NULL;
+ Slapi_Entry **entries;
+ int rc = 0;
+
+ search_pb = slapi_pblock_new();
+ slapi_search_internal_set_pb(search_pb, slapi_entry_get_dn_const(entry),
+ LDAP_SCOPE_ONELEVEL,
+ "(&(objectclass=ldapsubentry)(nsds5ReplConflict=namingConflict*))",
+ NULL, 0, NULL, NULL, plg_id, 0);
+ slapi_search_internal_pb(search_pb);
+ slapi_pblock_get(search_pb, SLAPI_PLUGIN_INTOP_RESULT, &rc);
+ if (rc) {
+ rc = -1;
+ } else {
+ slapi_pblock_get(search_pb, SLAPI_PLUGIN_INTOP_SEARCH_ENTRIES, &entries);
+ if (entries && entries[0]) {
+ /* we found at least one conflict entry */
+ rc = 1;
+ } else {
+ rc = 0;
+ }
+ slapi_free_search_results_internal(search_pb);
+ }
+ slapi_pblock_destroy(search_pb);
+
+ return rc;
+}
+
/*
* Renames an entry to simulate a MODRDN operation
*/
diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h
index 95cdcc0da..6978e258f 100644
--- a/ldap/servers/slapd/slapi-plugin.h
+++ b/ldap/servers/slapd/slapi-plugin.h
@@ -2000,6 +2000,8 @@ int slapi_entry_has_children(const Slapi_Entry *e);
*/
int slapi_entry_has_children_ext(const Slapi_Entry *e, int include_tombstone);
+int slapi_entry_has_conflict_children(const Slapi_Entry *e, void *plg_id);
+
/**
* This function determines if an entry is the root DSE.
*
diff --git a/ldap/servers/slapd/slapi-private.h b/ldap/servers/slapd/slapi-private.h
index 548d5cabb..b08c0d7ce 100644
--- a/ldap/servers/slapd/slapi-private.h
+++ b/ldap/servers/slapd/slapi-private.h
@@ -403,6 +403,7 @@ char *slapi_filter_to_string_internal(const struct slapi_filter *f, char *buf, s
#define OP_FLAG_NEVER_CHAIN SLAPI_OP_FLAG_NEVER_CHAIN /* 0x000800 */
#define OP_FLAG_TOMBSTONE_ENTRY SLAPI_OP_FLAG_TOMBSTONE_ENTRY /* 0x001000 */
#define OP_FLAG_RESURECT_ENTRY 0x002000
+#define OP_FLAG_CENOTAPH_ENTRY 0x004000
#define OP_FLAG_ACTION_NOLOG 0x008000 /* Do not log the entry in \
* audit log or change log \
*/
diff --git a/ldap/servers/slapd/task.c b/ldap/servers/slapd/task.c
index 4bd8895ff..3f9d5d995 100644
--- a/ldap/servers/slapd/task.c
+++ b/ldap/servers/slapd/task.c
@@ -2352,10 +2352,10 @@ task_fixup_tombstone_thread(void *arg)
if (task_data->stripcsn) {
/* find tombstones with nsTombstoneCSN */
- filter = "(&(nstombstonecsn=*)(objectclass=nsTombstone))";
+ filter = "(&(nstombstonecsn=*)(objectclass=nsTombstone)(|(objectclass=*)(objectclass=ldapsubentry)))";
} else {
/* find tombstones missing nsTombstoneCSN */
- filter = "(&(!(nstombstonecsn=*))(objectclass=nsTombstone))";
+ filter = "(&(!(nstombstonecsn=*))(objectclass=nsTombstone)(|(objectclass=*)(objectclass=ldapsubentry)))";
}
/* Okay check the specified backends only */
--
2.13.6