[SSSD] [PATCH] fix early free of sdap_handle

Simo Sorce ssorce at redhat.com
Thu Apr 15 13:14:37 UTC 2010


On Thu, 15 Apr 2010 10:13:46 +0400
Eugene Indenbom <eindenbom at gmail.com> wrote:

> On 04/13/2010 08:51 PM, Simo Sorce wrote:
> >
> > This patch attempts to address the problem Eugene found with
> > sdap_handle_release()
> >
> > Simo.
> >
> >    
> I have found 2 critical problems in Simo's patch:
> 
> 1. After setting sdap_handle->connected = false, LDAP handle is never 
> destroyed. (see sdap_handle_release). So actually patch introduces
> huge resource leak.

Ahh, thanks for catching this one, I forgot to remove the sh->connected
check in sdap_handle_release()

> 2. Despite all efforts not to free sdap_handle from callbacks it is 
> still freed when connect operation fails (together with connection 
> tevent_req). So the following sequence still reproduces the bug:
>          a. Using debugger break in sdap_cli_rootdse_step
>          b. Restart LDAP server or network connection leading to it
>          c. Resume execution
>          d. Observe crash

This looks like a separate issue. Within the connect functions we are
not using the global sdap_handle. I guess the issue here is that until
we return from the connect request, the sh is a child of the request
memory hierarchy, so when we call the callback within
sdap_handle_release() we end up freeing the hierarchy.

Do you confirm ?

> Actually I still do not understand why it is feasible instead of
> fixing destruction of sdap_handle in callback to invent a workaround
> trying to avoid doing so.

I am not sure why you call it a workaround, it is just a different
solution.

> The bug in destruction of sdap_handle in callback violates basic 
> programming principles:
>      - Every creatable object must by destructible (or in case of 
> reference counted object it must be possible to release 
> ownership/reference from object)

This is not really true in a program based on talloc, as you can see a
talloc hierarchy as a set of references. We do indeed almost
exclusively base our destruction sequences on talloc hierarchies.
The only case where this is not entirely possible is indeed the
interface code between the program and the ldap libraries (for obvious
reasons), and I want to keep the difference restricted in an area as
small as possible. One of the reasons I do not like your approach is
that the talloc hierarchy become very fuzzy.

>      - There should be no restriction on when method starting 
> destruction is called

Not sure where you got this, but it is certainly not true :)
You can't destroy objects in use. If you are thinking about deferred
destruction that is what we want to achieve but using correct talloc
hierarchies as much as possible. Unfortunately this is not the case,
because of the interfcae we are forced into with the openldap libraries.

>      - After object destruction is started it is object's
> responsibility to complete destruction as soon as possible. There
> should not be any tricks like keeping an object in garbage collecting
> queue.

Unfortunately this is not possible as well, because pending requests
all reference a common object (the one that ultimately holds the ldap
handler).

> Violation of the above principles calls for __path__ coverage
> analysis of code as we have to ensure that no __path__ of code
> destructs object at wrong time. Even if Simo fixes the above problems
> there will be no guarantee that sdap_handle destruction is never
> called from callback.

We just need to guarantee this, I will check again if I can steal some
code from your patch though :)

> Regards, Eugene
> 
> PS Although my patch never sets sdap_handle->connected to false, I
> have attached an amendment to my patch to improve robustness of
> destruction code when somebody really sets sdap_handle->connected to
> false.
> 
> PPS Even without this amendment no resource leaks exist in my patch
> as all memory and callbacks are a bit later destroyed through talloc
> ownership.

Given you alloc private on NULL, you can hardly claim it is released
through a talloc hierarchy, but it is explicitly freed in
sdap_handle_data_release().

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 ?

Simo.

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



More information about the sssd-devel mailing list