Re: AutoFS segfaults in get_query_dn()

[Ian Kent <raven@xxxxxxxxxx>]

On Fri, 2012-03-23 at 10:50 -0400, Leonardo Chiquitto wrote:
> On Fri, Mar 23, 2012 at 6:48 AM, Ian Kent <raven@xxxxxxxxxx> wrote:
> > On Fri, 2012-03-23 at 11:23 +0800, Ian Kent 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.
> >> >
> >> > Ian, I'd appreciate if you could review the patch.
> >>
> >> I'd like to keep the config file update.
> >>
> >> This should also fix the problem, can you give it a try please.
> >
> > The previous patch is definitely not quite right.
> > Try this one.
> 
> Hi Ian,
> 
> Thanks for the patch. I verified that it resolves the problem too, however,
> do you think the benefit is worth the additional complexity and performance
> penalty?
> 
> Actually, I believe that re-reading /etc/sysconfig/autofs dynamically might
> come as a surprise to some system administrators. I, for example, always
> expect that a restart will be required for my changes to sysconfig files to
> take effect.

That is the place where the configuration is updated on receiving a HUP
signal. I'll re-consider your patch but ....

The lookup module should be opened once for each map and closed and
opened again on map re-read, whether that is due to autofs thinking the
map has changed or due to a HUP signal.

Map key lookups should not cause get_query_dn() to be called so this
should not be a problem. If that's not what your seeing then we need to
start looking at a debug log.

Ian

--
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