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