On Tue, Mar 27, 2012 at 8:48 PM, Ian Kent <raven@xxxxxxxxxx> wrote:
> On Wed, 2012-03-28 at 08:26 +0800, Ian Kent wrote:
>> On Thu, 2012-03-15 at 23:15 -0400, Leonardo Chiquitto wrote:
>> >
>> > 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.
>>
>
> An even better approach would be to move the read of the search dns to
> the lookup_init() module function. That should still do what is needed
> and reduce the code executed at lookup a little.
>
> autofs-5.0.6 - fix segfault in get_query_dn()
>
> From: Leonardo Chiquitto <leonardo.lists@xxxxxxxxx>
>
> 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.
>
> edit: Ian Kent <raven@xxxxxxxxxx>
> move the read of configured search dns to lookup_init().
> edit end
> ---
Thanks for the updated patch, Ian. I verified that it also resolves
the problem (I know it's obvious but, as the test case is trivial,
I wanted to verify).
Leonardo
> modules/lookup_ldap.c | 13 +++----------
> 1 files changed, 3 insertions(+), 10 deletions(-)
>
>
> diff --git a/modules/lookup_ldap.c b/modules/lookup_ldap.c
> index b6875fe..8d12920 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,15 +329,6 @@ 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;
> - }
> - }
> -
> dn = NULL;
> if (!ctxt->sdns) {
> rv = ldap_search_s(ldap, ctxt->base,
> @@ -1467,6 +1457,9 @@ int lookup_init(const char *mapfmt, int argc, const char *const *argv, void **co
> return 1;
> }
>
> + if (!ctxt->base)
> + ctxt->sdns = defaults_get_searchdns();
> +
> ctxt->timeout = defaults_get_ldap_timeout();
> ctxt->network_timeout = defaults_get_ldap_network_timeout();
>
--
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
