From c2c64017e0e25a4376139debe29b4f587964710f Mon Sep 17 00:00:00 2001 From: Noriko Hosoi Date: Jun 10 2013 17:47:10 +0000 Subject: 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=...+ ,". [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=..., ) 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 --- 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=+" */ 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 f3823cb..156461b 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c @@ -3152,6 +3152,7 @@ _entryrdn_index_read(backend *be, /* Node might be a tombstone. */ rc = _entryrdn_get_tombstone_elem(cursor, tmpsrdn, &key, nrdn, elem); + rdnidx--; /* consider nsuniqueid=.., one RDN */ } if (rc || NULL == *elem) { slapi_log_error(SLAPI_LOG_BACKLDBM, ENTRYRDN_TAG, @@ -3258,6 +3259,7 @@ _entryrdn_index_read(backend *be, } goto bail; } + rdnidx--; /* consider nsuniqueid=.., 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 c61dcce..e68b897 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_search.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_search.c @@ -1602,7 +1602,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 b40b6a0..dda439b 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,8 +2601,17 @@ 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 4e7aae5..4a2544e 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