[SSSD] [PATCH] fix early free of sdap_handle

Eugene Indenbom eindenbom at gmail.com
Thu Apr 15 13:58:18 UTC 2010


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.
> I haven't checked if this can actually happen, but given the ways you
> use locks here it looks like it might.
>
>    
Sure this could not happen. I have __checked__ it and actually run my 
test scenario using step-by-step execution in debugger. I have 
reproduced the situation in debugger and made sure that both inner 
sdap_handle_destructor and outer sdap_handle_release complete without 
errors and that sdap_handle_data is actually freed (talloc_free is 
called). I have done all that.

Eugene



More information about the sssd-devel mailing list