[SSSD] [PATCH] fix early free of sdap_handle

Eugene Indenbom eindenbom at gmail.com
Fri Apr 30 07:39:20 UTC 2010


On 04/29/2010 09:47 PM, Simo Sorce wrote:
> On Mon, 19 Apr 2010 15:19:22 +0400
> Eugene Indenbom<eindenbom at gmail.com>  wrote:
>
>    
>> 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.
>>      
> If you think hard about it, it actually makes a lot of sense to avoid
> and prevents recursion, which can never be a good thing when freeing
> resources.
>
>    
Sure it's convenient. I just say it is unusual, at least for seasoned 
C++ programmer.
>>> 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.
>>      
> Ok I split the patch in 2 parts.
>
>    

0002-Avoid-freeing-sdap_handle-too-early.patch is mostly OK. I have only 
one concern with it, see below.

0001-Better-handle-sdap_handle-memory-from-callers.patch, from my point 
of view, should be dropped or integrated into later patch addressing 
failover reconnect problem as:
     - The patch itself does not solve failover reconnect problems (it 
just adds a bit of robustness that is good, but is not sufficient)
     - 0002-Avoid-freeing-sdap_handle-too-early.patch fixes LDAP backend 
crash on its own and therefore failover reconnect problem could wait for 
compete solution.
> <...>
>
>> 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)
>>      
> In this case although the memory is child of the original requests, the
> semantic owner is the sdap_handle(), so it is eantirely appropriate to
> free the operations if sdap_handle is being freed, for the simple
> reason that the "technical" owner has no access to those objects and
> they would be just hanging memory to their requests.
>
>
>    
I still disagree with this approach. The memory must be deleted only by 
owner. If it is not deleted after callback completes, 
sdap_handle_release must assume that it is still needed for some 
purpose. Destroying memory that is not owned is a sure way to problems.

Nevertheless in current sdap_handle usage, sdap_op is __always__ deleted 
by owner. So it would make no major harm if you stay with your point of 
view. It actually does not matter in current state of development.

Eugene



More information about the sssd-devel mailing list