[SSSD] [PATCH] fix early free of sdap_handle

Simo Sorce ssorce at redhat.com
Wed Apr 14 13:00:25 UTC 2010


On Wed, 14 Apr 2010 10:48:28 +0400
Eugene Indenbom <eindenbom at gmail.com> wrote:

> Hi Simo,
> 
> On 04/13/2010 08:51 PM, Simo Sorce wrote:
> > This patch attempts to address the problem Eugene found with
> > sdap_handle_release()
> >
> >    
> I have checked the suggested patch and found several problems:
> 
> 1. providers/ipa/ipa_access.c still lacks failover retries

As I said this patch only fixes the bug without changing any behavior.
I want to see the reconnection stuff on top of it later.

> 2. Why number of retries is 2? It is only enough for one server. If
> we have failover set up we need more retries (at least 1 per server +
> 1 for stale cached connection)

That's an arbirtary number, I came up just to avoid having it loop
forever, it is by no means intended to replace your 4th patch. Just a
temporary stopgap to be able to push this into stable earlier, with the
understanding we will push a better patch set in master once we agree
on its form.

> 3. My patch contained an additional fix to sdap_handle_release.
> Before calling op callbacks LDAP callback data was destroyed:
> 
> commit: sssd-1_1_1
> line 102:        talloc_zfree(sh->conncb->lc_arg);
> 
> and later LDAP handle was unbound:
> line 116:        ldap_unbind_ext(sh->ldap, NULL, NULL);
> 
> But during unbind the LDAP callbacks are called. And sssd callback 
> (sdap_ldap_connect_callback_del) accesses zero pointer (lc_arg).
> 
> So before freeing sh->conncb->lc_arg it is necessary to call:
>                  /* remove callback first */
>                  ldap_get_option(sh->ldap, LDAP_OPT_CONNECT_CB,
> sh->conncb);
> 
> Actually I wonder how does it work for you without this fix? Probably 
> you have never tried to close a connection with still open file
> descriptor.

As you found out later, my patch prevents this from happening because
it marks the connection as faulty.

> 4. The suggested patch is a workaround. It relies on the agreement
> that all _send functions will be called from top level event context,
> not from subreq callback context. This is hard to guarantee.

It is easier to analyze and agree on for now.
It solves the problem, without changing the underlying logic.
This is a big plus for a patch to go into a stable tree as it will not
cause surprises.

> I have attached to this e-mail a version of my patch addressing the
> same problem that applies directly on top of sssd 1.1.1.

Se my comments on your approach, I really don't like it.
We try very hard to avoid using references and allocations on NULL, as
they often just end up being memory leaks and cause other issues when
freeing memory hierarchies as they are not connected in.

> I think its better to use my solution as:
> 
> 1. It provides a real fix for the problem rather than workaround

Both patches provide a real fix to the segfault you saw.

> 2. The complexity of patches is alike (5 files, 213 versus 138
> changed lines)

No it is not, your patches changes fundamentally how sdap_handle
behaves, this has a higher inpact than you realize. 

> 3. The real changes in my patch are localized to sdap.h, 
> sdap_async_private.h, sdap_handle_create, sdap_handle_release

See comments about that vs the code.

> 4. Other changes are cosmetic and related to changes in structure 
> definition and therefore can be easily checked by peer review

A seprate patch with cosmetic changes is always in order, but don't
rush on it, so far I am not convinced your patch is ok,
sorry.

Simo.

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



More information about the sssd-devel mailing list