[SSSD] [PATCH] fix early free of sdap_handle

Eugene Indenbom eindenbom at gmail.com
Thu Apr 15 16:05:17 UTC 2010


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.

Regards, Eugene



More information about the sssd-devel mailing list