Re: AutoFS segfaults in get_query_dn()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


On Wed, 2012-03-28 at 10:48 -0400, Leonardo Chiquitto wrote:
> On Tue, Mar 27, 2012 at 8:26 PM, Ian Kent <raven@xxxxxxxxxx> wrote:
> > On Thu, 2012-03-15 at 23:15 -0400, Leonardo Chiquitto wrote:
> >> Hello,
> >>
> >> The automounter can crash if two threads execute get_query_dn()
> >> simultaneously. To reproduce the problem you'll need to:
> >>
> >> - Setup AutoFS with an indirect map on LDAP
> >> - Set the LDAP base dn ($SEARCH_BASE) in /etc/sysconfig/autofs
> >> - Trigger many mounts simultaneously (it doesn't matter
if the keys
> >>   exist or not in the map). Something like this will be enough:
> >>
> >>    $ for i in $(seq 1 100); do
> >>        nohup ls test$i &
> >>       done
> >>
> >> Thread "a" crashes when it tries to dereference the "this" pointer:
> >>
> >>  369     } else {
> >>  370         struct ldap_searchdn *this = ctxt->sdns;
> >>  371
> >>  372         debug(logopt, MODPREFIX "check search base list");
> >>  373
> >>  374         result = NULL;
> >>  375         while (this) {
> >>  376             rv = ldap_search_s(ldap, this->basedn,        <= here
> >>  377                        scope, query, attrs, 0, &result);
> >>  378             if ((rv == LDAP_SUCCESS) && result) {
> >>  379                 debug(logopt, MODPREFIX
> >>  380                       "found search base under %s",
> >>  381                       this->basedn);                      <= or here
> >>
> >> Because Thread "b" is running this other section of the same function:
> >>
> >>  333     if (!ctxt->base) {
> >>  334         sdns = defaults_get_searchdns();
> >>  335         if (sdns) {
> >>  336             if (ctxt->sdns)
> >>  337                 defaults_free_searchdns(ctxt->sdns);   <= frees sdns
> >>  338             ctxt->sdns = sdns;
> >>  339         }
> >>  340     }
> >>
> >> So ctxt->sdns is freed but Thread "a" still keeps a copy of the pointer
> >> and tries to use it.
> >>
> >> The patch below resolves the problem, but I don't know if it is correct.
> >> AutoFS rereads $SEARCH_BASE from /etc/sysconfig/autofs every
> >> time get_query_dn() is called. This *looks* unnecessary, unless the
> >> intention is to react to sysconfig changes without restarting the
> >> daemon.
> >
> > Although the get_query_dn() function was originally only called on map
> > read and at startup that has changed and it is called on every connect
> > or reconnect so I agree that we shouldn't reading the search dns every
> > time.
> 
> Thanks for investigating this further. I was going to post the debug logs
> today, but you were faster :-)
> 
> >> Ian, I'd appreciate if you could review the patch.
> >>
> >> Thanks!
> >> Leonardo
> >>
> >> ---
> >>
> >> Fix segfault in get_query_dn()
> >>
> >> Automount will segfault when two threads run get_query_dn()
> >> simultaneously and $SEARCH_BASE is defined in sysconfig.
> >> This happens because a thread tries to dereference ctxt->sdns
> >> while another thread running the same function frees the
> >> memory.
> >>
> >> I believe we don't need to reread $SEARCH_BASE every time
> >> get_query_dn() is called.
> >>
> >> diff --git a/modules/lookup_ldap.c b/modules/lookup_ldap.c
> >> index b6875fe..6ed3e17 100644
> >> --- a/modules/lookup_ldap.c
> >> +++ b/modules/lookup_ldap.c
> >> @@ -281,7 +281,6 @@ static int get_query_dn(unsigned logopt, LDAP
> >> *ldap, struct lookup_context *ctxt
> >>       char buf[MAX_ERR_BUF];
> >>       char *query, *dn, *qdn;
> >>       LDAPMessage *result = NULL, *e;
> >> -     struct ldap_searchdn *sdns = NULL;
> >>       char *attrs[2];
> >>       struct berval **value;
> >>       int scope;
> >> @@ -330,14 +329,8 @@ static int get_query_dn(unsigned logopt, LDAP
> >> *ldap, struct lookup_context *ctxt
> >>               scope = LDAP_SCOPE_SUBTREE;
> >>       }
> >>
> >> -     if (!ctxt->base) {
> >> -             sdns = defaults_get_searchdns();
> >> -             if (sdns) {
> >> -                     if (ctxt->sdns)
> >> -                             defaults_free_searchdns(ctxt->sdns);
> >> -                     ctxt->sdns = sdns;
> >> -             }
> >> -     }
> >> +     if (!ctxt->base && !ctxt->sdns)
> >> +             ctxt->sdns = defaults_get_searchdns();
> >
> > While this narrows the race window and should prevent the fault, it
> > still has a problem since ctxt->sdns can change while it is in use. The
> > above two lines are a non-atomic read then update which can (at the
> > least) lead to a memory leak when the if (...) is evaluated while
> > another thread is executing defaults_get_searchdns().
> >
> > So a mutex is still needed around these two lines.
> 
> You're completely right, of course. Thanks for catching this.

I'll add the mutex and commit the change.
Thanks for your efforts on this.

> 
> Leonardo


--
To unsubscribe from this list: send the line "unsubscribe autofs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Tools]     [DDR & Rambus]     [Asterisk Internet PBX]     [Linux API]     [Monitors]

Add to Google