[SSSD] [PATCH] fix early free of sdap_handle

Simo Sorce ssorce at redhat.com
Thu Apr 15 18:22:25 UTC 2010


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

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 ?

Simo.

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



More information about the sssd-devel mailing list