ldb 1.3.2 fails some tests

[Timur I. Bakeyev]

On 2 March 2018 at 19:49, Andrew Bartlett <abartlet at samba.org> wrote:

> On Fri, 2018-03-02 at 19:44 +0100, Timur I. Bakeyev via samba-technical
> wrote:
> > For the new LDB 1.3.2 been released recently at least two tests fail on
> > FreeBSD:
> > [  FAILED  ] 2 test(s), listed below:
> > [  FAILED  ] test_ldb_unique_index_duplicate_logging
> > [  FAILED  ] test_ldb_unique_index_duplicate_with_guid
> >
> >  2 FAILED TEST(S)
>
> Very strange,
>
> I can only suggest getting into a debugger and working out why it
> fails.
>
>
Well, in the end the problem is exposed in that note in the `man qsort` on
FreeBSD:

The algorithms implemented by qsort(), qsort_r(), and heapsort() are not
stable, that is, if two members compare as equal, their order in the
sorted array is undefined.

Corresponding manpage on Linux is very terse, so I can only assume that
`qsort` on Linux
is either stable or it's just lucky coincidence that code works as expected.

In more details the problem, at least in those failing the tests, lays in
defining two different syntaxes
for the "cn" attribute:

(gdb) p ldb->schema
$13 = {attribute_handler_override_private = 0x0, attribute_handler_override
= 0, num_attributes = 7, attributes = 0x802036560, num_dn_extended_syntax =
0,
  dn_extended_syntax = 0x0, index_handler_override = false,
one_level_indexes = false, GUID_index_attribute = 0x0,
GUID_index_dn_component = 0x0}
(gdb) p *ldb->schema.attributes
$15 = {name = 0x80202a090 "cn", flags = 74, syntax = 0x800c8eec0}
(gdb) p ldb->schema.attributes[0]->name
$17 = 0x80202a090 "cn"
(gdb) p *ldb->schema.attributes[0]->syntax
$19 = {name = 0x800a865ad "1.3.6.1.4.1.1466.115.121.1.40", ldif_read_fn =
0x800a6d5d0 <ldb_handler_copy>, ldif_write_fn = 0x800a6d5d0
<ldb_handler_copy>,
  canonicalise_fn = 0x800a6d5d0 <ldb_handler_copy>, comparison_fn =
0x800a6d8b0 <ldb_comparison_binary>, operator_fn = 0}
(gdb) p ldb->schema.attributes[1]->name
$20 = 0x800a86554 "cn"
(gdb) p *ldb->schema.attribute[1]->syntax
$21 = {name = 0x800a86557 "1.3.6.1.4.1.1466.115.121.1.15", ldif_read_fn =
0x800a6d5d0 <ldb_handler_copy>, ldif_write_fn = 0x800a6d5d0
<ldb_handler_copy>,
  canonicalise_fn = 0x800a6d680 <ldb_handler_fold>, comparison_fn =
0x800a6d920 <ldb_comparison_fold>, operator_fn = 0}

So, array of attributes schemas gets two definitions for "cn" attribute
with different handlers. I'm not sure, if this is a correct behavior at all,
but current code allows such an addition through the
ltdb_attributes_load()->ldb_schema_attribute_fill_with_syntax() sequence,
while
doing proper definition replacement in the
ldb_schema_attribute_add_with_syntax().

So, in ltdb_attributes_load() we perform qsort() of the attribute schemas
and it happens so that order of two "cn" attributes differs on
FreeBSD from Linux.

Later on when we look for the attribute schema in
ldb_schema_attribute_by_name_internal() bisect brings us to the different
"cn" definitions
on two platforms. Again, I'm not sure that having double schema definition
for one attribute is a valid case.

One more thing, besides different canonicalize functions, that differs for
those definitions is the `flags` value. So, quick and dirty fix I used
utilizes that fact:

--- ldb_tdb/ldb_cache.c.orig    2018-03-04 05:41:25.313506000 +0100
+++ ldb_tdb/ldb_cache.c 2018-03-04 05:46:09.353115000 +0100
@@ -91,7 +91,9 @@ static int ldb_schema_attribute_compare(
 {
        const struct ldb_schema_attribute *sa1 = (const struct
ldb_schema_attribute *)p1;
        const struct ldb_schema_attribute *sa2 = (const struct
ldb_schema_attribute *)p2;
-       return ldb_attr_cmp(sa1->name, sa2->name);
+       int res = ldb_attr_cmp(sa1->name, sa2->name);
+
+       return (res) ? res : (sa1->flags > sa2->flags) ? 1 : (sa1->flags <
sa2->flags) ? -1 : 0;
 }

 /*
--- tests/ldb_mod_op_test.c.orig        2018-03-02 23:35:09.639709000 +0100
+++ tests/ldb_mod_op_test.c     2018-03-02 23:44:00.194683000 +0100
@@ -3529,7 +3529,7 @@ static void test_ldb_unique_index_duplic
        assert_int_equal(ret, LDB_SUCCESS);

        msg02 = ldb_msg_new(tmp_ctx);
-       assert_non_null(msg01);
+       assert_non_null(msg02);

        msg02->dn = ldb_dn_new_fmt(msg02, test_ctx->ldb, "dc=test02");
        assert_non_null(msg02->dn);
-- 
And a small typo fix in the corresponding failing test, although it doesn't
change match.

With regards,
Timur Bakeyev.