From b6220eb610c92ead1a7e789317e158cfcb8def90 Mon Sep 17 00:00:00 2001

From: Noriko Hosoi <nhosoi@redhat.com>

Date: Thu, 6 Jun 2013 16:52:14 -0700

Subject: [PATCH 54/99] Ticket #47367 - (phase 1) ldapdelete returns non-leaf

 entry error while trying to remove a leaf entry

Bug description: Replication conflict confuses the numsubordinate

count, which leaves an entry that cannot be deleted even its

subordinate entries are all removed.

Fix description:

[urp.c] get_dn_plus_uniqueid: a logic to create a conflict DN

  had a bug.  It used to call slapi_sdn_get_rdn to get the rdn.

  The function slapi_sdn_get_rdn blindly returned the "dn" field

  without checking whether the field is NULL or not.  Instead,

  this patch changes the interface of the helper function get_

  dn_plus_uniqueid and use the original Slapi_DN with slapi_

  sdn_get_dn, then generates the conflict DN "nsuniqueid=...+

  <RDN>,<PARENT>".

[ldbm_delete.c] There is a case a parent of a delete-candidate

  entry runs into a conflict and multiple parent entries exist.

  Once it occurs, a parent entry found by the parent dn string

  may not be the entry which manages the numsubordinate count

  the delete-candidate entry belonging to.  It confuses the

  numsubordinate counts and leaves an entry which cannot be

  deleted due to the numsubordinate count mismatch. This patch

  retrieves parent entry by parent id if it is available.

[ldbm_entryrdn.c] When traversing the DIT, a special treatment

  is needed for a tombstone entry. I.e, 2 RDNs (nsuniqueid=...,

  <RDN>) is treated as one RDN.  It should decrement the index

  (rdnidx) one more to point to the right position of the RDN

  array in Slapi_RDN.

[ldbm_search.c] When checking the scope of an entry in ldbm_

  back_next_search_entry_ext, a tombstone entry was not properly

  examined.  This patch introduces a new slapi api slapi_sdn_

  scope_test_ext.

[dn.c] In slapi_sdn_get_rdn, use slapi_sdn_get_dn to get the

  dn value of Slapi_DN.  It was one cause of the problem in

  get_dn_plus_uniqueid (urp.c).

  This patch adds slapi_sdn_scope_test_ext, which takes flags

  to indicates the first argument dn is a tombstone sdn.

  Also, this patch replaces "malloc + strcpy + strcat" with

  slapi_ch_smprintf to improve the readability of the code.

[rdn.c] This patch replaces "malloc + strcpy + strcat" with

  slapi_create_dn_string to normalize the newly added rdn and

  improve the readability of the code.

Reviewed by Rich (Thank you!!)

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

(cherry picked from commit c2c64017e0e25a4376139debe29b4f587964710f)

---

 ldap/servers/plugins/replication/urp.c       | 18 ++++-----

 ldap/servers/slapd/back-ldbm/ldbm_delete.c   | 35 ++++++++++++-----

 ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c |  2 +

 ldap/servers/slapd/back-ldbm/ldbm_search.c   |  2 +-

 ldap/servers/slapd/dn.c                      | 59 +++++++++++++++++++++++++---

 ldap/servers/slapd/rdn.c                     | 24 ++++-------

 ldap/servers/slapd/slapi-plugin.h            | 18 +++++++++

 7 files changed, 116 insertions(+), 42 deletions(-)

diff --git a/ldap/servers/plugins/replication/urp.c b/ldap/servers/plugins/replication/urp.c

index 1d8799a..e236541 100644

--- a/ldap/servers/plugins/replication/urp.c

+++ b/ldap/servers/plugins/replication/urp.c

