LGTM!

Ack from me too, thanks! (Apologies for william of the past for messing this up :) )

@spichugi, I am sorry but I miss how your fix fixes the issue :(
allowed_sasl_mechs was/is set to nval (list of space separated value).
allowed_sasl_mechs_array was set to a array of value built from nval.
So the two fields were not pointing to the same structure. I miss how the patch changes that.

@spichugi, I am sorry but I miss how your fix fixes the issue :(
allowed_sasl_mechs was/is set to nval (list of space separated value).
allowed_sasl_mechs_array was set to a array of value built from nval.
So the two fields were not pointing to the same structure. I miss how the patch changes that.

Yeah, it was my initial thought too. So I went checking all other parts and spent some time with it...
But everything else was okay.

And then, assuming that slapi_str2charray_ext makes a copy of str *, I've decided to awatch the slapdFrontendConfig->allowed_sasl_mechs piece of memory to see where it is modified and truncated (because I thought that something may happen cuncurrently when we unlock slapdFrontendConfig).

But it's turned out that the issue happens in slapi_str2charray_ext where we work with a pointer to str (our string we want to convert to an array)
https://pagure.io/389-ds-base/blob/master/f/ldap/servers/slapd/charray.c#_347

So when we find a separator (i.e. " "), we get the pointer and we set it to 0.
https://pagure.io/389-ds-base/blob/master/f/ldap/servers/slapd/utf8.c#_306

So basically, the array forallowed_sasl_mechs_array that we get from slapi_str2charray_ext is a former nval string. The same char * we set to allowed_sasl_mechs.

slapi_str2charray_ext is called with 'nval'. It will create an array of value that is different (address) than 'nval'. BUT it modifies 'nval' (during ldap_utf8strtok_r).

for example before the call slapi_str2charray_ext(nval, " ", 0)
slapdFrontendConfig->allowed_sasl_mechs = "PLAIN GSSAPI" slapdFrontendConfig->allowed_sasl_mechs_array = NULL

After the call
slapdFrontendConfig->allowed_sasl_mechs = "PLAIN"
slapdFrontendConfig->allowed_sasl_mechs_array = {"PLAIN" , "GSSAPI"}
but actually slapdFrontendConfig->allowed_sasl_mechs = "PLAIN\0GSSAPI"

Is the bug trigger by the shorten value of slapdFrontendConfig->allowed_sasl_mechs ?

IMHO a problem is in slapi_str2charray_ext that should duplicate 'str' at the beginning of the function because it may corrupt it.

IMHO a problem is in slapi_str2charray_ext that should duplicate 'str' at the beginning of the function because it may corrupt it.

I get the impression that it is an intended behavior... For example, here we work with slapi_str2charray_ext function using tmp_string - https://pagure.io/389-ds-base/blob/master/f/ldap/servers/plugins/replication/repl5_agmt.c#_535

So I didn't want to modify the existing logic in other places where the function is used.

I am not opposed to changing the function's behavior if you think that it is justified.
I really think it is about what we decide and how we define its interface.

But maybe we should create a separate issue for that? (as the function is used with a tmp_string in other places and it looks like it's intended)

All the others places using slapi_str2charray(_ext) are calling it with temporary string, freed immediately after.

I am fine keeping slapi_str2charray_ext with the current behavior.
Please add warning comment in the head of slapi_str2charray_ext stating that it can change the input string.
Also in your patch you may comment that tmp_array_nval is used (instead of nval) because slapi_str2charray can change it.

Thanks! I added the comments.

Thanks. This looks good to me as well. Ack

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 pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4050

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.