#49509 Indexing of internationalized matching rules is failing
Opened 2 years ago by tbordaz. Modified 3 months ago

Issue Description

When an attribute is indexed with matching rules from collation plugin, the indexer is not retrieved and indexing fails

Package Version and Platform

This bug exists in 1.3.6 and 1.3.7

Steps to reproduce

The failure was initially detected in I18n suite.
It is reproducible with the attached test case

Actual results

Indexing fails and warnings are logged in error logs

unknown or invalid matching rule

Expected results

Indexing should succeeds and


Metadata Update from @tbordaz:
- Custom field component adjusted to None
- Custom field origin adjusted to None
- Custom field reviewstatus adjusted to None
- Custom field type adjusted to None
- Custom field version adjusted to None

2 years ago

Metadata Update from @tbordaz:
- Custom field reviewstatus adjusted to review (was: None)

2 years ago

Copy/past of a review done by @lkrispen

First question

  • I do not see where a pblock where a pblock was memcopied before, and why you call your function mr_pblock_memcpy, it does copy a few addresses from on pb to another, but naybe I missed soemthing.
  • The memcpy were removed with git log -p -1 74c666b ../ldap/servers/slapd/plugin_mr.c.
    That is correct, the memcpy was not strictly necessary as only few input/output fields were interesting.

Second question

  • in the new memcpy function you copy pointers to arrays like KEYS or VALUES, but then you have two or more references to the same array, doesn't this lead to double free in pblock destroy ?
  • I reworked the first patch to only copy the expected fields. Those fields are describe in the interface of slapi_mr_indexer_create. (note that a field SLAPI_PLUGIN_MR_INDEX_SV_FN was missing in external documentation).

Copy/past of a last review part done by @lkrispen

  • before and after calling default_mr_indexer_create you do pblock copy stuff, but default_mr_indexer_create() does chnage the pblock only when it returns success and triggers the copying, why not pass the original pblock ? If it doesn't match nothin will be done and if it does you would copy the pblock changes anyway, so let it set it in the original pb.
    I do not know if this would apply for the loop as well, but it looks like on the first success the loop is terminated, so if the create fns called do change nothing if they do not apply you could also use the orig pb.
  • good point. Indeed for the new style mr-plugin (using default indexer) it looks good to use the original pblock (I attached a third patch for that). For the loop (old style) I am not so sure. It calls the indexer create function and the function could break the pblock somewhere. I think it may have been the reason of using a private pblock.

Thank you for writing test case also. :)

A few points we need to improve:

  • Python 3 support:
    We need to use lib389 functions for it and avoid modify_s, add_s, etc.
    I've updated documentation for Indexes with examples and links to source code:
    https://fedorapeople.org/~spichugi/html/indexes.html
    https://fedorapeople.org/~spichugi/html/guidelines.html#python-3-support

    Please, replace you code in function 'description_collation_indexed' with it.

  • Test file and function naming conventions:
    It's better to put new test cases directly to suites. It was decided that we need to get rid of tickets dir soon anyway.
    I think we can put the test case to something like 'suites/indexes/regression_test.py' (new directory)

    Then you can rename the test function with something sensible like 'test_collation_utf8' instead of 'test_ticket49509'.

  • Docstring:

    7850 +    """Specify a test case purpose or name here
    7851 +
    7852 +    :id: d2b02800-a903-4647-98e2-89e47d99df39
    7853 +    :setup: Fill in set up configuration here
    7854 +    :steps:
    7855 +        1. Create a UTF-8
    7856 +        2. Configure collation plugin indexes for 'description' attribute
    7857 +        3. import UTF-8 file and check no error indexing
    7858 +        4. dump 'description' index and check it contains collation keys
    7859 +    """
    

    We need to write a small description in the first line.
    :setup: will be something like 'Standalone instance'
    Besides :steps:, please, fill in :expectedresults: too (it can be either one number '1. It should compile', or the number matching steps, if it makes more sense)

    You can check any other test case in suites for an example:
    https://pagure.io/389-ds-base/blob/master/f/dirsrvtests/tests/suites/filter/filter_test.py#_20

  • Also, please, remove boilerplate code:

    7869 +    # If you need any test suite initialization,
    7870 +    # please, write additional fixture for that (including finalizer).
    7871 +    # Topology for suites are predefined in lib389/topologies.py.
    7872 +
    7873 +    # If you need host, port or any other data about instance,
    7874 +    # Please, use the instance object attributes for that (for example, topo.ms["master1"].serverid)
    7875 +
    7876 +    if DEBUGGING:
    7877 +        # Add debugging steps(if any)...
    7878 +        pass
    

@tbordaz There are a number of object oriented types for things like indexes on backends and others.

As well, I'd rather not see raw lidfs in the source: They could be python objects in the test case for our purposes, and even better, would be to add some of these data types to the dbgen.py in lib389 so we can test them in "many other" cases besides just this one,

