On Wed, Apr 29, 2020 at 03:09:28PM -0400, Eric Covener wrote: > On Wed, Apr 29, 2020 at 2:43 PM Ruediger Pluem <rpl...@apache.org> wrote: > > Hm. Don't we need to do the apr_pool_clear(ldc->rebind_pool); in any case > > if ldc->ChaseReferrals == AP_LDAP_CHASEREFERRALS_ON or > > does this only makes sense if ldc->ldap != NULL? > > And can we still do an ldap_unbind_s(ldc->ldap); if we did an > > apr_ldap_rebind_remove(ldc->ldap); before? > > I see what you mean. If we lost the ldc->ldap some other way, anything > allocated from ldc->rebind_pool is leaked because the LDAP key can't > be looked up in the xref linked list. > We would likely have linked list growth in that case too.
Shouldn't clearing the pool here be sufficient anyway? Since there is a cleanup registered by apr_ldap_rebind_add() which calls apr_ldap_rebind_remove() with the original LDAP * pointer value it looks safe, and it avoids entering _rebind_remove twice. Doing it before the ldap_unbind() call so that LDAP * pointer is not a dangling pointer seems better. The underlying problem here is a performance issue which looked like linked list growth so actually with: a) threaded MPM and large number of threads, b) each thread may have its own LDAP * and an associated rebind entry (is that really right?!) c) on unbind each thread walks the linked list w/mutex held which looks quite painful regardless, and doing (c) twice is clearly worse. So as well as changing this the indexed linked list should really be a hash table? Regards, Joe -- Joe Orton // Red Hat Core Services
