You don't have to NULL check here, charray_free() can handle NULL. Same thing for the password user attribute code below...

If you haven't done so can you please do a quick ASAN test to make sure we are not leaking anything?

You don't have to NULL check here, charray_free() can handle NULL. Same thing for the password user attribute code below...

Sure!

If you haven't done so can you please do a quick ASAN test to make sure we are not leaking anything?

Yeah, about that... I have two news:

1. I've done it and my code is leak-free as far as I am aware;

2. I've spent half of the day debugging and trying to understand how my code affects the password policy passwordMaxSeqSets attrubute because the test has started to fail...

   Then, after some time, I've checked the ASAN build on master and it also has failed. So...
   Somehow, passwordMaxSeqSets fails only on ASAN build. All the tests pass on our usual RPM build (and my PR is okay too).

I've created an issue for further investigation:
https://pagure.io/389-ds-base/issue/51013

Nice catch, gotta love ASAN :-)

Ack for this issue!

If this islocking and changing the config, maybe it should be in libglobs.c?

Is there a test for the badwords array changes?

why 'all possible mechs' ? This is related to forbiden password words not mechanism. Am I missing something ?

Same, is it 'mechs' or 'password' ?

Is it a space " " separator ? I thought it was comma separator.

why not strcasecmp

I thought it was comma separated list.
Is this list valid ? "passwordUserAttributes: telephonenumber, givenname,cn" (note the space before 'givenname')

If this islocking and changing the config, maybe it should be in libglobs.c?

But it is in libglobs.c... Do I miss something?

Is there a test for the badwords array changes?

Good catch, gonna add it!

I thought it was comma separated list.
Is this list valid ? "passwordUserAttributes: telephonenumber, givenname,cn" (note the space before 'givenname')

It is not valid and it was not (the way the code was written before).
But after you said that, I checked our docs, and it says "comma-separated list".
So I am gonna add the functionality. Thanks!

why not strcasecmp

I took it from the original code. We need to check for a substring here (if our password has the bad word or not).

And... Looks like we got some indentation issues here :-p @firstyear, sorry I didn't catch that sooner, maybe it's just your patches where the indentation stands out more haha

Changes are done! Please, review

Should be size_t, and it should be declared inside of the for loop: for (size_t i = 0; ...

Should be size_t, and it should be declared inside of the for loop: for (size_t i = 0; ...

Besides this issue, if this build passes ASAN tests then ACK from me.

I think it initvalue should be NULL not an empty string. "" is pointer in data segment (IIRC) and slapi_ch_free_string will not like that.

The same

comment does not apply any more

idem

Comma-separated list..

idem

minor point
It looks similar to replace_char defined in 'vlv.c'.
You could move replace_char from vlv.c to util.c and call "replace_char(s, ',', ' ')"

minor point
It looks similar to replace_char defined in 'vlv.c'.
You could move replace_char from vlv.c to util.c and call "replace_char(s, ',', ' ')"

Good catch! Thanks!

Should be size_t, and it should be declared inside of the for loop: for (size_t i = 0; ...

Done.

I think it initvalue should be NULL not an empty string. "" is pointer in data segment (IIRC) and slapi_ch_free_string will not like that.

If we set the init value to NULL and then if we try to delete the attribute, it won't reset the value and it will throw LDAP_UNWILLING_TO_PERFORM. More info here:
https://pagure.io/389-ds-base/blob/master/f/ldap/servers/slapd/libglobs.c#_40
And the code itself is here:
https://pagure.io/389-ds-base/blob/master/f/ldap/servers/slapd/libglobs.c#_7983

I think we still want to be able to reset the value (and not setting it manually to "")...

Changes are made. Please, review :)

!= NULL?

Otherwise all good, thanks!

The patch looks good to me but I have a global concern regarding CONFIG_STRING config attribute.
the initvalue of such attributes is an initialized string, that would be on data segment (so no malloc).
The set function of those attributes usually slapi_ch_free_string the former value with the new value.
so IMHO that means a 'free' of buffer that was not malloc !.
Anyone could give a second look at this ?

@spichugi, did you test asan build with setting of 'pw_bad_words' ? anything reported ?

Opps... My fault ! This is valid. Actually the magic is done by the set_value callback that malloc/duplicate the initvalue.
So no problem with these settings of CONFIG_STRING. sorry for the noise

You have my ack.

@spichugi, did you test asan build with setting of 'pw_bad_words' ? anything reported ?

Everything is clean.
Thank you!

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

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.