@@ -57,7 +57,7 @@ static int urp_annotate_dn (char *sessionid, Slapi_Entry *entry, CSN *opcsn, con

 static int urp_naming_conflict_removal (Slapi_PBlock *pb, char *sessionid, CSN *opcsn, const char *optype);

 static int mod_namingconflict_attr (const char *uniqueid, const Slapi_DN *entrysdn, const Slapi_DN *conflictsdn, CSN *opcsn);

 static int del_replconflict_attr (Slapi_Entry *entry, CSN *opcsn, int opflags);

-static char *get_dn_plus_uniqueid(char *sessionid,const char *olddn,const char *uniqueid);

+static char *get_dn_plus_uniqueid(char *sessionid,const Slapi_DN *oldsdn,const char *uniqueid);

 static char *get_rdn_plus_uniqueid(char *sessionid,const char *olddn,const char *uniqueid);

 static int is_suffix_entry (Slapi_PBlock *pb, Slapi_Entry *entry, Slapi_DN **parenddn);

@@ -180,7 +180,7 @@ urp_add_operation( Slapi_PBlock *pb )

 	if (r<0)

 	{

 		/* Entry to be added is a loser */

-		char *newdn= get_dn_plus_uniqueid (sessionid, basedn, adduniqueid);

+		char *newdn = get_dn_plus_uniqueid (sessionid, (const Slapi_DN *)addentry, adduniqueid);

 		if(newdn==NULL)

 		{

 			op_result= LDAP_OPERATIONS_ERROR;

@@ -1222,16 +1222,15 @@ bailout:

 /* The returned value is either null or "uniqueid=<uniqueid>+<basedn>" */

 static char *

-get_dn_plus_uniqueid(char *sessionid, const char *olddn, const char *uniqueid)

+get_dn_plus_uniqueid(char *sessionid, const Slapi_DN *oldsdn, const char *uniqueid)

 {

-	Slapi_DN *sdn= slapi_sdn_new_dn_byval(olddn);

 	Slapi_RDN *rdn= slapi_rdn_new();

 	char *newdn;

 	PR_ASSERT(uniqueid!=NULL);

 	/* Check if the RDN already contains the Unique ID */

-	slapi_sdn_get_rdn(sdn,rdn);

+	slapi_rdn_set_dn(rdn, slapi_sdn_get_dn(oldsdn));

 	if(slapi_rdn_contains(rdn,SLAPI_ATTR_UNIQUEID,uniqueid,strlen(uniqueid)))

 	{

 		/* The Unique ID is already in the RDN.

@@ -1241,16 +1240,15 @@ get_dn_plus_uniqueid(char *sessionid, const char *olddn, const char *uniqueid)

 		 * require admin intercession

 		 */

 		slapi_log_error(SLAPI_LOG_FATAL, sessionid,

-				"Annotated DN %s has naming conflict\n", olddn );

+				"Annotated DN %s has naming conflict\n", slapi_sdn_get_dn(oldsdn) );

 		newdn= NULL;

 	}

 	else

 	{

-		slapi_rdn_add(rdn,SLAPI_ATTR_UNIQUEID,uniqueid);

-		slapi_sdn_set_rdn(sdn, rdn);

-		newdn= slapi_ch_strdup(slapi_sdn_get_dn(sdn));

+		char *parentdn = slapi_dn_parent(slapi_sdn_get_dn(oldsdn));

+		slapi_rdn_add(rdn, SLAPI_ATTR_UNIQUEID, uniqueid);

+		newdn = slapi_ch_smprintf("%s,%s", slapi_rdn_get_rdn(rdn), parentdn);

 	}

-	slapi_sdn_free(&sdn;;

 	slapi_rdn_free(&rdn;;

 	return newdn;

 }

diff --git a/ldap/servers/slapd/back-ldbm/ldbm_delete.c b/ldap/servers/slapd/back-ldbm/ldbm_delete.c

index 528693e..d80c54e 100644

--- a/ldap/servers/slapd/back-ldbm/ldbm_delete.c

+++ b/ldap/servers/slapd/back-ldbm/ldbm_delete.c

@@ -242,14 +242,12 @@ ldbm_back_delete( Slapi_PBlock *pb )

 	 */

 	is_tombstone_entry = slapi_entry_flag_is_set(e->ep_entry, SLAPI_ENTRY_FLAG_TOMBSTONE);

 	if (delete_tombstone_entry) {

-		PR_ASSERT(is_tombstone_entry);

 		if (!is_tombstone_entry) {

 			slapi_log_error(SLAPI_LOG_FATAL, "ldbm_back_delete",

 					"Attempt to delete a non-tombstone entry %s\n", dn);

 			delete_tombstone_entry = 0;

 		}

 	} else {

-		PR_ASSERT(!is_tombstone_entry);

 		if (is_tombstone_entry) { 

 				slapi_log_error(SLAPI_LOG_FATAL, "ldbm_back_delete",

 					"Attempt to Tombstone again a tombstone entry %s\n", dn);

@@ -328,12 +326,31 @@ ldbm_back_delete( Slapi_PBlock *pb )

 	if ( !slapi_sdn_isempty(&parentsdn) )

 	{

 		struct backentry *parent = NULL;

-		entry_address parent_addr;

+		char *pid_str = slapi_entry_attr_get_charptr(e->ep_entry, LDBM_PARENTID_STR);

+		if (pid_str) {

+			/* First, try to get the direct parent. */

+			/* 

+			 * Although a rare case, multiple parents from repl conflict could exist.

+			 * In such case, if a parent entry is found just by parentsdn

+			 * (find_entry2modify_only_ext), a wrong parent could be found,

+			 * and numsubordinate count could get confused.

+			 */

+			ID pid = (ID)strtol(pid_str, (char **)NULL, 10);

+			parent = id2entry(be, pid ,NULL, &retval);

+			if (parent && cache_lock_entry(&inst->inst_cache, parent)) {

+				/* Failed to obtain parent entry's entry lock */

+				CACHE_RETURN(&(inst->inst_cache), &parent);

+				goto error_return;

+			}

+		}

+		if (NULL == parent) {

+			entry_address parent_addr;

-		parent_addr.sdn = &parentsdn;

-		parent_addr.uniqueid = NULL;

-		parent = find_entry2modify_only_ext(pb, be, &parent_addr,

-		                                    TOMBSTONE_INCLUDED, &txn);

+			parent_addr.sdn = &parentsdn;

+			parent_addr.uniqueid = NULL;

+			parent = find_entry2modify_only_ext(pb, be, &parent_addr,

+		                                        TOMBSTONE_INCLUDED, &txn);

+		}

 		if (NULL != parent) {

 			int isglue;

 			size_t haschildren = 0;

@@ -1171,9 +1188,9 @@ common_return:

 	}

 diskfull_return:

-    if(ldap_result_code!=-1)

+	if(ldap_result_code!=-1)

 	{

-    	slapi_send_ldap_result( pb, ldap_result_code, NULL, ldap_result_message, 0, NULL );

+		slapi_send_ldap_result( pb, ldap_result_code, NULL, ldap_result_message, 0, NULL );

 	}

 	modify_term(&parent_modify_c,be);

 	if(dblock_acquired)

diff --git a/ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c b/ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c

index e50b930..4329b16 100644

--- a/ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c

+++ b/ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c

@@ -3164,6 +3164,7 @@ _entryrdn_index_read(backend *be,

             /* Node might be a tombstone. */

             rc = _entryrdn_get_tombstone_elem(cursor, tmpsrdn, 

                                               &key, nrdn, elem);

+            rdnidx--; /* consider nsuniqueid=..,<RDN> one RDN */

         }

         if (rc || NULL == *elem) {

             slapi_log_error(SLAPI_LOG_BACKLDBM, ENTRYRDN_TAG,

@@ -3270,6 +3271,7 @@ _entryrdn_index_read(backend *be,

                     }

                     goto bail;

                 }

+                rdnidx--; /* consider nsuniqueid=..,<RDN> one RDN */

             } else {

                 slapi_ch_free((void **)&tmpelem);

                 if (DB_NOTFOUND != rc) {

diff --git a/ldap/servers/slapd/back-ldbm/ldbm_search.c b/ldap/servers/slapd/back-ldbm/ldbm_search.c

index 5fbea24..f86f27c 100644

--- a/ldap/servers/slapd/back-ldbm/ldbm_search.c

+++ b/ldap/servers/slapd/back-ldbm/ldbm_search.c

@@ -1610,7 +1610,7 @@ ldbm_back_next_search_entry_ext( Slapi_PBlock *pb, int use_extension )

              * just forget about it, since we don't want to return anything at all. */

          {

              if ( slapi_uniqueIDCompareString(target_uniqueid, e->ep_entry->e_uniqueid) ||

-                  slapi_sdn_scope_test( backentry_get_sdn(e), basesdn, scope ))

+                  slapi_sdn_scope_test_ext( backentry_get_sdn(e), basesdn, scope, e->ep_entry->e_flags ))

              {

                  /* check size limit */

                  if ( slimit >= 0 )

diff --git a/ldap/servers/slapd/dn.c b/ldap/servers/slapd/dn.c

index 35c0700..2f50e97 100644

--- a/ldap/servers/slapd/dn.c

+++ b/ldap/servers/slapd/dn.c

@@ -2114,10 +2114,7 @@ slapi_sdn_add_rdn(Slapi_DN *sdn, const Slapi_RDN *rdn)

 	{

 		/* NewDN= NewRDN + DN */

 		const char *dn= slapi_sdn_get_dn(sdn);

-		char *newdn= slapi_ch_malloc(strlen(rawrdn)+1+strlen(dn)+1);

-		strcpy( newdn, rawrdn );

-		strcat( newdn, "," );

-		strcat( newdn, dn );

+		char *newdn = slapi_ch_smprintf("%s,%s", rawrdn, dn);

 		slapi_sdn_set_dn_passin(sdn,newdn);

 	}

 	return sdn;

@@ -2345,7 +2342,7 @@ slapi_sdn_get_backend_parent(const Slapi_DN *sdn,Slapi_DN *sdn_parent,const Slap

 void

 slapi_sdn_get_rdn(const Slapi_DN *sdn,Slapi_RDN *rdn)

 {

-	slapi_rdn_set_dn(rdn,sdn->dn);

+	slapi_rdn_set_dn(rdn, slapi_sdn_get_dn(sdn));

 }

 Slapi_DN *

@@ -2516,6 +2513,47 @@ slapi_sdn_scope_test( const Slapi_DN *dn, const Slapi_DN *base, int scope )

     return rc;

 }

+/* 

+ * Return non-zero if "dn" matches the scoping criteria

+ * given by "base" and "scope".

+ * If SLAPI_ENTRY_FLAG_TOMBSTONE is set to flags,

+ * DN without "nsuniqueid=...," is examined.

+ */

+int

+slapi_sdn_scope_test_ext( const Slapi_DN *dn, const Slapi_DN *base, int scope, int flags )

+{

+    int rc = 0;

+

+    switch ( scope ) {

+    case LDAP_SCOPE_BASE:

+        if (flags & SLAPI_ENTRY_FLAG_TOMBSTONE) {

+            Slapi_DN parent;

+            slapi_sdn_init(&parent);

+            slapi_sdn_get_parent(dn, &parent);

+            rc = ( slapi_sdn_compare( dn, &parent ) == 0 );

+            slapi_sdn_done(&parent);

+        } else {

+            rc = ( slapi_sdn_compare( dn, base ) == 0 );

+        }

+        break;

+    case LDAP_SCOPE_ONELEVEL:

+        if (flags & SLAPI_ENTRY_FLAG_TOMBSTONE) {

+            Slapi_DN parent;

+            slapi_sdn_init(&parent);

+            slapi_sdn_get_parent(dn, &parent);

+            rc = ( slapi_sdn_isparent( base, &parent ) != 0 );

+            slapi_sdn_done(&parent);

+        } else {

+            rc = ( slapi_sdn_isparent( base, dn ) != 0 );

+        }

+        break;

+    case LDAP_SCOPE_SUBTREE:

+        rc = ( slapi_sdn_issuffix( dn, base ) != 0 );

+        break;

+    }

+    return rc;

+}

+

 /*

  * build the new dn of an entry for moddn operations

  */

@@ -2563,7 +2601,16 @@ size_t

 slapi_sdn_get_size(const Slapi_DN *sdn)

 {

     size_t sz = sizeof(Slapi_DN);

+    /* slapi_sdn_get_ndn_len returns the normalized dn length

+     * if dn or ndn exists.  If both does not exist, it

+     * normalizes udn and set it to dn and returns the length.

+     */

     sz += slapi_sdn_get_ndn_len(sdn);

-    sz += strlen(sdn->dn) + 1;

+    if (sdn->dn && sdn->ndn) {

+        sz += slapi_sdn_get_ndn_len(sdn);

+    }

+    if (sdn->udn) {

+        sz += strlen(sdn->udn) + 1;

+    }

     return sz;

 }

diff --git a/ldap/servers/slapd/rdn.c b/ldap/servers/slapd/rdn.c

index d408f0e..fe2fae0 100644

--- a/ldap/servers/slapd/rdn.c

+++ b/ldap/servers/slapd/rdn.c

@@ -479,25 +479,17 @@ slapi_rdn_add(Slapi_RDN *rdn, const char *type, const char *value)

 	PR_ASSERT(NULL != value);

 	if(rdn->rdn==NULL)

 	{

-	    /* type=value '\0' */

-		rdn->rdn= slapi_ch_malloc(strlen(type)+1+strlen(value)+1);

-		strcpy( rdn->rdn, type );

-		strcat( rdn->rdn, "=" );

-		strcat( rdn->rdn, value );

+		/* type=value '\0' */

+		rdn->rdn = slapi_create_dn_string("%s=%s", type, value);

 	}

 	else

 	{

-	    /* type=value+rdn '\0' */

-		char *newrdn= slapi_ch_malloc(strlen(type)+1+strlen(value)+1+strlen(rdn->rdn)+1);

-		strcpy( newrdn, type );

-		strcat( newrdn, "=" );

-		strcat( newrdn, value );

-		strcat( newrdn, "+" );

-		strcat( newrdn, rdn->rdn );

-		slapi_ch_free((void**)&rdn->rdn);

-		rdn->rdn= newrdn;

-	}

-    slapi_unsetbit_uchar(rdn->flag,FLAG_RDNS);

+		/* type=value+rdn '\0' */

+		char *newrdn = slapi_create_dn_string("%s=%s+%s", type, value, rdn->rdn);

+		slapi_ch_free_string(&rdn->rdn);

+		rdn->rdn = newrdn;

+	}

+	slapi_unsetbit_uchar(rdn->flag,FLAG_RDNS);

 	return 1;

 }

diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h

index f78787e..0f69c0a 100644

--- a/ldap/servers/slapd/slapi-plugin.h

+++ b/ldap/servers/slapd/slapi-plugin.h

@@ -2698,6 +2698,24 @@ int slapi_sdn_get_ndn_len(const Slapi_DN *sdn);

 int slapi_sdn_scope_test( const Slapi_DN *dn, const Slapi_DN *base, int scope );

 /**

+ * Checks if a DN is within a specified scope under a specified base DN.

+ * This api adjusts tombstoned DN when comparing with the base dn.

+ *

+ * \param dn A pointer to the \c Slapi_DN structure to test.

+ * \param base The base DN against which \c dn is going to be tested.

+ * \param scope The scope tested.  Valid scopes are:

+ *        \arg \c LDAP_SCOPE_BASE

+ *        \arg \c LDAP_SCOPE_ONELEVEL

+ *        \arg \c LDAP_SCOPE_SUBTREE

+ * \param flags 0 or SLAPI_ENTRY_FLAG_TOMBSTONE

+ * \return non-zero if \c dn matches the scoping criteria given by \c base and \c scope.

+ * \see slapi_sdn_compare()

+ * \see slapi_sdn_isparent()

+ * \see slapi_sdn_issuffix()

+ */

+int slapi_sdn_scope_test_ext( const Slapi_DN *dn, const Slapi_DN *base, int scope, int flags );

+

+/**

  * Retreives the RDN from a given DN.

  *

  * This function takes the DN stored in the \c Slapi_DN structure pointed to

-- 

1.8.1.4