You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2020/05/07 06:04:03 UTC
Re: [PATCH] mod_ldap: fix apr_ldap_rebind_remove() use
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 <rp...@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?
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 :-)
Regards
Rüdiger
Re: [PATCH] mod_ldap: fix apr_ldap_rebind_remove() use
Posted by Eric Covener <co...@gmail.com>.
On Tue, Jun 16, 2020 at 10:40 AM Joe Orton <jo...@redhat.com> wrote:
>
> On Thu, May 07, 2020 at 09:50:26AM -0400, Eric Covener wrote:
> > On Thu, May 7, 2020 at 9:39 AM Joe Orton <jo...@redhat.com> wrote:
> > > Better question... or stupider question? For "modern" OpenLDAP where
> > > ldap_set_rebind_proc takes a void *, this linked list cache is
> > > completely redundant and you can "simply" pass the (bindDN, bindPW)
> > > through to the rebind callback via the void *, and that will work
> > > correctly?
> >
> > That makes sense, I would think so. The manual on my Mac has the
> > userdata parm too.
>
> I have merged that change in r1878890 - thanks a lot for reviewing &
> walking me through this far, apologies for moving slowing but I wanted
> to get a working test case before proceeding.
>
> Reviewing this thread again I am not sure if the change to
> remove/reorder the removal of the rebind is safe for non-OpenLDAP
> toolkits. I can revert that bit if required?
>
> https://github.com/apache/httpd/commit/130eac3ae6e8f7a8465c7792660ce8b747642b23#diff-f0cf47f5d582509d36e95f170231bc21L226
I think it should be left in. I found I had made a very similar
change in the distribution I look after while adding Darwin sandbox
support.
Re: [PATCH] mod_ldap: fix apr_ldap_rebind_remove() use
Posted by Joe Orton <jo...@redhat.com>.
On Thu, May 07, 2020 at 09:50:26AM -0400, Eric Covener wrote:
> On Thu, May 7, 2020 at 9:39 AM Joe Orton <jo...@redhat.com> wrote:
> > Better question... or stupider question? For "modern" OpenLDAP where
> > ldap_set_rebind_proc takes a void *, this linked list cache is
> > completely redundant and you can "simply" pass the (bindDN, bindPW)
> > through to the rebind callback via the void *, and that will work
> > correctly?
>
> That makes sense, I would think so. The manual on my Mac has the
> userdata parm too.
I have merged that change in r1878890 - thanks a lot for reviewing &
walking me through this far, apologies for moving slowing but I wanted
to get a working test case before proceeding.
Reviewing this thread again I am not sure if the change to
remove/reorder the removal of the rebind is safe for non-OpenLDAP
toolkits. I can revert that bit if required?
https://github.com/apache/httpd/commit/130eac3ae6e8f7a8465c7792660ce8b747642b23#diff-f0cf47f5d582509d36e95f170231bc21L226
Regards, Joe
Re: [PATCH] mod_ldap: fix apr_ldap_rebind_remove() use
Posted by Eric Covener <co...@gmail.com>.
On Thu, May 7, 2020 at 9:39 AM Joe Orton <jo...@redhat.com> wrote:
>
> On Thu, May 07, 2020 at 08:06:00AM -0400, Eric Covener wrote:
> > On Thu, May 7, 2020 at 2:04 AM Ruediger Pluem <rp...@apache.org> wrote:
> > > 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?
>
> I don't see why not.
>
> Better question... or stupider question? For "modern" OpenLDAP where
> ldap_set_rebind_proc takes a void *, this linked list cache is
> completely redundant and you can "simply" pass the (bindDN, bindPW)
> through to the rebind callback via the void *, and that will work
> correctly?
That makes sense, I would think so. The manual on my Mac has the
userdata parm too.
--
Eric Covener
covener@gmail.com
Re: [PATCH] mod_ldap: fix apr_ldap_rebind_remove() use
Posted by Joe Orton <jo...@redhat.com>.
On Thu, May 07, 2020 at 08:06:00AM -0400, Eric Covener wrote:
> On Thu, May 7, 2020 at 2:04 AM Ruediger Pluem <rp...@apache.org> wrote:
> > 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?
I don't see why not.
Better question... or stupider question? For "modern" OpenLDAP where
ldap_set_rebind_proc takes a void *, this linked list cache is
completely redundant and you can "simply" pass the (bindDN, bindPW)
through to the rebind callback via the void *, and that will work
correctly?
Re: [PATCH] mod_ldap: fix apr_ldap_rebind_remove() use
Posted by Eric Covener <co...@gmail.com>.
> 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.
Above is wrong since there is really no leak currently as Joe pointed
out, the apr_pool_clear is going to remove the entry with its stashed
away copy of the pointer.
Re: [PATCH] mod_ldap: fix apr_ldap_rebind_remove() use
Posted by Eric Covener <co...@gmail.com>.
On Thu, May 7, 2020 at 2:04 AM Ruediger Pluem <rp...@apache.org> 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 <rp...@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?!)
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?