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