If you need some examples, I can link them, but I concur with @spichugi's review :)

Thanks!

Metadata Update from @mreynolds:
- Custom field rhbz adjusted to https://bugzilla.redhat.com/show_bug.cgi?id=1531153

2 years ago

patch looks good to me now, maybe you split the fix and the testcase as tehre seem to be some requests regarding the testcase

I agree, like other pending ticket I was working on, patch will be split and testcase push will be postponed until testcase is python3.

Do you ack the C part of the patch ?

Note I will open a doc BZ as SLAPI_PLUGIN_MR_INDEX_SV_FN was missing in external doc.

Agreed, similar to the other case, you can merge C and test separately (but please don't forget about the tests :) )

Metadata Update from @lkrispen:
- Custom field reviewstatus adjusted to ack (was: review)

2 years ago

To ssh://pagure.io/389-ds-base.git
d3ba228..3a161b2 master -> master

Metadata Update from @tbordaz:
- Issue close_status updated to: fixed
- Issue set to the milestone: 1.3.7 backlog
- Issue status updated to: Closed (was: Open)

2 years ago

Metadata Update from @tbordaz:
- Issue set to the milestone: 1.3.6.0 (was: 1.3.7 backlog)
- Issue status updated to: Open (was: Closed)

2 years ago

Missing part of the current testcase
Backends.UTF-8.ldif

Metadata Update from @tbordaz:
- Issue close_status updated to: fixed

2 years ago

To ssh://pagure.io/389-ds-base.git
689b388..26f37b8 389-ds-base-1.3.6 -> 389-ds-base-1.3.6

Metadata Update from @tbordaz:
- Issue status updated to: Open (was: Closed)

2 years ago

Metadata Update from @tbordaz:
- Issue close_status updated to: fixed
- Issue status updated to: Closed (was: Open)

2 years ago

To ssh://pagure.io/389-ds-base.git
a2df295..4316907 389-ds-base-1.3.7 -> 389-ds-base-1.3.7

This is causing a serious regression, all server side sorting crashes the server with this patch.

Metadata Update from @mreynolds:
- Issue assigned to tbordaz
- Issue status updated to: Open (was: Closed)

a year ago
int
matchrule_values_to_keys(Slapi_PBlock *pb, struct berval **input_values, struct berval ***output_values)
{
    IFP mrINDEX = NULL;

    slapi_pblock_get(pb, SLAPI_PLUGIN_MR_INDEX_FN, &mrINDEX);
    slapi_pblock_set(pb, SLAPI_PLUGIN_MR_VALUES, input_values);
    if (mrINDEX) {
        mrINDEX(pb);
        slapi_pblock_get(pb, SLAPI_PLUGIN_MR_KEYS, output_values);
        return LDAP_SUCCESS;
    } else {
        return LDAP_OPERATIONS_ERROR;
    }
}

The crash happens because mrINDEX is not set, it returns an operations error and leaves the values NULL, which later get dereferenced

The crash happens because mrINDEX is not set, it returns an
operations error and leaves the values NULL, which later get
dereferenced

Where does it get dereferenced (do you have a stack trace/test case)?
It seems that if this (validly) sets an operation error, we should be
able to handle that.

So either we need to NOT return an op-error here, or we need the
caller to handle that mrINDEX could be NULL.

--
Sincerely,

William

@tbordaz - what is the state of this issue? Was this ever fixed? Can it be closed?

Metadata Update from @mreynolds:
- Issue set to the milestone: 1.4.2 (was: 1.3.6.0)

3 months ago

@mreynolds, the fix for this ticket has been pushed upstream since 1.3.6.

A possible regression (https://pagure.io/389-ds-base/issue/49509#comment-525208) is missing some details. In fact, server side sorting is working for me so I can not reproduce it. In addition all the calls to matchrule_values_to_keys look safe as output_values is initialized to NULL so if the function fails to retrieve a MR then output_values remains NULL an caller seem to handle it correctly.

So IMHO the remaining part of the ticket is the testcase. I wrote it 2 years ago but it was python2 style and I need to rewrite it.

@mreynolds, the fix for this ticket has been pushed upstream since 1.3.6.
A possible regression (https://pagure.io/389-ds-base/issue/49509#comment-525208) is missing some details. In fact, server side sorting is working for me so I can not reproduce it. In addition all the calls to matchrule_values_to_keys look safe as output_values is initialized to NULL so if the function fails to retrieve a MR then output_values remains NULL an caller seem to handle it correctly.
So IMHO the remaining part of the ticket is the testcase. I wrote it 2 years ago but it was python2 style and I need to rewrite it.

Cool, well when you rewrite the CI test please feel free to close this ticket. Thanks!

Login to comment on this ticket.

Metadata