Blob Blame Raw
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