Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue 6494 - Various errors when using extended matching rule on vlv sort filter #6495

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Issue 6494 - Various errors when using extended matching rule on vlv sort filter #6495

wants to merge 1 commit into from

Conversation

progier389
Copy link
Contributor

Various issues when configuring and using extended matching rule within a vlv sort filter:

  1. Race condition about the keys storage while indexing leading to various heap and data corruption. (lmdb only)
  2. Crash while indexing if vlv are misconfigured because NULL key is not checked.
  3. Read after block because of data type mismatch between SlapiValue and berval
  4. Memory leaks

Solution:

  1. Serialize the vlv index key generation if vlv filter has an extended matching rule.
  2. Check null keys
  3. Always provides SlapiValue even ifg we want to get keys as bervals
  4. Free properly the resources

Issue: #6494

Reviewed by: ?

progier389
progier389 commented Jan 10, 2025
View reviewed changes
ldap/servers/slapd/back-ldbm/db-mdb/mdb_import_threads.c
/*
* Determine if vlv require extended matching rule evaluation
*/
static int
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shoul use C11 bool

mreynolds389
mreynolds389 reviewed Jan 10, 2025
View reviewed changes
ldap/servers/slapd/back-ldbm/dblayer.c
dblayer_destroy_txn_stack(void)
{
/* Cleanup for the main thread to avoid false/positive leaks from libasan */
void *txn_stack = PR_GetThreadPrivate(thread_private_txn_stack);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused variable, but why "get" it if you just "set" it to NULL on the next line?

mreynolds389
mreynolds389 approved these changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@mreynolds389 mreynolds389 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a covscan run as well, and didn't see any regressions. Ack!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mreynolds389 mreynolds389 mreynolds389 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@progier389 @mreynolds389