[SSSD] [PATCH] fix early free of sdap_handle

Eugene Indenbom eindenbom at gmail.com
Wed Apr 14 09:17:52 UTC 2010


On 04/14/2010 11:53 AM, Eugene Indenbom wrote:
> On 04/14/2010 11:13 AM, Sumit Bose wrote:
>> On Wed, Apr 14, 2010 at 10:48:28AM +0400, Eugene Indenbom wrote:
>> ...
>>> 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.
>>>
>> ...
>>
>> This is not necessary, because sdap_ldap_connect_callback_del() checks
>> if lc_arg is NULL.
> That's right, but I remember that during debugging of my patch I have 
> indeed run across a crash related to this early free.
>
> Probably it was not in sdap_ldap_connect_callback_del, but rather in 
> sdap_ldap_connect_callback_add. Although I can not right now figure 
> out how to reproduce it.
>
> It's anyway safer to remove callback first and then free the callback 
> data.
>
I have found out how it has happened:

1. sdap_handle_release sets sdap_handle->connected = false near the end 
of the method, after callback are called.
2. So in recursion scenario (without Simo's patch and with partial 
changes from my patch #4), the connection was considered good in 
callbacks and reused.
3. As result new file descriptor was opened by LDAP and 
sdap_ldap_connect_callback_add was called. So here we have a crash.

So yet another important fix in my patch is that it sets 
sdap_handle->connected to false as soon as possible, thus preventing 
connection reuse.

Regards, Eugene



More information about the sssd-devel mailing list