The problem is that for the empty value

objectClass;vdcsn-5283b8e0000000c80000;deleted

it is compared to "ldapsubentry" and "nstombstone"

if (PL_strncasecmp(type.bv_val,"objectclass",type.bv_len) == 0) {
454 if (PL_strncasecmp(value.bv_val,"ldapsubentry",value.bv_len) == 0)
455 e->e_flags |= SLAPI_ENTRY_LDAPSUBENTRY;
456 if (PL_strncasecmp(value.bv_val, SLAPI_ATTR_VALUE_TOMBSTONE,value.bv_len) == 0)

There are two bugs here:
- the value is deleted, so it should not be considered as tombstone or subentry, even if it would match
- the strings are only compared up to the value length (0 in this case).

And the problem is not only for this specific value comparison.
Eg if the schema is extended to define an attribute: "nsu"

Then the test:
if ( PL_strncasecmp (type.bv_val, SLAPI_ATTR_UNIQUEID, type.bv_len) == 0 )

is true and takes the duplicate nsuniqueid branch.

In general all the calls
PL_strncasecmp (bv_val, <string>, bv_len) == 0

need to be checked and replaced.
The reason for using strNcasecmp is probably that bv_vals are not guaranteed to be NULL terminated, although it seems to be the case inside str2entry_fast()

Alternative 1: PL_strcasecmp (bv_val, <string>, bv_len) == 0

Alternative 2: (bv_len >= strlen(<string>) && PL_strncasecmp (bv_val, <string>, bv_len) == 0 )
It has to be ">=" and not "==" because we have types like:

(gdb) p type
$11 = {bv_len = 38, bv_val = 0x7f07f800155a "objectClass"}
(gdb) x/s 0x7f07f800155a
0x7f07f800155a: "objectClass"
(gdb) x/s 0x7f07f800155a+12
0x7f07f8001566: "vucsn-5284a364000000c80000"

Alternative 1:

attachment
0001-Ticket-47591-v1-entries-with-empty-objectclass-attri.patch

attachment
0001-Ticket-47591-v2-entries-with-empty-objectclass-attri.patch

Replying to [comment:2 lkrispen]:

And the problem is not only for this specific value comparison.
Eg if the schema is extended to define an attribute: "nsu"

Then the test:
if ( PL_strncasecmp (type.bv_val, SLAPI_ATTR_UNIQUEID, type.bv_len) == 0 )

is true and takes the duplicate nsuniqueid branch.

In general all the calls
PL_strncasecmp (bv_val, <string>, bv_len) == 0

need to be checked and replaced.
The reason for using strNcasecmp is probably that bv_vals are not guaranteed to be NULL terminated, although it seems to be the case inside str2entry_fast()

Alternative 1: PL_strcasecmp (bv_val, <string>, bv_len) == 0

Alternative 2: (bv_len >= strlen(<string>) && PL_strncasecmp (bv_val, <string>, bv_len) == 0 )
It has to be ">=" and not "==" because we have types like:

(gdb) p type
$11 = {bv_len = 38, bv_val = 0x7f07f800155a "objectClass"}
(gdb) x/s 0x7f07f800155a
0x7f07f800155a: "objectClass"
(gdb) x/s 0x7f07f800155a+12
0x7f07f8001566: "vucsn-5284a364000000c80000"

What if the user has an attribute type like
(gdb) p type
$11 = {bv_len = 14, bv_val = 0x7f07f800155a "objectClassExt"}
?

Alternative 1:

It should be handled correctly by both versions.
V2: strcasecmp returns != 0
V1: strncasecmp says it compares up to len or a string termonator, whatever comes first, so
strncasecmp ("objectclass", objectclassext",14) should also return != 0

I was wrong.

strncasecmp stops at len or the first null terminator and compares only the strings before,

so
strncasecmp ("objectclass", objectclassext",4)
strncasecmp ("objectclass", objectclassext",11)
strncasecmp ("objectclass", objectclassext",14)
strncasecmp ("objectclass", objectclassext",17)
all return equal

I'm sorry, I had written a small testprogramm and made a typo to always compare "objectclass","objectclass".

After fixing, the result is:
strncasecmp ("objectclass", objectclassext",4) EQUAL
strncasecmp ("objectclass", objectclassext",11) EQUAL
strncasecmp ("objectclass", objectclassext",14) NOT EQUAL
strncasecmp ("objectclass", objectclassext",17) NOT EQUAL

In general, I prefer the explicit length checking (v1). In these cases:
{{{

• if ( PL_strncasecmp( type.bv_val, "dn", type.bv_len ) == 0 ) {
• if ( type.bv_len >= 2 && PL_strncasecmp( type.bv_val, "dn", type.bv_len ) == 0 ) {
  }}}
  If type > 2, then PL_strncasecmp( "dna", "dn", 3 ) != 0
  I don't think you need the >= for this case, or rdn, entrydn, or uniqueid, since none of these will ever be of the form entrydn;vucsn-xxxxxx.

Do not call strlen on the constant strings "objectclass" or "ldapsubentry" or SLAPI_ATTR_VALUE_TOMBSTONE. I don't know if the compiler will optimize this into a sizeof or other constant, but best to define this outside of str2entry and use a real constant value for the length.

I think I'm missing something
{{{
(gdb) p type
$11 = {bv_len = 38, bv_val = 0x7f07f800155a "objectClass"}
(gdb) x/s 0x7f07f800155a
0x7f07f800155a: "objectClass"
(gdb) x/s 0x7f07f800155a+12
0x7f07f8001566: "vucsn-5284a364000000c80000"
}}}
Does the code find the first ';' in the string and write a '\0' there? And that's how we are able to do PL_strncasecmp(type.bv_val,"objectclass",type.bv_len) even though type.bv_len is > strlen("objectclass")?

I think I'm missing something
{{{
(gdb) p type
$11 = {bv_len = 38, bv_val = 0x7f07f800155a "objectClass"}
(gdb) x/s 0x7f07f800155a
0x7f07f800155a: "objectClass"
(gdb) x/s 0x7f07f800155a+12
0x7f07f8001566: "vucsn-5284a364000000c80000"
}}}
Does the code find the first ';' in the string and write a '\0' there? And that's how we are able to do PL_strncasecmp(type.bv_val,"objectclass",type.bv_len) even though type.bv_len is > strlen("objectclass")?

yes. In
str2entry_state_information_from_type(type.bv_val,&valuecsnset,&attributedeletioncsn,&maxcsn,&value_state,&attr_state);

it replaces the first ";" by "\0" and parses the rest into csnset,......

Replying to [comment:7 rmeggins]:

If type > 2, then PL_strncasecmp( "dna", "dn", 3 ) != 0
I don't think you need the >= for this case, or rdn, entrydn, or uniqueid, since none of these will ever be of the form entrydn;vucsn-xxxxxx.

for these types it is teh other way round, if you define a custom schema attr "nsu", then you can have:
$11 = {bv_len = 3, bv_val = 0x7f07f800155a "nsu"} and get
PL_strncasecmp (type.bv_val, SLAPI_ATTR_UNIQUEID, type.bv_len) == 0

Replying to [comment:9 lkrispen]:

Replying to [comment:7 rmeggins]:

If type > 2, then PL_strncasecmp( "dna", "dn", 3 ) != 0
I don't think you need the >= for this case, or rdn, entrydn, or uniqueid, since none of these will ever be of the form entrydn;vucsn-xxxxxx.

for these types it is teh other way round, if you define a custom schema attr "nsu", then you can have:
$11 = {bv_len = 3, bv_val = 0x7f07f800155a "nsu"} and get
PL_strncasecmp (type.bv_val, SLAPI_ATTR_UNIQUEID, type.bv_len) == 0

Right. Sorry, I wasn't clear. You don't need "type.bv_len >= N" you just need "type.bv_len == N".

attachment
0001-Ticket-47591-v1.1-entries-with-empty-objectclass-att.patch

This version is still using ">=" instead of "==" for things like dn, rdn, etc. Unless they really need to be >=, I would prefer ==.

attachment
0001-Ticket-47591-v1.2-entries-with-empty-objectclass-att.patch

sorry, new patch attached

Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1032317

$git merge ticket47591-1
Updating 6b83caf..3a6ce92
Fast-forward
ldap/servers/slapd/entry.c | 15 ++++++++-------
ldap/servers/slapd/slapi-plugin.h | 15 ++++++++++++++-
2 files changed, 22 insertions(+), 8 deletions(-)
$ git push origin master
Counting objects: 13, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (7/7), done.
Writing objects: 100% (7/7), 1.28 KiB, done.
Total 7 (delta 5), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
6b83caf..3a6ce92 master -> master

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/928

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.