On Tue, 13 Apr 2010 19:38:27 +0400
Eugene Indenbom <eindenbom(a)gmail.com> wrote:
Hi Simo,
On 04/13/2010 06:44 PM, Simo Sorce wrote:
[..]
> Well sdap_release_connection does not exist in current master
:-)
>
I am sorry for a typo mistake. The function I am talking about is
sdap_handle_release.
Ah ok, this make much more sense and is what we found as well :)
> 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()
>
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 sdap_check_gssapi_reconnect.
Yes I am aware of this problem.
> 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()
>
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 am making the rule that only _send() functions can call it for now.
Later on we can change how things work, but we need a minimal patch we
can apply also to released branches that do not introduce too many
changes.
> 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.
>
>
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 bare disconnect.
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.
Yes, in fact I discussed this on IRC already with other developers and
we decided to simply change talloc_zfree(ctx->gsh) with
ctx->gsh->connectd = false; and let the first _send() function free it
and reconnect.
This means sdap_handle_release() remains unchanged, but also that it
will not recurse as we never free gsh within a callback.
>> 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.
>
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
talloc_zfree(sdap_ctx->gsh).
Yes, we will definitely test it :)
> 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.
>
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.
Don't worry, as I said, they are just minor nitpicks.
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.
I think you forgot to set your email in ~/.gitconfig, the author of
the patches in the From is: eindenbom <eindenbom(a)indenbom-f12.abbyy.ru>
See the second line of the patches you've sent.
> 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.
>
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 logical layers.
Yeah, the intent is clear, I need to think if we can easily have a
common layer that abstract this stuff or if repeating causes less
issues than abstracting.
Simo.
--
Simo Sorce * Red Hat, Inc * New York