test patch

Code wise it looks okay, but I think that @mreynolds or @tbordaz may know the implications of this better than I do.

The code looks good but I am unsure if the prim_csn may stay in the thread private area longer than expected.

If writing in the CL fails, thread_priv.prim_csn will keep the prim_csn value. Later ruv_cancel_csn_inprogress will cleanup the pending list but not the thread_priv.prim_csn. Shouldn't it ?
Else I wonder if there is a risk that thread_priv.prim_csn may keep an old csn and a new incoming operation will not be consider as a prim_csn because thread_priv.prim_csn is not NULL.

I think you are right, in a replicated operation this could be hidden if the operation is retried. But it is not freed, so it would be a different bug than this ASAN use after free

on a second look it seems to work. If an operation fails, eg because of a plugin failure not because of failing to write the cl, then write_changelog_and_ruv() calls cancel_opcsn() and then goes to common_return and sets the thread_priv.prim_csn to NULL

there could be an issue if an already seen csn triggers the free of the primary csn, I have not understood how this could happen, this patch adds some logging around the suspicious code

Ack from me on this, I think @tbordaz seems happy too?

Here is a new cumulative fix that successfully passed the tests

Do you want me to run this under asan?

Thanks for asking !!
@vashirov already did 6 asan runs (with the patch) without hitting the issue. Before (without the patch) at least one of the run systematically hit the pb.
@firstyear do you know how your run differs from @vashirov ones ? if it is the same then no need to do additional runs else if you have enough bandwidth that would be a great help :)

They should be the same :) I'll just review the code then :)

One comment....

116 +»       »       »       »       slapi_log_err(SLAPI_LOG_INFO, repl_plugin_name,
117 +»       »       »       »       »       "replica_get_exclusive_access - "
118 +»       »       »       »       »       "This is a second acquire attempt from the same replica connection "
119 +»       »       »       »       »       " - return success instead of busy\n");

This should be REPL logging instead of INFO

Thanks for reviewing this, I updated the patch

Looks good, thanks!

git push origin master

Counting objects: 10, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (10/10), done.
Writing objects: 100% (10/10), 2.98 KiB | 0 bytes/s, done.
Total 10 (delta 8), reused 0 (delta 0)
To ssh://git@pagure.io/389-ds-base.git
e659e32..3bc42bf master -> master

git push origin 389-ds-base-1.3.6

Counting objects: 10, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (10/10), done.
Writing objects: 100% (10/10), 2.98 KiB | 0 bytes/s, done.
Total 10 (delta 8), reused 0 (delta 0)
To ssh://git@pagure.io/389-ds-base.git
9563d29..1947503 389-ds-base-1.3.6 -> 389-ds-base-1.3.6

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

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.