Re: [PATCH] fix util_ldap with older OpenLDAPs
On Wed, Jul 27, 2005 at 09:59:18PM +0100, Joe Orton wrote:
Since 2.0.54, it seems mod_auth_ldap just segfaults on any request if
built against older versions of OpenLDAP, 2.2.20 and earlier (pre-2005).
I worked this out a little better. It triggers only the *second* time
the LDAP connection is opened for a given process. I think I must have
started testing the 2.0.x code with slapd stopped and saw this being
triggered by the retry-10-times logic for every request.
Brad, can you explain why the ldap_set_option() call is used to change
the *process-global* connection timeout setting in the 2.0.x code,
rather than the connection-specific setting like the trunk code does?
Doing that seems generally undesirable as well as triggering the
OpenLDAP bug. Is it because some SDKs don't handle per-connection
settings, perhaps?
If so, this would be a a simpler, better fix for the issue:
Index: modules/experimental/util_ldap.c
===
--- modules/experimental/util_ldap.c(revision 227189)
+++ modules/experimental/util_ldap.c(working copy)
@@ -325,7 +325,11 @@
}
if (st-connectionTimeout = 0) {
+#if APR_HAS_OPENLDAP_LDAPSDK
+rc = ldap_set_option(ldc-ldap, LDAP_OPT_NETWORK_TIMEOUT, (void
*)timeOut);
+#else
rc = ldap_set_option(NULL, LDAP_OPT_NETWORK_TIMEOUT, (void
*)timeOut);
+#endif
if (APR_SUCCESS != rc) {
ap_log_error(APLOG_MARK, APLOG_ERR, 0, r-server,
LDAP: Could not set the connection timeout
);
Re: [PATCH] fix util_ldap with older OpenLDAPs
This is why I would like to see the release of 2.2 as soon as
possible. My only explanation is that I missed changing the NULL
parameter to ldc-ldap when I did the backport of the conversion from
global to per-connection from trunk (rev. 170805). Since the code bases
for util_ldap are significantly different between trunk and 2.0,
backporting this kind of functionality is closer to a rewrite than a
backport. In this case, I just missed it. The correct code should be:
Index: util_ldap.c
===
--- util_ldap.c (revision 226877)
+++ util_ldap.c (working copy)
@@ -325,7 +325,7 @@
}
if (st-connectionTimeout = 0) {
-rc = ldap_set_option(NULL, LDAP_OPT_NETWORK_TIMEOUT, (void
*)timeOut);
+rc = ldap_set_option(ldc-ldap, LDAP_OPT_NETWORK_TIMEOUT,
(void *)timeOut);
if (APR_SUCCESS != rc) {
ap_log_error(APLOG_MARK, APLOG_ERR, 0, r-server,
LDAP: Could not set the connection
timeout );
Brad
[EMAIL PROTECTED] Wednesday, August 03, 2005 4:48:46 AM
On Wed, Jul 27, 2005 at 09:59:18PM +0100, Joe Orton wrote:
Since 2.0.54, it seems mod_auth_ldap just segfaults on any request if
built against older versions of OpenLDAP, 2.2.20 and earlier
(pre-2005).
I worked this out a little better. It triggers only the *second* time
the LDAP connection is opened for a given process. I think I must have
started testing the 2.0.x code with slapd stopped and saw this being
triggered by the retry-10-times logic for every request.
Brad, can you explain why the ldap_set_option() call is used to change
the *process-global* connection timeout setting in the 2.0.x code,
rather than the connection-specific setting like the trunk code does?
Doing that seems generally undesirable as well as triggering the
OpenLDAP bug. Is it because some SDKs don't handle per-connection
settings, perhaps?
If so, this would be a a simpler, better fix for the issue:
Index: modules/experimental/util_ldap.c
===
--- modules/experimental/util_ldap.c(revision 227189)
+++ modules/experimental/util_ldap.c(working copy)
@@ -325,7 +325,11 @@
}
if (st-connectionTimeout = 0) {
+#if APR_HAS_OPENLDAP_LDAPSDK
+rc = ldap_set_option(ldc-ldap, LDAP_OPT_NETWORK_TIMEOUT,
(void *)timeOut);
+#else
rc = ldap_set_option(NULL, LDAP_OPT_NETWORK_TIMEOUT, (void
*)timeOut);
+#endif
if (APR_SUCCESS != rc) {
ap_log_error(APLOG_MARK, APLOG_ERR, 0, r-server,
LDAP: Could not set the connection
timeout );
Re: [PATCH] fix util_ldap with older OpenLDAPs
At 12:24 PM 8/3/2005, Brad Nicholes wrote:
The correct code should be:
Index: util_ldap.c
===
--- util_ldap.c (revision 226877)
+++ util_ldap.c (working copy)
@@ -325,7 +325,7 @@
}
if (st-connectionTimeout = 0) {
-rc = ldap_set_option(NULL, LDAP_OPT_NETWORK_TIMEOUT, (void
*)timeOut);
+rc = ldap_set_option(ldc-ldap, LDAP_OPT_NETWORK_TIMEOUT,
(void *)timeOut);
if (APR_SUCCESS != rc) {
ap_log_error(APLOG_MARK, APLOG_ERR, 0, r-server,
LDAP: Could not set the connection
timeout );
+1 to committing this fix.
Re: [PATCH] fix util_ldap with older OpenLDAPs
On Wed, Aug 03, 2005 at 11:24:49AM -0600, Brad Nicholes wrote: This is why I would like to see the release of 2.2 as soon as possible. My only explanation is that I missed changing the NULL parameter to ldc-ldap when I did the backport of the conversion from global to per-connection from trunk (rev. 170805). Since the code bases for util_ldap are significantly different between trunk and 2.0, backporting this kind of functionality is closer to a rewrite than a backport. In this case, I just missed it. The correct code should be: OK great, thanks Brad. +1 here too and so committed. joe
Re: [PATCH] fix util_ldap with older OpenLDAPs
+1 to this fix. Folks, either agree the code is correct, disagree that it should be some other way, identify it's bugs, or hush up. Plenty of people ARE using 2.2 ldap auth today - and there is no reason to stand in the way of committing obvious bug fixes, especially for recently modified code that was just wrong. Once 2.2 has run around the block a few times, most users will pick it up to close such bugs. But this involves reconfiguration, and the users who would appreciate if we would just fix the bug aren't looking to be beta testers. Holding up segfaults as guns to their heads, attempting to force them to 2.1-unstable isn't cool. Bill At 03:59 PM 7/27/2005, Joe Orton wrote: Since 2.0.54, it seems mod_auth_ldap just segfaults on any request if built against older versions of OpenLDAP, 2.2.20 and earlier (pre-2005). It looks like this was another regression caused the addition of the LDAPConnectionTimeout option. (New features, stable branch, regressions? Hmmm, I spot a pattern) http://issues.apache.org/bugzilla/show_bug.cgi?id=34618 mentions this with upgrade OpenLDAP as the solution, which isn't really a great way to give your users that warm fuzzy feeling. Any objections to this? Index: modules/experimental/util_ldap.c === --- modules/experimental/util_ldap.c(revision 225591) +++ modules/experimental/util_ldap.c(working copy) @@ -50,7 +50,21 @@ #define LDAP_CA_TYPE_BASE64 2 #define LDAP_CA_TYPE_CERT7_DB 3 +#if APR_HAS_OPENLDAP_LDAPSDK +#include ldap_features.h +/* LDAP_OPT_NETWORK_TIMEOUT is broken in OpenLDAP 2.2.21, see + * OpenLDAP bug ITS 3487. */ + +#if LDAP_VENDOR_VERSION_MAJOR 2 || \ +(LDAP_VENDOR_VERSION_MAJOR == 2 LDAP_VENDOR_VERSION_MINOR 2) || \ +(LDAP_VENDOR_VERSION_MAJOR == 2 LDAP_VENDOR_VERSION_MINOR == 2 \ + LDAP_VENDOR_VERSION_PATCH 21) +#undef LDAP_OPT_NETWORK_TIMEOUT +#endif + +#endif /* APR_HAS_OPENLDAP_LDAPSDK */ + module AP_MODULE_DECLARE_DATA ldap_module; int util_ldap_handler(request_rec *r);
Re: [PATCH] fix util_ldap with older OpenLDAPs
William A. Rowe, Jr. said: +1 to this fix. Folks, either agree the code is correct, disagree that it should be some other way, identify it's bugs, or hush up. Plenty of people ARE using 2.2 ldap auth today - and there is no reason to stand in the way of committing obvious bug fixes, especially for recently modified code that was just wrong. I'm not sure where the impression was gained that anybody is standing in the way of fixing bugs in v2.0? I gave it a +1, Joe a +1 by submitting it, and you a +1 above. That's three. Bugs were fixed in v2.2 due to a rewrite that spanned APR v1.1 and httpd, making the fixes neither atomic nor easy to backport. That said, if people want to use v2.0, and if people are keen to fix the remaining bugs in v2.0, then ++1 to that. Patches posted will definitely not be blocked. Regards, Graham --
Re: [PATCH] fix util_ldap with older OpenLDAPs
On Wed, Jul 27, 2005 at 11:39:36PM +0200, Graham Leggett wrote: Joe Orton wrote: Any objections to this? None at all - if the v2.0 code can be made more stable this is always a good thing, but there are lots more problems in the v2.0 code that are fixed in the v2.2 rewrite. Ah, sorry, I'd presumed this would affect 2.2 as well since the timeout handling there looked much the same, but it seems not (though I don't claim to understand why :). I'll just add this to STATUS for 2.0.x in that case. joe
[PATCH] fix util_ldap with older OpenLDAPs
Since 2.0.54, it seems mod_auth_ldap just segfaults on any request if built against older versions of OpenLDAP, 2.2.20 and earlier (pre-2005). It looks like this was another regression caused the addition of the LDAPConnectionTimeout option. (New features, stable branch, regressions? Hmmm, I spot a pattern) http://issues.apache.org/bugzilla/show_bug.cgi?id=34618 mentions this with upgrade OpenLDAP as the solution, which isn't really a great way to give your users that warm fuzzy feeling. Any objections to this? Index: modules/experimental/util_ldap.c === --- modules/experimental/util_ldap.c(revision 225591) +++ modules/experimental/util_ldap.c(working copy) @@ -50,7 +50,21 @@ #define LDAP_CA_TYPE_BASE64 2 #define LDAP_CA_TYPE_CERT7_DB 3 +#if APR_HAS_OPENLDAP_LDAPSDK +#include ldap_features.h +/* LDAP_OPT_NETWORK_TIMEOUT is broken in OpenLDAP 2.2.21, see + * OpenLDAP bug ITS 3487. */ + +#if LDAP_VENDOR_VERSION_MAJOR 2 || \ +(LDAP_VENDOR_VERSION_MAJOR == 2 LDAP_VENDOR_VERSION_MINOR 2) || \ +(LDAP_VENDOR_VERSION_MAJOR == 2 LDAP_VENDOR_VERSION_MINOR == 2 \ + LDAP_VENDOR_VERSION_PATCH 21) +#undef LDAP_OPT_NETWORK_TIMEOUT +#endif + +#endif /* APR_HAS_OPENLDAP_LDAPSDK */ + module AP_MODULE_DECLARE_DATA ldap_module; int util_ldap_handler(request_rec *r);
Re: [PATCH] fix util_ldap with older OpenLDAPs
Joe Orton wrote: Since 2.0.54, it seems mod_auth_ldap just segfaults on any request if built against older versions of OpenLDAP, 2.2.20 and earlier (pre-2005). It looks like this was another regression caused the addition of the LDAPConnectionTimeout option. (New features, stable branch, regressions? Hmmm, I spot a pattern) The LDAP code in v2.0.x is experimental, so is cannot be expected to be as stable as the rest of the code. To be more specific, a lot of the v2.0 code was rewritten and refactored for v2.2 and was not backported to v2.0 as the new code depended on APR v1.0. http://issues.apache.org/bugzilla/show_bug.cgi?id=34618 mentions this with upgrade OpenLDAP as the solution, which isn't really a great way to give your users that warm fuzzy feeling. The experimental tag on the LDAP code should have dispensed with any warm fuzzy feelings from the outset. Any objections to this? None at all - if the v2.0 code can be made more stable this is always a good thing, but there are lots more problems in the v2.0 code that are fixed in the v2.2 rewrite. Roll on httpd v2.2. Regards, Graham --
Re: [PATCH] fix util_ldap with older OpenLDAPs
Graham Leggett wrote: None at all - if the v2.0 code can be made more stable this is always a good thing, but there are lots more problems in the v2.0 code that are fixed in the v2.2 rewrite. Roll on httpd v2.2. Agreed, but some of us need an officially stable Apache with good LDAP authentication support. Apache 2.0.54 is the best thing out there in this regard, but as you note it leaves a lot to be desired. Apache 2.2 seems infinitely far away... -- Jess Holle
Re: [PATCH] fix util_ldap with older OpenLDAPs
Jess Holle wrote: Agreed, but some of us need an /officially /stable Apache with good LDAP authentication support. The problem is that the v2.0.x code was not very defensive, which means a simple failure up front causes an obscure and random crash somewhere further down in the code. The fixes as a result were relatively large and difficult to review for v2.0.x. Some of the fixes involved new and changed interfaces to apr-util which were difficult to backport. As a result the v2.0.x code lags far behind v2.2, and backporting fixes is a not trivial task. I think the effort is better spent getting v2.2 out the door. Apache 2.0.54 is the best thing out there in this regard, but as you note it leaves a lot to be desired. Apache 2.2 seems infinitely far away... v2.2 has been branched, not sure when the first actual release on this branch will be? Regards, Graham --
Re: [PATCH] fix util_ldap with older OpenLDAPs
Graham Leggett wrote: Jess Holle wrote: Agreed, but some of us need an /officially /stable Apache with good LDAP authentication support. The problem is that the v2.0.x code was not very defensive, which means a simple failure up front causes an obscure and random crash somewhere further down in the code. The fixes as a result were relatively large and difficult to review for v2.0.x. Some of the fixes involved new and changed interfaces to apr-util which were difficult to backport. As a result the v2.0.x code lags far behind v2.2, and backporting fixes is a not trivial task. I think the effort is better spent getting v2.2 out the door. I can't argue with this -- as much as I want multiple-provider support right now. [2.0.54's LDAP largely seems to hold up overall -- as amazing as that seems...] Apache 2.0.54 is the best thing out there in this regard, but as you note it leaves a lot to be desired. Apache 2.2 seems infinitely far away... v2.2 has been branched, not sure when the first actual release on this branch will be? That and the delay from then until the broader audience that suddenly turns on it shakes out problems the smaller development community did not are the big questions. I'm still hoping our next product releases can incorporate 2.2... -- Jess Holle
Re: [PATCH] fix util_ldap with older OpenLDAPs
Jess Holle wrote: I can't argue with this -- as much as I want multiple-provider support right now. [2.0.54's LDAP largely seems to hold up overall -- as amazing as that seems...] Most of the bugs in the LDAP code come about because of bad handling of timed out connections - meaning you get problems on low or no load, rather than on high load, which is a very frustrating thing to debug. I would find a problem, see the server worked, throw a few thousand hits at it to be sure, then get back the next day to find it was broken again - connections had timed out in the mean time and broke the server again. If there is a definite need, walk backwards through the patches to v2.2 and to apr-util to see what is fixed in each patch, I;m sure there are some segfaults that can be found if someone has the time to walk through it. That and the delay from then until the broader audience that suddenly turns on it shakes out problems the smaller development community did not are the big questions. I'm still hoping our next product releases can incorporate 2.2... I've been using v2.1.6 in production for a while (I needed the LDAP fixes, so it was the only viable option) with no hassles. If we get v2.2 out the door, I think we could ramp up to a stable and trusted version pretty quickly. Regards, Graham --
