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!

revised git patch file (389-ds-base-1.2.11 branch)
0001-Ticket-47367-phase-1-ldapdelete-returns-non-leaf-ent.patch

Diffs from the previous patch
0002-snapshot.patch

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

git patch file (master) - phase1: ported from 1.2.11
0001-Ticket-47367-phase-1-ldapdelete-returns-non-leaf-ent.2.patch

git patch file (master) - phase2
0002-Ticket-47367-phase-2-ldapdelete-returns-non-leaf-ent.patch

{{{
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.

memleak fixed in ticket #47517

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/704

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.