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.
>
> 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.
>
> dn = NULL;
> if (!ctxt->sdns) {
> --
> 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
--
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
