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
