On Wed, Apr 29, 2020 at 2:43 PM Ruediger Pluem <[email protected]> wrote: > > > > On 4/29/20 5:02 PM, Eric Covener wrote: > > On Wed, Apr 29, 2020 at 9:29 AM Joe Orton <[email protected]> wrote: > >> > >> In uldap_connection_unbind, apr_ldap_rebind_remove() is always passed > >> NULL since ldc->ldap is either NULL on entry or is set to NULL. This > >> looks safe, but seems like an expensive noop since > >> apr_ldap_rebind_remove() acquires a mutex and iterates a linked list > >> trying to find a rebind reference matching NULL (I assume always > >> failing). > >> > >> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ldap/util_ldap.c?revision=1876599&view=markup#l222 > >> > >> Can this be removed or should it be moved up so it's not a noop? I've > >> not tried to reproduce problems in an LDAP config with referrals. > >> > >> (Credit to Abhishek and Angel who saw apr_ldap_rebind_remove() spinning > >> and debugged this) > >> > >> Index: modules/ldap/util_ldap.c > >> =================================================================== > >> --- modules/ldap/util_ldap.c (revision 1877157) > >> +++ modules/ldap/util_ldap.c (working copy) > >> @@ -225,6 +225,12 @@ > >> > >> if (ldc) { > >> if (ldc->ldap) { > >> + /* forget the rebind info for this conn */ > >> + if (ldc->ChaseReferrals == AP_LDAP_CHASEREFERRALS_ON) { > >> + apr_ldap_rebind_remove(ldc->ldap); > >> + apr_pool_clear(ldc->rebind_pool); > >> + } > >> + > >> if (ldc->r) { > >> ap_log_rerror(APLOG_MARK, APLOG_TRACE5, 0, ldc->r, "LDC > >> %pp unbind", ldc); > >> } > >> @@ -233,11 +239,6 @@ > >> } > >> ldc->bound = 0; > >> > >> - /* forget the rebind info for this conn */ > >> - if (ldc->ChaseReferrals == AP_LDAP_CHASEREFERRALS_ON) { > >> - apr_ldap_rebind_remove(ldc->ldap); > >> - apr_pool_clear(ldc->rebind_pool); > >> - } > >> } > > > > +1 to moving up > > 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?
We need the ldc->LDAP to add the rebind as it is the key, and we do seem to uniformly only clear ldc->LDAP in the unbind method. So I think it 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? This is a good point. For the IBM/Tivoli SDK the callback can be called a 2nd time as a cleanup (in the IBM SDK terms, not APR). This will trigger a full/failing search of the linked list under the mutex. I don't know when the "cleanup" style of callback is actually called by the SDK but it is probably best to avoid that ordering.
