[SSSD] [PATCH] fix early free of sdap_handle

Eugene Indenbom eindenbom at gmail.com
Mon Apr 19 11:19:22 UTC 2010


On 04/16/2010 10:22 PM, Simo Sorce wrote:
> On Fri, 16 Apr 2010 19:00:57 +0400
> Eugene Indenbom<eindenbom at gmail.com>  wrote:
>
>    
>> Looks equivalent to what you have written, a bit more verbose may be.
>>      
> Ok in this case I think your patch is overkill.
> All we really need is to use the destructor to do what it is meant for,
> and just prevent freeing sdap_handle if we are recursing, and at the
> same time use a lock so that only one sdap_handle_release (the
> ""outermost"" does the job).
>
> Attached find a patch.
>    
Very clever solution. Actually I am pretty much surprised by talloc 
library. Allowing an object to cancel its own destruction is funny and 
really unexpected feature.
> Note that the patch still includes my old changes to avoid freeing
> ctx->gsh, they are *not* necessary, but I think they are good hygiene
> anyway so I left them in.
>    
I think that these changes need to go to separate patch. Look for 
further discussion below.
> The only changes really necessary are the ones to add the 2 booleans to
> sdap_handle in sdap.h and the changes to sdap_handle_destructor() and
> sdap_handle_release() in sdap_async.c
>
> These changes are much less invasive than the patch you propose and it
> looks to me they provide for the same outcome.
>
> Of course comments are welcome.
>    
I think that several more or less cosmetic additions could be of some 
value, so I have created my own version of the patch. I know that this 
looks more or less as not-invented-here syndrome, so you could discard 
the patch altogether without looking into it. I have attached it merely 
to illustrate the following points that seem valuable to me:

1. Better names for sh->locked and sh->release respectively could be 
sh->being_disconnected and sh->orphan. Some additional comments would 
also be of some help to future maintainers.

2. A new method sdap_handle_disconnect should be used to forcibly 
disconnect sdap_handle. Setting sh->connected = false (as in full 
version of patch) is no good as it leaves the LDAP connection open for 
indeterminate period of time and violates object encapsulation.

3. sdap_handle_release should unregister sh->connb from LDAP prior to 
deleting it. Although NULL pointer check is done LDAP callback, it's 
good policy to remove callbacks when no longer needed:
         ldap_get_option(sh->ldap, LDAP_OPT_CONNECT_CB, sh->conncb);

4. It's bad policy to destroy memory allocated and owned by other code 
(in sdap_handle_release):
         if (op == sh->ops) talloc_free(op);
     So I suggest to use early call of sdap_op destructor instead of 
deallocation (see my patch)

//-------------------------------------

So now we are ready to continue with fixing failover reconnect and 
GSSAPI authentication in LDAP and IPA providers. From my point of view 
at least the following problems needs to be addressed by final solution:

1. When two (or more) BE requests are executed in parallel and there is 
no cached connection, only one LDAP connection should be established. In 
current implementation 2 connections will be established and the first 
one killed failing the operation that connected first.

2. When OFFLINE state is detected during request execution (there were 
cached connection, but all failover servers failed to connect during 
request execution), the backend must return DP_ERR_OFFLINE. It currently 
returns DP_ERR_FATAL with EIO error. Next request completes with 
DP_ERR_OFFLINE. So there is a big inconsistency in behaviour.

3. It is essential to close LDAP connection before GSSAPI ticket is 
expired as closing connection with already expired ticket still writes a 
message in message log.

4. The about-to-expire connection should be closed gracefully: all 
requests already in progress and using the connection should be 
completed, new requests should establish and use new connection.

5. ipa_access backend should also use failover retries.

6. I think it is essential to reduce amount of copy-paste code handling 
LDAP connect/reconnect code. My strong opinion is that a special 
mechanism for handling LDAP connect/retry logic is required.

Eugene

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-Avoid-freeing-sdap_handle-too-early.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100419/2c4024d3/attachment.ksh>


More information about the sssd-devel mailing list