On Thu, May 7, 2020 at 2:04 AM Ruediger Pluem <[email protected]> wrote: > > > > On 4/30/20 11:34 AM, Joe Orton wrote: > > On Wed, Apr 29, 2020 at 03:09:28PM -0400, Eric Covener wrote: > >> On Wed, Apr 29, 2020 at 2:43 PM Ruediger Pluem <[email protected]> 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?!)
I think it is right, the LDAP* is the only userdata we portably get on the callback from the SDK and we have to return credentials. There are also pooled LDAP* sitting idle who have entries floating around. Probably not too many extras unless multiple LDAP servers are used. > > 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? My hope is that the httpd-only fix will eliminate the leaks and the locking will not be too much relative to ldap costs, especially w/ the caching in mod_ldap. > Not looked at the problem further, but there is now a PR for this: > > https://bz.apache.org/bugzilla/show_bug.cgi?id=64414 > > Maybe this revives the discussion :-) Nothing stood out in the source, is it OK to hash a pointer just by passing APR_SIZEOF_VOIDP as the length?
