Re: [PATCH] mod_ldap: fix apr_ldap_rebind_remove() use

[Eric Covener]

On Wed, Apr 29, 2020 at 2:43 PM Ruediger Pluem  wrote:
>
>
>
> On 4/29/20 5:02 PM, Eric Covener wrote:
> > On Wed, Apr 29, 2020 at 9:29 AM Joe Orton  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?
> 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.

Re: [PATCH] mod_ldap: fix apr_ldap_rebind_remove() use

[Ruediger Pluem]

On 4/29/20 5:02 PM, Eric Covener wrote:
> On Wed, Apr 29, 2020 at 9:29 AM Joe Orton  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?
And can we still do an ldap_unbind_s(ldc->ldap); if we did an 
apr_ldap_rebind_remove(ldc->ldap); before?

Regards

Rüdiger

> 

Re: Can github activity (new PRs, comments) be forwarded to dev@ ?

[Yann Ylavic]

On Wed, Apr 29, 2020 at 6:47 PM Daniel Gruno  wrote:
>
> On 29/04/2020 10.52, Daniel Gruno wrote:
> > On 29/04/2020 10.22, Daniel Gruno wrote:
> >>
> >>>
> >>> I recommend using selfserve.apache.org 
> >>> to create notifications@httpd (the typical name/pattern for this
> >>> stuff), then to use .asf.yaml to sort out the various emails.
> >>
> >> Once we are in agreement on a name here, I can get this processed with
> >> my VP hat on, and task infra to figure out how to relay notifications
> >> there :)
> >
> > Going to JFDI here... notifications@httpd coming right up :p
>
> And done, plus we have GH notifications going there now :)

Awesome, thanks Daniel!

Re: Can github activity (new PRs, comments) be forwarded to dev@ ?

[Daniel Gruno]

On 29/04/2020 10.52, Daniel Gruno wrote:

On 29/04/2020 10.22, Daniel Gruno wrote:




I recommend using selfserve.apache.org  
to create notifications@httpd (the typical name/pattern for this 
stuff), then to use .asf.yaml to sort out the various emails.


Once we are in agreement on a name here, I can get this processed with 
my VP hat on, and task infra to figure out how to relay notifications 
there :)


Going to JFDI here... notifications@httpd coming right up :p


And done, plus we have GH notifications going there now :)







Cheers,
-g

Re: Can github activity (new PRs, comments) be forwarded to dev@ ?

[Daniel Gruno]

On 29/04/2020 10.22, Daniel Gruno wrote:




I recommend using selfserve.apache.org  
to create notifications@httpd (the typical name/pattern for this 
stuff), then to use .asf.yaml to sort out the various emails.


Once we are in agreement on a name here, I can get this processed with 
my VP hat on, and task infra to figure out how to relay notifications 
there :)


Going to JFDI here... notifications@httpd coming right up :p





Cheers,
-g

Re: Can github activity (new PRs, comments) be forwarded to dev@ ?

[Daniel Gruno]

On 29/04/2020 10.11, Greg Stein wrote:
On Wed, Apr 29, 2020 at 8:19 AM Joe Orton > wrote:


On Tue, Apr 28, 2020 at 05:19:09PM +0200, Ruediger Pluem wrote:
 > +1 g...@httpd.apache.org  (I declare
the naming discussion for the list name opened :-)).
 > This offers an easy opt-in for those who are interested in these
updates.

+1 from me, does anybody know if it's actually technically possible to
do though?  AFAICT there is not github project setting to do this kind
of thing specifically so we'd either need to use the API somehow?  Or
add whate...@httpd.apache.org  to
a personal github account and have
notifications through that, which seems ugly.


Solved.
https://cwiki.apache.org/confluence/display/INFRA/.asf.yaml+features+for+git+repositories#id-.asf.yamlfeaturesforgitrepositories-Notificationsettingsforrepositories 



This unfortunately won't work since httpd.git is a mirror repository.
I'm sure we can solve the PR notifications one way or the other, but it 
can't be done through .asf.yaml until such a point where httpd is using 
git as its main VCS.




I recommend using selfserve.apache.org  to 
create notifications@httpd (the typical name/pattern for this stuff), 
then to use .asf.yaml to sort out the various emails.


Once we are in agreement on a name here, I can get this processed with 
my VP hat on, and task infra to figure out how to relay notifications 
there :)




Cheers,
-g

Re: Can github activity (new PRs, comments) be forwarded to dev@ ?

[Greg Stein]

On Wed, Apr 29, 2020 at 8:19 AM Joe Orton  wrote:

> On Tue, Apr 28, 2020 at 05:19:09PM +0200, Ruediger Pluem wrote:
> > +1 g...@httpd.apache.org (I declare the naming discussion for the list
> name opened :-)).
> > This offers an easy opt-in for those who are interested in these updates.
>
> +1 from me, does anybody know if it's actually technically possible to
> do though?  AFAICT there is not github project setting to do this kind
> of thing specifically so we'd either need to use the API somehow?  Or
> add whate...@httpd.apache.org to a personal github account and have
> notifications through that, which seems ugly.
>

Solved.
https://cwiki.apache.org/confluence/display/INFRA/.asf.yaml+features+for+git+repositories#id-.asf.yamlfeaturesforgitrepositories-Notificationsettingsforrepositories

I recommend using selfserve.apache.org to create notifications@httpd (the
typical name/pattern for this stuff), then to use .asf.yaml to sort out the
various emails.

Cheers,
-g

Re: [PATCH] mod_ldap: fix apr_ldap_rebind_remove() use

[Eric Covener]

On Wed, Apr 29, 2020 at 9:29 AM Joe Orton  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

[PATCH] mod_ldap: fix apr_ldap_rebind_remove() use

[Joe Orton]

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);
-}
 }
 
 return APR_SUCCESS;

Re: Can github activity (new PRs, comments) be forwarded to dev@ ?

[Joe Orton]

On Tue, Apr 28, 2020 at 05:19:09PM +0200, Ruediger Pluem wrote:
> +1 g...@httpd.apache.org (I declare the naming discussion for the list name 
> opened :-)).
> This offers an easy opt-in for those who are interested in these updates.

+1 from me, does anybody know if it's actually technically possible to 
do though?  AFAICT there is not github project setting to do this kind 
of thing specifically so we'd either need to use the API somehow?  Or 
add whate...@httpd.apache.org to a personal github account and have 
notifications through that, which seems ugly.

Regards, Joe