[Freeipa-devel] [PATCH] Fix race condition leading to segfaults

[Sumit Bose]

On Fri, Jul 24, 2009 at 10:15:25AM +0200, Sumit Bose wrote:
> On Thu, Jul 23, 2009 at 04:18:15PM -0400, Simo Sorce wrote:
> > 
> > Sumit found out that ldap auth would segfault from time to time.
> > 
> > The problem was the way ldap_result() works you don't know how many
> > results are in the pipe so you have to keep polling, once the fde fired,
> > until you get back a 0.
> > 
> > The problem although, was that once a result was received the calling
> > function, upon getting called by op->callback(), could decide to free
> > the sdap_handle and all dependent memory like it happens in
> > sdap_pam_auth_done() which is called through a chain of callbacks from
> > sdap_process_message()
> > 
> > The following patch addresses the problem using, temporarily a timer
> > event to schedule any other call only after the callback is finished.
> > The events are appended on the sdap_handle, so that if the function
> > being called actually frees it, all pending events are also freed
> > without risk.
> 
> No more segfaults (so far :-)
> 
> But I think the following piece of code might be an example where gcc
> might optimize away a check, because the pointers to check
> (sh->ldap) are already referenced in the debug
> statement. The glibc printf which is used by DEBUG can handle NULL
> pointers and will not segfault here and if the check is optimized away
> ldap_result won't like sh->ldap being NULL.
> 
> > +
> > +    DEBUG(8, ("Trace: sh[%p], connected[%d], ops[%p], fde[%p], ldap[%p]\n",
> > +              sh, (int)sh->connected, sh->ops, sh->fde, sh->ldap));
> > +
> > +    if (!sh->connected || !sh->ldap) {
> > +        DEBUG(2, ("ERROR: LDAP connection is not connected!\n"));
> > +        return;
> > +    }
> > +
> > +    ret = ldap_result(sh->ldap, LDAP_RES_ANY, 0, &no_timeout, &msg);
> 
> 

After a discussion on irc Simo and I came to the conclusion that the
conditional code in DEBUG would prevent gcc from optimizing away the
check.

ACK

bye,
Sumit