#47367 ldapdelete returns non-leaf entry error while trying to remove a leaf entry
Closed: Fixed None Opened 6 years ago by nhosoi.

Ticket was cloned from Red Hat Bugzilla (product Red Hat Enterprise Linux 6): Bug 947583

{{
Description of problem:

Unable to remove an ou from the directory server, Looking at the ldapsearch
outputs and the db2ldif dump I can confirm there are no child entries, but the
server does not allow me to remove this particular entry.. it returns err 66
(non-leaf entry).

Steps:
Set up MMR (preferably, more than 2 way)
Create a script which includes add/modify/delete.
The delete should include parent level:
uid=test,ou=people,<suffix>
changetype: delete

ou=people,<suffix>
changetype: delete
Run the same script against the all masters at the same time.
Repeat it until naming conflicts occur.

Some parent entry could have mismatched numsubordinates.
Example:
dn: nsuniqueid=1965a88d-c3f611e2-935ac747-051f73f9,<suffix>
numSubordinates: 3
tombstoneNumSubordinates: 9
hassubordinates: TRUE

This entry happened to have 12 tombstoned child entries.
If you delete all of them, numsubordinates of the above entry turned to be:
numsubordinates: 3
hassubordinates: TRUE
tombstonenumsubordinates: 0

And deleting attempt fails by "Operation not allowed on nonleaf"
nsuniqueid=1965a88d-c3f611e2-935ac747-051f73f9,dc=gsslab,dc=pnq,dc=redhat,dc=com
ldap_delete: Operation not allowed on nonleaf
}}


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] This patch removes 2 PR_ASSERT calls for
is_tombstone_entry, which allows us to test deleting an
tombstone entry without aborting the server built with debug
flag.
[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_ch_smprintf to improve the readability of the code.
}}}

Note: this patch is for 389-ds-base-1.2.11. To apply this patch to master, it requires a few conflict fixes.

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

why not use slapi_create_dn_string instead? or are you guaranteed here that the components are already properly normalized?

in slapi_sdn_scope_test_ext() - you need to do slapi_sdn_init(&parent); before calling slapi_sdn_get_parent(dn, &parent);

in slapi_sdn_get_size() - is it possible for both sdn->dn and sdn->udn to be set? If so, then sz will be both lengths.

Does slapi_rdn_add need to use slapi_create_dn_string, or otherwise ensure that the DNs are normalized?

Replying to [comment:5 rmeggins]:

{{{
newdn = slapi_ch_smprintf("%s,%s", slapi_rdn_get_rdn(rdn), parentdn);
}}}
why not use slapi_create_dn_string instead? or are you guaranteed here that the components are already properly normalized?
I thought it is. But your comment made me rethink... "parentdn" is guaranteed. And the individual value of the leaf, multi-valued rdn is normalized, as well. But the order of "nsuniqueid=<id>+<rdm>" could be changed by the dn normalization. I'm going to fix the leaf part.

in slapi_sdn_scope_test_ext() - you need to do slapi_sdn_init(&parent); before calling slapi_sdn_get_parent(dn, &parent);
Done.

in slapi_sdn_get_size() - is it possible for both sdn->dn and sdn->udn to be set? If so, then sz will be both lengths.
{{{
sz += slapi_sdn_get_ndn_len(sdn);
if (sdn->dn) {
sz += strlen(sdn->dn) + 1;
}
if (sdn->udn) {
sz += strlen(sdn->udn) + 1;
}
}}}
You are right. There are some cases this logic returns a wrong result (e.g., only udn is set, slapi_sdn_get_ndn_len generates dn and returns the size of dn. Then, sdn->dn is counted again.) Let me fix it.

Does slapi_rdn_add need to use slapi_create_dn_string, or otherwise ensure that the DNs are normalized?
Yep, a very nice idea! It'll solve the first issue" the order of nsuniqueid=<id>+<rdn>.

Thanks a lot, Rich!

Thanks to Rich for his comments. I updated the code following his suggestions.

In addition, the heavier test revealed more issues in the deletion. Revised patch contains this fix:
{{{
[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.
}}}

Reviewed by Rich (Thank you!!)

Pushed to 389-ds-base-1.2.11: commit c2c6401

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

should this use slapi_create_dn_string()?
and here
{{{
char *newdn = slapi_ch_smprintf("%s,%s", rawrdn, dn);
}}}

otherwise, ack

Thanks, Rich!
Replying to [comment:10 rmeggins]:

{{{
newdn = slapi_ch_smprintf("%s,%s", slapi_rdn_get_rdn(rdn), parentdn);
}}}
[urp.c]
This rdn is normalized here:
1260 slapi_rdn_set_dn(rdn, slapi_sdn_get_dn(oldsdn));
and parentdn is at
1275 char *parentdn = slapi_dn_parent(slapi_sdn_get_dn(oldsdn));
So, we can save one dn_normalization for this. (I'm putting the comment...)

should this use slapi_create_dn_string()?
and here
{{{
char *newdn = slapi_ch_smprintf("%s,%s", rawrdn, dn);
}}}
[dn.c]
Regarding this one, rawrdn thus newdn is not normalized. But it's passed to slapi_sdn_set_dn_passin(sdn,newdn) as a pre-normalized dn. So, it should be okay.

otherwise, ack

Reviewed by Rich (Thank you!!)

Pushed to master:
commit 2f4f74b
commit 7330597

Pushed to 389-ds-base-1.3.1:
commit 690d58b
commit ad0a2ca

Pushed to 389-ds-base-1.3.0:
commit 28b313c
commit 6b87414

There is a memory leak:
==24307== 2,264 bytes in 1,132 blocks are definitely lost in loss record 1,363 of 1,456
==24307== at 0x4A069EE: malloc (vg_replace_malloc.c:270)
==24307== by 0x4C5A9DD: slapi_ch_malloc (ch_malloc.c:155)
==24307== by 0x4C710D5: slapi_entry_attr_get_charptr (entry.c:2730)
==24307== by 0xA42FD59: ldbm_back_delete (ldbm_delete.c:329)
==24307== by 0x4C604A7: op_shared_delete (delete.c:364)
==24307== by 0x4C5FC90: do_delete (delete.c:128)
==24307== by 0x414A58: connection_dispatch_operation (connection.c:584)
==24307== by 0x416469: connection_threadmain (connection.c:2339)
==24307== by 0x3A33429995: _pt_root (ptthread.c:204)
==24307== by 0x3A244079D0: start_thread (pthread_create.c:301)
==24307== by 0x3A23CE8B6C: clone (clone.S:115)

  • char *pid_str = slapi_entry_attr_get_charptr(e->ep_entry, LDBM_P

The pid_str is not freed.

Metadata Update from @rmeggins:
- Issue assigned to nhosoi
- Issue set to the milestone: 1.2.11.22

2 years ago

Login to comment on this ticket.

Metadata