[SSSD] Patch to fix LDAP ID backend GSSAPI credential expired messages

Simo Sorce ssorce at redhat.com
Tue Apr 13 16:43:05 UTC 2010


On Tue, 13 Apr 2010 19:38:27 +0400
Eugene Indenbom <eindenbom at 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 at 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 at 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



More information about the sssd-devel mailing list