[SSSD] [PATCH] fix early free of sdap_handle

Sumit Bose sbose at redhat.com
Mon May 3 09:21:38 UTC 2010


On Thu, Apr 29, 2010 at 01:47:42PM -0400, Simo Sorce wrote:
> On Mon, 19 Apr 2010 15:19:22 +0400
> Eugene Indenbom <eindenbom at gmail.com> wrote:
> 
> > On 04/16/2010 10:22 PM, Simo Sorce wrote:
> > > On Fri, 16 Apr 2010 19:00:57 +0400
> > > Eugene Indenbom<eindenbom at gmail.com>  wrote:
> > >
> > >      
> > >> Looks equivalent to what you have written, a bit more verbose may
> > >> be. 
> > > Ok in this case I think your patch is overkill.
> > > All we really need is to use the destructor to do what it is meant
> > > for, and just prevent freeing sdap_handle if we are recursing, and
> > > at the same time use a lock so that only one sdap_handle_release
> > > (the ""outermost"" does the job).
> > >
> > > Attached find a patch.
> > >      
> > Very clever solution. Actually I am pretty much surprised by talloc 
> > library. Allowing an object to cancel its own destruction is funny
> > and really unexpected feature.
> 
> If you think hard about it, it actually makes a lot of sense to avoid
> and prevents recursion, which can never be a good thing when freeing
> resources.
> 
> > > Note that the patch still includes my old changes to avoid freeing
> > > ctx->gsh, they are *not* necessary, but I think they are good
> > > hygiene anyway so I left them in.
> > >      
> > I think that these changes need to go to separate patch. Look for 
> > further discussion below.
> 
> Ok I split the patch in 2 parts.
> 
> > > The only changes really necessary are the ones to add the 2
> > > booleans to sdap_handle in sdap.h and the changes to
> > > sdap_handle_destructor() and sdap_handle_release() in sdap_async.c
> > >
> > > These changes are much less invasive than the patch you propose and
> > > it looks to me they provide for the same outcome.
> > >
> > > Of course comments are welcome.
> > >      
> > I think that several more or less cosmetic additions could be of some 
> > value, so I have created my own version of the patch. I know that
> > this looks more or less as not-invented-here syndrome, so you could
> > discard the patch altogether without looking into it. I have attached
> > it merely to illustrate the following points that seem valuable to me:
> 
> Ok I took some advice, but my patch remains mostly unchanged.
> 
> > 1. Better names for sh->locked and sh->release respectively could be 
> > sh->being_disconnected and sh->orphan. Some additional comments would 
> > also be of some help to future maintainers.
> 
> I agree the names where not the best I could think, but I prefer to
> give names that represent the function of the variable rather than what
> is supposedly going to happen when the variable is true.
> 
> I renamed lock -> destructor_lock, so it is clear what it is locking.
> 
> I also renamed release -> release_memory, orphan doesn't fit as memory
> is not really orphan of anything, it is still child of whatever parent
> until freed, but we want to signal that memory needs be released once
> we are done.
> 
> > 2. A new method sdap_handle_disconnect should be used to forcibly 
> > disconnect sdap_handle. Setting sh->connected = false (as in full 
> > version of patch) is no good as it leaves the LDAP connection open
> > for indeterminate period of time and violates object encapsulation.
> 
> I' leave this for a following patch if necessary.
> 
> > 3. sdap_handle_release should unregister sh->connb from LDAP prior to 
> > deleting it. Although NULL pointer check is done LDAP callback, it's 
> > good policy to remove callbacks when no longer needed:
> >          ldap_get_option(sh->ldap, LDAP_OPT_CONNECT_CB, sh->conncb);
> 
> This is now taken care of automatically with latest Sumit patches,
> thanks for pointing it out.
> 
> > 4. It's bad policy to destroy memory allocated and owned by other
> > code (in sdap_handle_release):
> >          if (op == sh->ops) talloc_free(op);
> >      So I suggest to use early call of sdap_op destructor instead of 
> > deallocation (see my patch)
> 
> In this case although the memory is child of the original requests, the
> semantic owner is the sdap_handle(), so it is eantirely appropriate to
> free the operations if sdap_handle is being freed, for the simple
> reason that the "technical" owner has no access to those objects and
> they would be just hanging memory to their requests.
> 
> 
> 
> I propose the attached patches for inclusion in master.
> I'd like to divert any modification that is not an intrinsic bug in the
> patches to later patches and a separate thread so we can move on :)
> 
> Simo.
> 
> -- 
> Simo Sorce * Red Hat, Inc * New York

ACK to both.

I have seen Eugene comments on the patches and do not want to argure
that these patches make the code only cleaner and more robust. But I
think these improvements are still worth to be added to the current code
base and some of them might get lost if we postponed them to a later
patch.

bye,
Sumit



More information about the sssd-devel mailing list