[SSSD] [PATCH] fix early free of sdap_handle

Eugene Indenbom eindenbom at gmail.com
Fri Apr 16 06:02:08 UTC 2010


On 04/15/2010 10:22 PM, Simo Sorce wrote:
> On Thu, 15 Apr 2010 20:05:17 +0400
> Eugene Indenbom<eindenbom at gmail.com>  wrote:
>
>    
>> On 04/15/2010 07:16 PM, Simo Sorce wrote:
>>      
>>> On Thu, 15 Apr 2010 17:58:18 +0400
>>> Eugene Indenbom<eindenbom at gmail.com>   wrote:
>>>
>>>
>>>        
>>>> On 04/15/2010 05:35 PM, Simo Sorce wrote:
>>>>
>>>>          
>>>>> On Thu, 15 Apr 2010 09:14:37 -0400
>>>>> Simo Sorce<ssorce at redhat.com>    wrote:
>>>>>
>>>>>
>>>>>
>>>>>            
>>>>>> 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 ?
>>>>>> 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 ?
>>>>>>
>>>>>>
>>>>>>              
>>>>> Ah btw, while thinking about how you handle release of
>>>>> sdap_handle_data I think I found a case where you end up with some
>>>>> issues.
>>>>>
>>>>> If you free sdap_handle while sdap_handle_data is locked
>>>>> sdap_handle_release will be called with final = ture, and this
>>>>> means sh->private is set to NULL.
>>>>> If later on sdap_handle_release is called again shd will be NULL
>>>>> (shd = sh->private) and sdap_handle_data_lock(shd); wiull
>>>>> derefernce a NULL pointer causing a segfault.
>>>>>
>>>>>
>>>>>
>>>>>            
>>>> No, it will not. sdap_handle_data->sh is never dereferenced. :-)
>>>> As I have written earlier it is simply a flag showing that
>>>> sdap_handle_data still have a parent.
>>>> When sdap_handle_data->sh is zero the structure deletes itself as
>>>> soon as sdap_handle_data->destroy_lock is zero too (see
>>>> sdap_handle_data_release).
>>>>
>>>> Shall ever back pointer sdap_handle_data->sh be dereferenced, it
>>>> needs to be checked for NULL before use. That's clear.
>>>>
>>>>          
>>> Sorry, I don't understand your comment, I never once mentioned
>>> sdap_handle_data->sh
>>>
>>>        
>> Oops, I have got your concern wrong for the first time.
>>
>> sh->private is set to NULL when and only when final == true, that
>> means we are called from sdap_handle_destructor, so when immediately
>> after that sdap_handle will be deleted by talloc.
>>
>> So there is no chance of having sdap_handle where
>> sdap_handle->private = NULL. I have set sdap_handle->private to NULL
>> merely for housekeeping to completely break bi-link sdap_handle->sh
>> <=>  sdap_handle_data->shd.
>>
>> Actually it looks like that sequence of actions during destruction is
>> not clear.
>> Well, it's common case for me. I take a lot of complicated staff for
>> granted.
>>
>> The sequence of actions in recursive destruction sequence:
>> 1. sdap_process_result is called
>> 2. sdap_handle_release(final = false) is called
>> 3. conncb destroyed
>> 3. op->callback is called
>> 4. talloc_zfree(gsh) is called
>> 5. sdap_handle_destroy is called
>> 6. sdap_handle_release(final = true) is called
>> 7. LDAP handle destroyed
>> 8. sh->private = NULL, shd->sh = NULL
>> 9. sdap_handle_release(final = true) returns
>> 10. sdap_handle_destroy returns
>> =>  11. sdap_handle is destroyed by talloc (so there is no chance of
>> dereferencing sh->private)
>> 12. op->callback returns
>> =>  13. sdap_handle_data_release destroys sdap_handle_data
>> 14. sdap_handle_release(final = false) returns
>> 15. sdap_process_result returns
>>
>> The sequence of actions in normal destruction sequence:
>> 1. sdap_handle_destroy is called
>> 2. sdap_handle_release(final = true) is called
>> 3. conncb destroyed
>> 4. LDAP handle destroyed
>> 8. sh->private = NULL, shd->sh = NULL
>> =>  9. sdap_handle_data_release destroys sdap_handle_data
>>       NB that's why I set sh->private = NULL, here sdap_handle_data is
>> destroyed first
>> 10. sdap_handle_release(final = true) returns
>> 11. sdap_handle_destroy returns
>> =>  12. sdap_handle is destroyed by talloc (so there is no chance of
>> dereferencing sh->private)
>>
>> Therefore setting both sh->private and shd->sh to NULL serves the
>> purpose of marking relationship between them broken. So if any code
>> needs to be executed after that (currently there is no such code) is
>> could check whether sdap_handle_data is orphan or sdap_handle is
>> almost destroyed so it does not even have inner sdap_handle_data
>> object.
>>      
> Thanks for posting the sequence, I managed to come up with it before
> though, and there is a case you are not considering.
>
> You said you want to be resistant to random callback freeing the
> sdap_handle, so this is the sequence that still gives me trouble:
>
> 1. sdap_process_result is called
> 2. sdap_handle_release(final = false) is called
> 3. conncb destroyed
> 3. op->callback is called
> 4. talloc_zfree(gsh) is called
> 5. sdap_handle_destroy is called
> 6. sdap_handle_release(final = true) is called
> 7. netx op->callback is called
> 8. talloc_zfree(gsh) is called (2nd time .. not good)
>
> I don't see how you prevent this in your code, given ctx->gsh is zeroed
> only when talloc_zfree() returns.
>    
This problem was one of major issues when I have started to work on 
failover reconnect problem.

