On 04/13/2010 06:44 PM, Simo Sorce wrote:
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
>>> 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
>> 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
>> 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 :-)
I am sorry for a typo mistake. The function I am talking about is
However thanks to this explanation I think I see where the problem
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() ->
talloc_zfree(sdap_ctx->gsh) is called in many other places as well. All
reconnect code relies on timely destruction of singleton connection
(sdap_ctx->gsh). For example take a look at code right after calls to
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
You can not really rely on the fact that no code calls
talloc_zfree(sdap_ctx->gsh). This is potential source of future errors.
Somebody will call it and we will never find out until crash report comes.
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
This would not help. I have evaluated this solution. The problem is:
1. The top level call to sdap_handle_release is not a destructor. It's
2. The memory is freed after inner call to sdap_handle_destructor
completes. You can not change this.
3. When sdap_handle_release iterates over ops and calls callbacks on
disconnect, it has to have an access to sdap_handle->ops member
__after__ callback returns. This means that memory sdap_handle->ops
occupies must survive destructor. That's exactly what my patch does.
> Actually, I have already sent a patch fixing the issue (please,
> 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.
Please, before sending your patch, repeat the test scenario I have
described above and you will still have a crash. Unless you really
manage to kill in your patch all occurrences of
1 and 2 looks reasonable although there are 2 minor nitpicks.
Your email address is probably wrong. And you add trailing spaces in
I am sorry for trailing spaces. It has taken some time to set up
development environment. I am starting a new Linux project after more
than 3 years without Linux. So it takes time to get all tools at hand.
I'll find all such formatting errors and correct them.
In what places the e-mail is wrong? It is eindenbom(a)gmail.com. I have
sent a test e-mail to myself and appears to work. Sorry about that too.
On the 4th I'll try to do a bit more analysis later, but it looks
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.
OK. One more argument, sdap_id_op is needed for:
1. To keep track of current connection.
2. To reference count the connection (otherwise it would be
indeterminable when to destroy connection).
3. To keep track of number of retries.
This does not amount to much work and responsibility, but nevertheless
it does not make sense to repeat retry tracking and other staff in upper