[SSSD] [PATCH] fix early free of sdap_handle

Simo Sorce ssorce at redhat.com
Thu Apr 15 15:16:07 UTC 2010


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

> > 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.

I trust you did. That's not the point.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York



More information about the sssd-devel mailing list