[SSSD] [PATCH] fix early free of sdap_handle

Eugene Indenbom eindenbom at gmail.com
Thu Apr 15 13:43:46 UTC 2010


On 04/15/2010 05:14 PM, Simo Sorce wrote:
> On Thu, 15 Apr 2010 10:13:46 +0400
> Eugene Indenbom<eindenbom at gmail.com>  wrote:
>
>    
>> On 04/13/2010 08:51 PM, Simo Sorce wrote:
>>      
>>> This patch attempts to address the problem Eugene found with
>>> sdap_handle_release()
>>>
>>> Simo.
>>>
>>>
>>>        
>> I have found 2 critical problems in Simo's patch:
>>
>> 1. After setting sdap_handle->connected = false, LDAP handle is never
>> destroyed. (see sdap_handle_release). So actually patch introduces
>> huge resource leak.
>>      
> Ahh, thanks for catching this one, I forgot to remove the sh->connected
> check in sdap_handle_release()
>
>    
>> 2. Despite all efforts not to free sdap_handle from callbacks it is
>> still freed when connect operation fails (together with connection
>> tevent_req). So the following sequence still reproduces the bug:
>>           a. Using debugger break in sdap_cli_rootdse_step
>>           b. Restart LDAP server or network connection leading to it
>>           c. Resume execution
>>           d. Observe crash
>>      
> This looks like a separate issue. Within the connect functions we are
> not using the global sdap_handle. I guess the issue here is that until
> we return from the connect request, the sh is a child of the request
> memory hierarchy, so when we call the callback within
> sdap_handle_release() we end up freeing the hierarchy.
>
> Do you confirm ?
>
>    
Yes.
>> Actually I still do not understand why it is feasible instead of
>> fixing destruction of sdap_handle in callback to invent a workaround
>> trying to avoid doing so.
>>      
> I am not sure why you call it a workaround, it is just a different
> solution.
>
>    
>> The bug in destruction of sdap_handle in callback violates basic
>> programming principles:
>>       - Every creatable object must by destructible (or in case of
>> reference counted object it must be possible to release
>> ownership/reference from object)
>>      
> This is not really true in a program based on talloc, as you can see a
> talloc hierarchy as a set of references. We do indeed almost
> exclusively base our destruction sequences on talloc hierarchies.
> The only case where this is not entirely possible is indeed the
> interface code between the program and the ldap libraries (for obvious
> reasons), and I want to keep the difference restricted in an area as
> small as possible. One of the reasons I do not like your approach is
> that the talloc hierarchy become very fuzzy.
>
>    
I agree to this principle, but some exceptions must be allowed at least 
for cases like this when we need to delay destruction of structure 
holding head of callback list.

NB In my patch only sdap_handle_data memory block is allocated on NULL 
context. All others are allocated on talloc hierarchy. It is absolutely 
required as this memory block is used by callback iteration. Therefore 
it is owned not only by sdap_handle itself, but by each stack frame 
iterating over callbacks (sdap_handle->ops member). This stack frame 
ownership is taken into account by destroy_lock, simple reference 
counting variable. There is no room to screw the things up here.

The whole technique is stock and widely used design pattern. I really do 
not understand why you are so reluctant to accept it. It's much simpler 
and cheaper in future maintenance than crasy rules on when it is allowed 
and when it is not allowed to destroy sdap_handle.

>>       - There should be no restriction on when method starting
>> destruction is called
>>      
> Not sure where you got this, but it is certainly not true :)
> You can't destroy objects in use. If you are thinking about deferred
> destruction that is what we want to achieve but using correct talloc
> hierarchies as much as possible. Unfortunately this is not the case,
> because of the interfcae we are forced into with the openldap libraries.
>
>    
If object is busy (like in our case) it simply starts destruction and 
completes it as soon as all dependent operations complete and it is not 
busy any longer.
>>       - After object destruction is started it is object's
>> responsibility to complete destruction as soon as possible. There
>> should not be any tricks like keeping an object in garbage collecting
>> queue.
>>      
> Unfortunately this is not possible as well, because pending requests
> all reference a common object (the one that ultimately holds the ldap
> handler).
>
>    
Yes, it is possible, and more over implemented in my patch. The only 
memory block left over and destroyed later after talloc_zfree is 
sdap_handle_data. All handles, file descriptors and in particular LDAP 
handle are destroyed or closed.
>> Violation of the above principles calls for __path__ coverage
>> analysis of code as we have to ensure that no __path__ of code
>> destructs object at wrong time. Even if Simo fixes the above problems
>> there will be no guarantee that sdap_handle destruction is never
>> called from callback.
>>      
> We just need to guarantee this, I will check again if I can steal some
> code from your patch though :)
>
>    
It's hopeless. There always will be a code path we overlook.
>> Regards, Eugene
>>
>> PS Although my patch never sets sdap_handle->connected to false, I
>> have attached an amendment to my patch to improve robustness of
>> destruction code when somebody really sets sdap_handle->connected to
>> false.
>>
>> PPS Even without this amendment no resource leaks exist in my patch
>> as all memory and callbacks are a bit later destroyed through talloc
>> ownership.
>>      
> Given you alloc private on NULL, you can hardly claim it is released
> through a talloc hierarchy, but it is explicitly freed in
> sdap_handle_data_release().
>
>    
Yes, I claim that my patch follow the talloc hierarchy. The only 
exception is sdap_handle_data memory block, which is freed when stack 
unwinds to top-level callback iterator. The exception is well defined 
and managed by 2 simple methods 
(sdap_handle_data_lock/sdap_handle_data_release).
> I know the sdap_handle destructor() will end up caling it (through
> sdap_handle_release(), but then why all the games with the final
> argument ? Why not simply attach a destructor to sdap_handle_data that
> prevents destruction until it is safe ?
>    
Because, sdap_handle_destructor and sdap_handle_release return 
__before__ sdap_handle_data is freed. All other objects are freed 
(sdap_handle itself, LDAP handle, callback data) and all aborted 
operations are notified. Only this sdap_handle_data memory block 
survives the destructor, because it is needed to signal end of callback 
iteration to outer sdap_handle_release.

The final argument signals that sdap_handle_data needs to be detached 
from parent sdap_handle and go into orphan state. In orphan state is 
self-destroys as soon as stack unwinds to top-level sdap_handle_release.
> Akso given we can prevent releasing sdap_handle too early through the
> destructor, why not simply apply all this to sdap_handle directly w/o
> requireing another sub-structure ?
>    
Because:
1. sdap_handle is not an opaque data structure. It's pointer is known to 
outside code which can accidentally call talloc_free. It's easier to 
allow calling talloc_free than to find all places where it is called. It 
also reduces amount of changed legacy code.

2. I want to have regular talloc parent for all other sub-objects of 
sdap_handle. This way we exclude from hierarchy only one memory block. 
This makes the solution more simple and robust.

Eugene



More information about the sssd-devel mailing list