The complete set of my patches addressed the issue by implementing 
reference counting of connection usage (see 
https://fedorahosted.org/pipermail/sssd-devel/2010-April/003113.html, 
patch #4). When reference counting is in place, sdap_handle is not 
destroyed until all operations complete. So inner 
sdap_handle_release(final = true) calls no callbacks.

So after I suggested patch #3 as a separate fix for "early free of 
sdap_handle" problem, I have looked into this issue again and found the 
deficiency in talloc_zfree you have mentioned above. The I have 
developed a simple patch to fix it, but was holding it back for a while 
waiting for this discussion to calm down.

Now I am sending this patch in separate e-mail for approval.

> This is why I very much would prefer to avoid recursive loops within
> sdap_handle_release rather than just try to use locks to deal with
> them, and hence why the rule to never free ctx->gsh.
>
>    
I agree that it would be good to prevent or at least limit recursion of 
callbacks, but unfortunately as callbacks are called from destructor it 
is not possible to resolve within this patch.

IMHO it is very bad policy to call callbacks from destructor. My patch 
#4 resolves the issue by introducing reference counting of sdap_handle.

> One way to do that is to prevent freeing ctx->gsh by returning -1 in
> the destructor if we are recursing and checking the talloc_free()
> return to decide whether to zero ctx->gsh or not depending on whether
> the free was successful.
>
> Another way, maybe, is to NULL ctx->gsh before actually freeing it, but
> this again is a special rule on how to free ctx->gsh, so we can more
> easily just ban freeing ctx->gsh as I was trying to do in my patches.
>
> ...
>
> ok, I guess another way is to make sure we loop over shd->ops only in
> the outermost sdap_handle_release() ... I guess we can do that using a
> flag to be set the first time we enter sdap_handle_release ...
>
> Comments anyone ?
>    
It is not possible to call callback in outer sdap_handle_release(final = 
false), as all callbacks must complete before sdap_handle is destroyed 
(that is sdap_handle_release(final = true) completes).

On the other hand it is an absolute must to prevent double destruction 
of sdap_handle. This could be achieved by fixing talloc_zfree. Therefore 
in worst case the recursion is limited by to levels:
     - sdap_handle_release(final = false)
     - sdap_handle_release(final = true)

The third level of sdap_handle_release(final = true) would mean double 
destruction, that is obviously a disaster.

Eugene




More information about the sssd-devel mailing list