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

Simo Sorce ssorce at redhat.com
Tue Apr 13 14:44:19 UTC 2010


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



More information about the sssd-devel mailing list