[SSSD] [PATCH] fix early free of sdap_handle

Simo Sorce ssorce at redhat.com
Fri Apr 16 14:17:48 UTC 2010


On Fri, 16 Apr 2010 10:02:08 +0400
Eugene Indenbom <eindenbom at gmail.com> wrote:

> I agree that it would be good to prevent or at least limit recursion
> of callbacks, but unfortunately as callbacks are called from
> destructor it is not possible to resolve within this patch.

Well, I'll think about that.

> IMHO it is very bad policy to call callbacks from destructor. My
> patch #4 resolves the issue by introducing reference counting of
> sdap_handle.

calling callbacks from a destructor is perfectly fine, it is the method
used to clean-up and reply to callers if something went wrong, I don't
see anything bad with that.

What is important is that callbacks do not attempt to recurse (starting
new requests) when there are errors. This is a general implicit rule
of the tevent model anyway, not something we are making up here for
this case.

> It is not possible to call callback in outer
> sdap_handle_release(final = false), as all callbacks must complete
> before sdap_handle is destroyed (that is sdap_handle_release(final =
> true) completes).
> 
> On the other hand it is an absolute must to prevent double
> destruction of sdap_handle. This could be achieved by fixing
> talloc_zfree. Therefore in worst case the recursion is limited by to
> levels:
>      - sdap_handle_release(final = false)
>      - sdap_handle_release(final = true)
> 
> The third level of sdap_handle_release(final = true) would mean
> double destruction, that is obviously a disaster.

Ok, let me try to summarized what is the problem and what we need to do
in abstract here, because we are going down a rat hole of exceptions
and I think we are loosing the point.

There are only 2 ways for sdap_handle_release() to be called.
1. from sdap_process_result() in case the ldap handler returns a fatal
error.
2. from sdap_handle_destructor when someone tries to free the handle.

In any case what we want is to signal all pending requests that they
need to terminate either because we want to close the connection
immediately or because there was an error.

In any case, ideally we never want sdap_handle_release to recurse,
because we are already cleaning up and recursing causes issues.

The only difference between the 2 patch to get to sdap_handle_release
is that if the destructor calls it we want the handle to be freed at
the end of the outermost loop. While if we call it only from
sdap_process_result we do not want to free it because other functions
may still reference ctx->gsh, so we just want to release everythin but
the memory context itself and only to avoid segfault if callers try to
dereference it. But no caller will ever use the handle anymore because
sh->connected is false.

Is there anything I am missing here ?

Simo.





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



More information about the sssd-devel mailing list