On Tue, 13 Apr 2010 09:28:28 +0400
Eugene Indenbom <eindenbom(a)gmail.com> wrote:
Hi Stephen and Simo,
On 04/13/2010 12:23 AM, Stephen Gallagher wrote:
> On 04/07/2010 10:10 AM, Eugene Indenbom wrote:
>
>> --- 0003-Fixed-recursive-sdap_handle-disconnect-sequence-from.patch
>>
>> This patch fixes a critical bug of accessing freed memory in
>> sdap_handle_release. The problem can be reproduced as follows:
>> 1. Break LDAP connection when LDAP query is in progress (e.g. by
>> restarting directory server)
>> 2. sdap_process_result detects communication error and calls
>> sdap_handle_release
>> 3. sdap_handle_release calls active sdap_op callback
>> 4. callback deletes sdap_handle
>> 5. sdap_release_handle segfaults or asserts on access to
>> deallocated memory
>>
>>
> I am unable to find any code path such that step 4 (callback deletes
> sdap_handle) is possible. Every callback that we ever add with
> sdap_op_add() behaves the same way when called from
> sdap_handle_release().
>
> Since sdap_handle_release() invokes the callbacks with the error
> code of EIO, they all immediately set tevent_req_error() and
> return. There is no path in which they delete any memory.
>
> Furthermore, all sdap_op_add() calls use a memory heirarchy that is
> either below or completely unrelated to the sdap_handle hierarchy.
> So calling talloc_free() on the sdap_op object cannot free the
> sdap_handle.
>
> Eugene, if you are sure that this exists in the current master, can
> you point out the relevant problem area?
>
Yes, I am sure. The callback deleting the sdap_handle structure is
not direct callback to sdap_op, but chain callback on associated
tevent_req. The complete list of offending callbacks is:
- sdap_account_info_users_done
- sdap_account_info_groups_done
- sdap_account_info_initgr_done
- ldap_id_enum_users_done
- ldap_id_enum_groups_done
The crash itself happens in sdap_release_connection.
Well sdap_release_connection does not exist in current master :-)
However thanks to this explanation I think I see where the problem lies.
In these callbacks we call sdap_mark_offline() on some errors, and that
frees ctx->gsh which causes us this nice loop:
sdap_process_result() -> sdap_handle_release() -> callback ->
sdap_mark_offline() -> talloc_zfree() -> sdap_handle_destructor() ->
sdap_handle_release()
And so on depending on how many ops we have.
The solution though is much simpler than the patch you propose, I will
remove the free from sdap_mark_offline() and replace it with marking
sdap_handle as to be released, then free it at the end of
sdap_handle_release()
I will also move the current sdap_handle_release() code in
sdap_handle_release_internal() so that it can be called from the
destructor without us attempting to free sh within the destructor
itself.
Actually, I have already sent a patch fixing the issue (please, take
a look at it) and it is explained in the fix and accompanying mail
how does crash happens and how it has been fixed.
Yes I looked at the patch but it didn't make things any clearer, and
introduced changes that were a bit more invasive than needed IMO.
Steps to reproduce are extremely simple:
1. Using debugger break in sdap_generic_send
2. Restart LDAP server or network connection leading to it
3. Resume execution
4. Observe crash.
I will send an alternative patch shortly.
Then will resume my review of your patches.
1 and 2 looks reasonable although there are 2 minor nitpicks.
Your email address is probably wrong. And you add trailing spaces in
some lines.
On the 4th I'll try to do a bit more analysis later, but it looks a bit
too complicated and out of style. In particular I don't like much the
sdap_id_op idea and the associated _create() function. I will try to
give some more constructive feedback on it later.
Simo.
--
Simo Sorce * Red Hat, Inc * New York