Bug 631862 - crash - delete entries not in cache + referint

https://bugzilla.redhat.com/show_bug.cgi?id=631862
Resolves: bug 631862
Bug Description: crash - delete entries not in cache + referint
Reviewed by: rmeggins and nhosoi
Branch: 389-ds-base-1.2.6
Fix Description: When deleting an entry, the referential integrity (referint)
plugin does an internal search to find references to this entry (e.g. in
group entries) and removes them.  The search code wants to ensure that the
entrydn attribute is present in the entry when using entryrdn (subtree
rename).  The search code sets a flag to tell the id2entry code to add the
entrydn attribute if it is not present.  However, it was doing this to an
entry in the cache, which may be in use by another thread.  The solution is
to add the entrydn attribute before adding the entry to the cache.  In the
id2entry code, this is after the entry has been read from the id2entry db
successfully, but before the entry is added to the cache.  In the LDAP ADD
code, this is done when the other computed operational attributes are added
to the new entry.
In addition to the above fix by rmeggins@redhat.com, following changes are
made:
1) entrydn attribute is always added to the entry in memory before putting
   it in the entry cache, and the attribute is removed before writing the
   entry to the database.
2) eliminating id2entry_ext, which was introduced to pass flags, but it is
   no longer needed since only a flag ID2ENTRY_ADD_ENTRYDN was removed.
   Platforms tested: RHEL5 x86_64
   Flag Day: no
   Doc impact: no

5 files changed, 96 insertions, 54 deletions

diff --git a/ldap/servers/slapd/back-ldbm/back-ldbm.h b/ldap/servers/slapd/back-ldbm/back-ldbm.h
index f0e290eb..200e950d 100644
--- a/ldap/servers/slapd/back-ldbm/back-ldbm.h
+++ b/ldap/servers/slapd/back-ldbm/back-ldbm.h
@@ -828,9 +828,6 @@ typedef struct _back_search_result_set
 /* whether we call fat lock or not [608146] */
 #define SERIALLOCK(li)	(li->li_fat_lock)
 
-/* id2entry_ext flags */
-#define ID2ENTRY_ADD_ENTRYDN 0x1
-
 /* 
  * 0: SUCCESS
  * libdb returns negative error codes
diff --git a/ldap/servers/slapd/back-ldbm/id2entry.c b/ldap/servers/slapd/back-ldbm/id2entry.c
index 31d3b724..93068132 100644
--- a/ldap/servers/slapd/back-ldbm/id2entry.c
+++ b/ldap/servers/slapd/back-ldbm/id2entry.c
@@ -60,6 +60,7 @@ id2entry_add_ext( backend *be, struct backentry *e, back_txn *txn, int encrypt
     int    len, rc;
     char   temp_id[sizeof(ID)];
     struct backentry *encrypted_entry = NULL;
+    char *entrydn = NULL;
 
     LDAPDebug( LDAP_DEBUG_TRACE, "=> id2entry_add( %lu, \"%s\" )\n",
                                  (u_long)e->ep_id, backentry_get_ndn(e), 0 );
@@ -92,6 +93,7 @@ id2entry_add_ext( backend *be, struct backentry *e, back_txn *txn, int encrypt
         memset(&data, 0, sizeof(data));
         if (entryrdn_get_switch())
         {
+            Slapi_Attr *eattr = NULL;
             struct backdn *oldbdn = NULL;
             Slapi_DN *sdn =
                           slapi_sdn_dup(slapi_entry_get_sdn_const(e->ep_entry));
@@ -100,7 +102,14 @@ id2entry_add_ext( backend *be, struct backentry *e, back_txn *txn, int encrypt
 
             /* If the ID already exists in the DN cache, replace it. */
             if (CACHE_ADD( &inst->inst_dncache, bdn, &oldbdn ) == 1) {
-                cache_replace( &inst->inst_dncache, oldbdn, bdn );
+                if (slapi_sdn_compare(sdn, oldbdn->dn_sdn)) {
+                    if (cache_replace( &inst->inst_dncache, oldbdn, bdn ) != 0) {
+                        /* The entry was not in the cache for some reason (this
+                         * should not happen since CACHE_ADD said it existed above). */
+                        LDAPDebug( LDAP_DEBUG_ANY, "id2entry_add_ext(): Entry disappeared "
+                                   "from cache (%s)\n", oldbdn->dn_sdn, 0, 0 );
+                    }
+                }
                 CACHE_RETURN(&inst->inst_dncache, &oldbdn); /* to free oldbdn */
             }
 
