Still need to write a testcase. Easy one acl 'ip' + anonymous search 'cn=config'

indentation is off in this code block

Should we being freeing cin_addr_aclip value, if it is set, before overwriting the value?

We'd need to check all the places that do a set of cin_addr_aclip because often with pblok the caller is responsible for the free, not the pblock, and if we free here we could be double free-ing.

I need to update the PR according to the comments. Just a comment about the testing phase.

The leak is reproducible with suites/acl/keywords_tests.py and the PR fixes it.
Basic and acl tests are successful.

You don't need to check if a pointer is NULL before freeing it. Free() was improved many years ago to do this check for you. Firstyear did have some concerns about this free though. Did you see his comment and double check if it is okay to free it here?

Right I will change this.

Regarding Firstyear concern, cin_addr_aclip is an ACL_ATTR_IP property added in the subject property list during the aci evaluation.
In theory it should be freed when we are done with aci evaluation (acl__done_aclpb). The concern is that at that time only property lists are freed but none of the property. I guess that it is because we do not know which type is the property and if a simple slapi_ch_free is good enough.
So the actual free of the property is postponed until the connection is reset by adding the property directly in the connection.
The only setting is done when we need to retrieve the PRAddrnet for ACI evaluation.

I'm assuming that this passes under ASAN/LSAN? My reading of the code doesn't show any major concerns now, @mreynolds do you have anything else?

@firstyear Sorry for the late answer... Indeed it passes successfully with ASAN/LSAN build

In that case I'm happy, provided it passes asan/lsan :)

You have my ack, I'll leave it to @mreynolds to follow up to be sure he's happy too.

@mreynolds any concern on this PR ?

Do we need to take the lock since we are not duplicating the value?

I was using the same locking approach as the others connections setting (auth type/method, client/server addr, conn flags...).
The idea behind locking is that serveral ops/event can get/set a given connection setting. I agree that client address should be set only once and I can not imagine concurrency at this point. But it looks simpler to not make exception when get/set connection setting.

Set, frees the setting so a reader may read something that will be freed (in theory) under it.

I was using the same locking approach as the others connections setting (auth type/method, client/server addr, conn flags...).
The idea behind locking is that serveral ops/event can get/set a given connection setting. I agree that client address should be set only once and I can not imagine concurrency at this point. But it looks simpler to not make exception when get/set connection setting.
Set, frees the setting so a reader may read something that will be freed (in theory) under it.

Yeah I have no strong objection to it, it was more of an observation. The value could still be freed when another thread is reading it, but if there is no concurrency potential then it's fine not string dupping it.

Regardless, you have my ack!

Commit a446d97f fixes this pull-request

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

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.