@@ -108,6 +117,13 @@ id2entry_add_ext( backend *be, struct backentry *e, back_txn *txn, int encrypt
             LDAPDebug( LDAP_DEBUG_TRACE,
                    "=> id2entry_add (dncache) ( %lu, \"%s\" )\n",
                    (u_long)e->ep_id, slapi_entry_get_dn_const(e->ep_entry), 0 );
+            /* If entrydn exists in the entry, we have to remove it before
+             * writing the entry to the database. */
+            if (0 == slapi_entry_attr_find(e->ep_entry,
+                                           LDBM_ENTRYDN_STR, &eattr)) {
+                /* entrydn exists in the entry.  let's removed it. */
+                slapi_entry_delete_values(e->ep_entry, LDBM_ENTRYDN_STR, NULL);
+            }
         }
         data.dptr = slapi_entry2str_with_options(entry_to_use, &len, options);
         data.dsize = len + 1;
@@ -133,10 +149,54 @@ id2entry_add_ext( backend *be, struct backentry *e, back_txn *txn, int encrypt
 
     if (0 == rc)
     {
-        /* DBDB the fact that we don't check the return code here is
-         * indicitive that there may be a latent race condition lurking
-         * ---what happens if the entry is already in the cache by this point?
-         */
+        if (entryrdn_get_switch()) {
+            struct backentry *parententry = NULL;
+            ID parentid = slapi_entry_attr_get_ulong(e->ep_entry, "parentid");
+            const char *myrdn = slapi_entry_get_rdn_const(e->ep_entry);
+            const char *parentdn = NULL;
+            char *myparentdn = NULL;
+            Slapi_Attr *eattr = NULL;
+            /* If the parent is in the cache, check the parent's DN and 
+             * adjust to it if they don't match. (bz628300) */
+            if (parentid && myrdn) {
+                parententry = cache_find_id(&inst->inst_cache, parentid);
+                if (parententry) {
+                    parentdn = slapi_entry_get_dn_const(parententry->ep_entry);
+                    if (parentdn) {
+                        myparentdn = 
+                         slapi_dn_parent(slapi_entry_get_dn_const(e->ep_entry));
+                        if (myparentdn && PL_strcmp(parentdn, myparentdn)) {
+                            Slapi_DN *sdn = slapi_entry_get_sdn(e->ep_entry);
+                            char *newdn = NULL;
+                            slapi_sdn_done(sdn);
+                            newdn = slapi_ch_smprintf("%s,%s", myrdn, parentdn);
+                            slapi_sdn_init_dn_passin(sdn, newdn);
+                            slapi_sdn_get_ndn(sdn); /* to set ndn */
+                        }
+                        slapi_ch_free_string(&myparentdn);
+                    }
+                    CACHE_RETURN(&inst->inst_cache, &parententry);
+                }
+            }
+            /* 
+             * Adding entrydn attribute value to the entry,
+             * which should be done before adding the entry to the entry cache.
+             * Note: since we removed entrydn from the entry before writing
+             * it to the database, it is guaranteed not in the entry.
+             */
+            /* slapi_ch_strdup and slapi_dn_ignore_case never returns NULL */
+            entrydn = slapi_ch_strdup(slapi_entry_get_dn_const(e->ep_entry));
+            entrydn = slapi_dn_ignore_case(entrydn);
+            slapi_entry_attr_set_charptr (e->ep_entry,
+                                          LDBM_ENTRYDN_STR, entrydn);
+            if (0 == slapi_entry_attr_find(e->ep_entry,
+                                           LDBM_ENTRYDN_STR, &eattr)) {
+                /* now entrydn should exist in the entry */
+                /* Set it to operational attribute */
+                eattr->a_flags = SLAPI_ATTR_FLAG_OPATTR;
+            }
+            slapi_ch_free_string(&entrydn);
+        }
         /* 
          * For ldbm_back_add and ldbm_back_modify, this entry had been already
          * reserved as a tentative entry.  So, it should be safe.
@@ -208,7 +268,7 @@ id2entry_delete( backend *be, struct backentry *e, back_txn *txn )
 }
 
 struct backentry *
-id2entry_ext( backend *be, ID id, back_txn *txn, int *err, int flags  )
+id2entry( backend *be, ID id, back_txn *txn, int *err  )
 {
     ldbm_instance    *inst = (ldbm_instance *) be->be_instance_info;
     DB               *db = NULL;
@@ -350,6 +410,30 @@ id2entry_ext( backend *be, ID id, back_txn *txn, int *err, int flags  )
                             "attrcrypt_decrypt_entry failed in id2entry\n");
         }
         
+        /* 
+         * If return entry exists AND entryrdn switch is on,
+         * add the entrydn value.
+         */
+        if (entryrdn_get_switch()) {
+            Slapi_Attr *eattr = NULL;
+            /* Check if entrydn is in the entry or not */
+            if (slapi_entry_attr_find(e->ep_entry, LDBM_ENTRYDN_STR, &eattr)) {
+                /* entrydn does not exist in the entry */
+                char *entrydn = NULL;
+                /* slapi_ch_strdup and slapi_dn_ignore_case never returns NULL */
+                entrydn = slapi_ch_strdup(slapi_entry_get_dn_const(e->ep_entry));
+                entrydn = slapi_dn_ignore_case(entrydn);
+                slapi_entry_attr_set_charptr (e->ep_entry,
+                                              LDBM_ENTRYDN_STR, entrydn);
+                if (0 == slapi_entry_attr_find(e->ep_entry,
+                                               LDBM_ENTRYDN_STR, &eattr)) {
+                    /* now entrydn should exist in the entry */
+                    /* Set it to operational attribute */
+                    eattr->a_flags = SLAPI_ATTR_FLAG_OPATTR;
+                }
+                slapi_ch_free_string(&entrydn);
+            }
+        }
         retval = CACHE_ADD( &inst->inst_cache, e, &imposter );
         if (1 == retval) {
             /* This means that someone else put the entry in the cache
@@ -376,29 +460,6 @@ id2entry_ext( backend *be, ID id, back_txn *txn, int *err, int flags  )
     }
 
 bail:
-    /* 
-     * If return entry exists AND adding entrydn is requested AND
-     * entryrdn switch is on, add the entrydn value.
-     */
-    if (e && e->ep_entry && (flags & ID2ENTRY_ADD_ENTRYDN) &&
-        entryrdn_get_switch()) {
-        Slapi_Attr *eattr = NULL;
-        /* Check if entrydn is in the entry or not */
-        if (slapi_entry_attr_find(e->ep_entry, "entrydn", &eattr)) {
-            /* entrydn does not exist in the entry */
-            char *entrydn = NULL;
-            /* slapi_ch_strdup and slapi_dn_ignore_case never returns NULL */
-            entrydn = slapi_ch_strdup(slapi_entry_get_dn_const(e->ep_entry));
-            entrydn = slapi_dn_ignore_case(entrydn);
-            slapi_entry_attr_set_charptr (e->ep_entry, "entrydn", entrydn);
-            if (0 == slapi_entry_attr_find(e->ep_entry, "entrydn", &eattr)) {
-                /* now entrydn should exist in the entry */
-                /* Set it to operational attribute */
-                eattr->a_flags = SLAPI_ATTR_FLAG_OPATTR;
-            }
-            slapi_ch_free_string(&entrydn);
-        }
-    }
     slapi_ch_free( &(data.data) );
 
     dblayer_release_id2entry( be, db );
@@ -407,10 +468,3 @@ bail:
                     "<= id2entry( %lu ) %p (disk)\n", (u_long)id, e);
     return( e );
 }
-
-struct backentry *
-id2entry( backend *be, ID id, back_txn *txn, int *err  )
-{
-    return id2entry_ext(be, id, txn, err, 0);
-}
-
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_add.c b/ldap/servers/slapd/back-ldbm/ldbm_add.c
index d2d6197e..c2e86f28 100644
--- a/ldap/servers/slapd/back-ldbm/ldbm_add.c
+++ b/ldap/servers/slapd/back-ldbm/ldbm_add.c
@@ -575,17 +575,12 @@ ldbm_back_add( Slapi_PBlock *pb )
 	
 	if(is_resurect_operation)
 	{
-		if (!entryrdn_get_switch()) { /* subtree-rename: off */
-			/* add the entrydn operational attributes to the addingentry */
-			add_update_entrydn_operational_attributes(addingentry);
-		}
+		add_update_entrydn_operational_attributes(addingentry);
 	}
 	else if (is_tombstone_operation)
 	{
-		if (!entryrdn_get_switch()) { /* subtree-rename: off */
-			/* Remove the entrydn operational attributes from the addingentry */
-			delete_update_entrydn_operational_attributes(addingentry);
-		}
+		/* Remove the entrydn operational attributes from the addingentry */
+		delete_update_entrydn_operational_attributes(addingentry);
 	}
 	else
 	{
@@ -978,10 +973,8 @@ add_update_entry_operational_attributes(struct backentry *ep, ID pid)
 	bv.bv_len = strlen( buf );
 	entry_replace_values( ep->ep_entry, "entryid", bvp );
 
-    if (!entryrdn_get_switch()) { /* subtree-rename: off */
-		/* add the entrydn operational attribute to the entry. */
-		add_update_entrydn_operational_attributes(ep);
-	}
+	/* add the entrydn operational attribute to the entry. */
+	add_update_entrydn_operational_attributes(ep);
 }
 
 /*
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_search.c b/ldap/servers/slapd/back-ldbm/ldbm_search.c
index 9a4925f6..0010a58a 100644
--- a/ldap/servers/slapd/back-ldbm/ldbm_search.c
+++ b/ldap/servers/slapd/back-ldbm/ldbm_search.c
@@ -1241,7 +1241,7 @@ ldbm_back_next_search_entry_ext( Slapi_PBlock *pb, int use_extension )
         ++sr->sr_lookthroughcount;    /* checked above */
 
         /* get the entry */
-        e = id2entry_ext( be, id, NULL, &err, ID2ENTRY_ADD_ENTRYDN );
+        e = id2entry( be, id, NULL, &err );
         if ( e == NULL )
         {
             if ( err != 0 && err != DB_NOTFOUND )
diff --git a/ldap/servers/slapd/back-ldbm/proto-back-ldbm.h b/ldap/servers/slapd/back-ldbm/proto-back-ldbm.h
index 90ac5700..c7b81545 100644
--- a/ldap/servers/slapd/back-ldbm/proto-back-ldbm.h
+++ b/ldap/servers/slapd/back-ldbm/proto-back-ldbm.h
@@ -214,8 +214,6 @@ int id2entry_add( backend *be, struct backentry *e, back_txn *txn );
 int id2entry_add_ext( backend *be, struct backentry *e, back_txn *txn, int encrypt );
 int id2entry_delete( backend *be, struct backentry *e, back_txn *txn );
 struct backentry * id2entry( backend *be, ID id, back_txn *txn, int *err );
-struct backentry * id2entry_ext( backend *be, ID id, back_txn *txn, int *err, int flags  );
-
 
 /*
  * idl.c