Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c
On 13 Oct 2004 15:25:06 -, [EMAIL PROTECTED] <[EMAIL PROTECTED]> wrote:
> minfrin 2004/10/13 08:25:06
>
> Modified:.Tag: APACHE_2_0_BRANCH CHANGES STATUS
>modules/experimental Tag: APACHE_2_0_BRANCH util_ldap.c
> Log:
> mod_ldap: fix a bogus error message to tell the user which file
> is causing a potential problem with the LDAP shared memory cache.
> Index: util_ldap.c
> @@ -1425,12 +1425,15 @@
>
>sts = apr_global_mutex_child_init(&st->util_ldap_cache_lock, st->lock_file,
> p);
>if (sts != APR_SUCCESS) {
> -ap_log_error(APLOG_MARK, APLOG_CRIT, sts, s, "failed to init caching lock
> in child process");
> +ap_log_error(APLOG_MARK, APLOG_CRIT, sts, s,
> + "Failed to initialise global mutex %s in child process %d.",
> + st->lock_file, getpid());
another fix to make: format string for getpid() is "%" APR_PID_T_FMT
instead of "%d"... there are common platforms that use long instead
of int for the pid
>return;
>}
>else {
>ap_log_error(APLOG_MARK, APLOG_DEBUG|APLOG_NOERRNO, 0, s,
> - "INIT global mutex %s in child %d ", st->lock_file,
> getpid());
> + "Initialisation of global mutex %s in child process %d
> successful.",
> + st->lock_file, getpid());
ditto
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c
Agreed, and interested in other thoughts as well. Brad Brad Nicholes Senior Software Engineer Novell, Inc., the leading provider of Net business solutions http://www.novell.com >>> Graham Leggett <[EMAIL PROTECTED]> Monday, June 28, 2004 11:23:57 AM >>> Brad Nicholes wrote: > I was hoping to avoid the MMN bump mainly because that means we can't > backport the changes to the 2.0 branch. If the httpd 2.2 includes a > caching_util module then the only reason for these stabilization patches > is the 2.0 branch. Also, if there are any other modules that do depend > on util_ldap, wouldn't they be just as broken as mod_auth_ldap was and > probably want to update anyway? I don't know, I was hoping to get some > other feedback from the list. Well... the question is are there modules out there that use util_ldap right now? I think being experimental, the module is subject to incompatible change anyway, and there seems to be a catch 22 situation here - the module is currently broken, but the fix cannot be applied without an MMN bump (if it was a fully fledged module). I think to break this catch 22 situation, it seems reasonable to apply the changes to v2.0 without an MMN bump as the code is experimental. Any other thoughts? Regards, Graham --
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c
Brad Nicholes wrote: I was hoping to avoid the MMN bump mainly because that means we can't backport the changes to the 2.0 branch. If the httpd 2.2 includes a caching_util module then the only reason for these stabilization patches is the 2.0 branch. Also, if there are any other modules that do depend on util_ldap, wouldn't they be just as broken as mod_auth_ldap was and probably want to update anyway? I don't know, I was hoping to get some other feedback from the list. Well... the question is are there modules out there that use util_ldap right now? I think being experimental, the module is subject to incompatible change anyway, and there seems to be a catch 22 situation here - the module is currently broken, but the fix cannot be applied without an MMN bump (if it was a fully fledged module). I think to break this catch 22 situation, it seems reasonable to apply the changes to v2.0 without an MMN bump as the code is experimental. Any other thoughts? Regards, Graham --
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c
>Modules (right now, only mod_auth_ldap) depend on util_ldap, and >changing the header would mean a break in binary compatibility (in >theory). I think we should probably stick with an MMN bump for this to >be consistent, even though only the mod_auth_ldap module uses it (to my >knowledge). I was hoping to avoid the MMN bump mainly because that means we can't backport the changes to the 2.0 branch. If the httpd 2.2 includes a caching_util module then the only reason for these stabilization patches is the 2.0 branch. Also, if there are any other modules that do depend on util_ldap, wouldn't they be just as broken as mod_auth_ldap was and probably want to update anyway? I don't know, I was hoping to get some other feedback from the list. >As for the header being in /include, I think it should be there so that >external modules can be built against util_ldap. If the header was in >/experiemental, the full httpd source would be needed to build an Apache >module that used util_ldap (unless I am misunderstanding something). True. It just seems a little strange. Brad Brad Nicholes Senior Software Engineer Novell, Inc., the leading provider of Net business solutions http://www.novell.com
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c
Brad Nicholes wrote: I'm not sure what you are asking. I haven't proposed any of my recent changes for backport yet because of the changes required in include/util_ldap.h. Since util_ldap.h can be considered a public header, technically any changes to the structure would require an MMN bump. But the question is since mod_auth_ldap and mod_ldap are still experimental, do the rules still apply. Being experimental modules would imply that changes like this could happen without notice. On a different but related issue. I still don't understand why util_ldap.h is in the /include directory. There is nothing in it that directly relates to anything in httpd itself. Without mod_ldap (aka. util_ldap) loaded, it makes no sense on its own. It simply defines all of the APIs and structures that are part of mod_ldap. If util_ldap.h were to move from /include to /experimental (for now) along side of the other util_ldap code, that would eliminate the MMN bump issue, right? Modules (right now, only mod_auth_ldap) depend on util_ldap, and changing the header would mean a break in binary compatibility (in theory). I think we should probably stick with an MMN bump for this to be consistent, even though only the mod_auth_ldap module uses it (to my knowledge). As for the header being in /include, I think it should be there so that external modules can be built against util_ldap. If the header was in /experiemental, the full httpd source would be needed to build an Apache module that used util_ldap (unless I am misunderstanding something). Regards, Graham --
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c
I'm not sure what you are asking. I haven't proposed any of my recent changes for backport yet because of the changes required in include/util_ldap.h. Since util_ldap.h can be considered a public header, technically any changes to the structure would require an MMN bump. But the question is since mod_auth_ldap and mod_ldap are still experimental, do the rules still apply. Being experimental modules would imply that changes like this could happen without notice. On a different but related issue. I still don't understand why util_ldap.h is in the /include directory. There is nothing in it that directly relates to anything in httpd itself. Without mod_ldap (aka. util_ldap) loaded, it makes no sense on its own. It simply defines all of the APIs and structures that are part of mod_ldap. If util_ldap.h were to move from /include to /experimental (for now) along side of the other util_ldap code, that would eliminate the MMN bump issue, right? Brad Brad Nicholes Senior Software Engineer Novell, Inc., the leading provider of Net business solutions http://www.novell.com >>> [EMAIL PROTECTED] Monday, June 28, 2004 6:24:26 AM >>> [EMAIL PROTECTED] wrote: > Check for a NULL file name before trying to delete the file Is this in STATUS? Regards, Graham --
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c
[EMAIL PROTECTED] wrote: Check for a NULL file name before trying to delete the file Is this in STATUS? Regards, Graham --
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c util_ldap_cache.c util_ldap_cache.h
This patch should fix the shared memory issues on Linux and Solaris.
But this code could use some more review and testing. I would like to
get mod_auth_ldap and mod_ldap out of experimental if this proves to
stabilize the module. On a related issue, I would also like to backport
these patches to the 2.0 tree. Does the change to the util_ldap_state_t
structure in util_ldap.h require a MMN bump even though mod_ldap is an
experimental module?
Brad
Brad Nicholes
Senior Software Engineer
Novell, Inc., the leading provider of Net business solutions
http://www.novell.com
>>> [EMAIL PROTECTED] Friday, June 25, 2004 1:56:35 PM >>>
bnicholes2004/06/25 12:56:35
Modified:include util_ldap.h
modules/experimental util_ldap.c util_ldap_cache.c
util_ldap_cache.h
Log:
Replace the thread reader/writer lock that protects the shared memory
cache to a global mutex so that the shared memory is protected across
processes.
Revision ChangesPath
1.20 +2 -1 httpd-2.0/include/util_ldap.h
Index: util_ldap.h
===
RCS file: /home/cvs/httpd-2.0/include/util_ldap.h,v
retrieving revision 1.19
retrieving revision 1.20
diff -u -r1.19 -r1.20
--- util_ldap.h 16 Jun 2004 23:25:27 - 1.19
+++ util_ldap.h 25 Jun 2004 19:56:35 - 1.20
@@ -102,8 +102,8 @@
apr_pool_t *pool; /* pool from which this state is
allocated */
#if APR_HAS_THREADS
apr_thread_mutex_t *mutex; /* mutex lock for the
connection list */
-apr_thread_rwlock_t *util_ldap_cache_lock;
#endif
+apr_global_mutex_t *util_ldap_cache_lock;
apr_size_t cache_bytes; /* Size (in bytes) of shared memory
cache */
char *cache_file; /* filename for shm */
@@ -124,6 +124,7 @@
/* cache ald */
void *util_ldap_cache;
+char *lock_file; /* filename for shm lock mutex */
} util_ldap_state_t;
1.34 +77 -25httpd-2.0/modules/experimental/util_ldap.c
Index: util_ldap.c
===
RCS file: /home/cvs/httpd-2.0/modules/experimental/util_ldap.c,v
retrieving revision 1.33
retrieving revision 1.34
diff -u -r1.33 -r1.34
--- util_ldap.c 17 Jun 2004 17:19:01 - 1.33
+++ util_ldap.c 25 Jun 2004 19:56:35 - 1.34
@@ -88,6 +88,11 @@
"\"http://www.w3.org/TR/REC-html40/frameset.dtd\";>\n"
#endif
+#define LDAP_CACHE_LOCK() \
+apr_global_mutex_lock(st->util_ldap_cache_lock)
+#define LDAP_CACHE_UNLOCK() \
+apr_global_mutex_unlock(st->util_ldap_cache_lock)
+
static void util_ldap_strdup (char **str, const char *newstr)
{
@@ -498,11 +503,8 @@
util_ldap_state_t *st = (util_ldap_state_t
*)ap_get_module_config(r->server->module_config, &ldap_module);
-/* read lock this function */
-LDAP_CACHE_LOCK_CREATE(st->pool);
-
/* get cache entry (or create one) */
-LDAP_CACHE_WRLOCK();
+LDAP_CACHE_LOCK();
curnode.url = url;
curl = util_ald_cache_fetch(st->util_ldap_cache, &curnode);
@@ -526,7 +528,7 @@
if (curl) {
/* no - it's a server side compare */
-LDAP_CACHE_RDLOCK();
+LDAP_CACHE_LOCK();
/* is it in the compare cache? */
newnode.reqdn = (char *)reqdn;
@@ -581,7 +583,7 @@
else {
if (curl) {
/* compare successful - add to the compare cache */
-LDAP_CACHE_WRLOCK();
+LDAP_CACHE_LOCK();
newnode.reqdn = (char *)reqdn;
newnode.dn = (char *)dn;
@@ -625,11 +627,8 @@
(util_ldap_state_t
*)ap_get_module_config(r->server->module_config,
&ldap_module);
-/* read lock this function */
-LDAP_CACHE_LOCK_CREATE(st->pool);
-
/* get cache entry (or create one) */
-LDAP_CACHE_WRLOCK();
+LDAP_CACHE_LOCK();
curnode.url = url;
curl = util_ald_cache_fetch(st->util_ldap_cache, &curnode);
if (curl == NULL) {
@@ -639,7 +638,7 @@
if (curl) {
/* make a comparison to the cache */
-LDAP_CACHE_RDLOCK();
+LDAP_CACHE_LOCK();
curtime = apr_time_now();
the_compare_node.dn = (char *)dn;
@@ -705,7 +704,7 @@
(LDAP_NO_SUCH_ATTRIBUTE == result)) {
if (curl) {
/* compare completed; caching result */
-LDAP_CACHE_WRLOCK();
+LDAP_CACHE_LOCK();
the_compare_node.lastcompare = curtime;
the_compare_node.result = result;
@@ -762,11 +761,8 @@
(util_ldap_state_t
*)ap_get_module_config(r->server->module_config,
&ldap_module);
-/* read lock this funct
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c util_ldap_cache.c util_ldap_cache.h
ptemp shouldn't ever be NULL on a post_config, right? I just fixed the code so that it checks for a NULL file name before calling apr_file_remove(). Brad Brad Nicholes Senior Software Engineer Novell, Inc., the leading provider of Net business solutions http://www.novell.com >>> [EMAIL PROTECTED] Saturday, June 26, 2004 10:01:07 AM >>> Brad Nicholes wrote: > No, I didn't change anything that would allow for anonymous shared > memory. This should probably check for a NULL before calling > apr_file_remove(). >> +apr_file_remove(st->cache_file, ptemp); Will this line segfault if ptempt is NULL? If not, then it should be fine. If so, we should probably test for NULLness first (as you recommend above). Regards, Graham --
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c util_ldap_cache.c util_ldap_cache.h
Brad Nicholes wrote: No, I didn't change anything that would allow for anonymous shared memory. This should probably check for a NULL before calling apr_file_remove(). +apr_file_remove(st->cache_file, ptemp); Will this line segfault if ptempt is NULL? If not, then it should be fine. If so, we should probably test for NULLness first (as you recommend above). Regards, Graham --
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c util_ldap_cache.c util_ldap_cache.h
No, I didn't change anything that would allow for anonymous shared
memory. This should probably check for a NULL before calling
apr_file_remove().
Brad
Brad Nicholes
Senior Software Engineer
Novell, Inc., the leading provider of Net business solutions
http://www.novell.com
>>> [EMAIL PROTECTED] Saturday, June 26, 2004 9:30:42 AM >>>
[EMAIL PROTECTED] wrote:
> +#if APR_HAS_SHARED_MEMORY
> +/* If the cache file already exists then delete it.
Otherwise we are
> + * going to run into problems creating the shared memory.
*/
> +apr_file_remove(st->cache_file, ptemp);
> +if (st->cache_file) {
> +char *lck_file = apr_pstrcat (st->pool,
st->cache_file, ".lck", NULL);
> +apr_file_remove(lck_file, ptemp);
> +}
> +#endif
Does this patch support the idea of the cache file being NULL (ie
anonymous shared memory?).
The previous code insisted on specifying a cache file, which didn't
work
properly under Linux. Now, not specifying a cache filename means "use
anonymous shared memory", I'd just like to check that this NULL
filename
is case is still handled properly.
Regards,
Graham
--
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c util_ldap_cache.c util_ldap_cache.h
[EMAIL PROTECTED] wrote:
+#if APR_HAS_SHARED_MEMORY
+/* If the cache file already exists then delete it. Otherwise we are
+ * going to run into problems creating the shared memory. */
+apr_file_remove(st->cache_file, ptemp);
+if (st->cache_file) {
+char *lck_file = apr_pstrcat (st->pool, st->cache_file, ".lck", NULL);
+apr_file_remove(lck_file, ptemp);
+}
+#endif
Does this patch support the idea of the cache file being NULL (ie
anonymous shared memory?).
The previous code insisted on specifying a cache file, which didn't work
properly under Linux. Now, not specifying a cache filename means "use
anonymous shared memory", I'd just like to check that this NULL filename
is case is still handled properly.
Regards,
Graham
--
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c
Brad Nicholes wrote: Do the docs need to be updated for this change? Allowing relative paths to be resolved against ServerRoot seemed like fairly standard procedure. Looking at the docs there now, I think you're right. Just wanted to check whether there was anything that implied an absolute path was necessary. Regards, Graham --
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c
Do the docs need to be updated for this change? Allowing relative paths to be resolved against ServerRoot seemed like fairly standard procedure. Brad Brad Nicholes Senior Software Engineer Novell, Inc., the leading provider of Net business solutions http://www.novell.com >>> [EMAIL PROTECTED] Tuesday, June 15, 2004 3:42:25 AM >>> [EMAIL PROTECTED] wrote: > bnicholes2004/06/11 09:15:43 > > Modified:.Tag: APACHE_2_0_BRANCH CHANGES STATUS >modules/experimental Tag: APACHE_2_0_BRANCH util_ldap.c > Log: > Allow relative paths for LDAPTrustedCA to be resolved against ServerRoot PR#26602 Have the docs been updated to reflect this? Just to confirm - are docs changes subject to RTC? Regards, Graham --
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c
* Graham Leggett <[EMAIL PROTECTED]> wrote: > Just to confirm - are docs changes subject to RTC? No. nd
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c
[EMAIL PROTECTED] wrote: bnicholes2004/06/11 09:15:43 Modified:.Tag: APACHE_2_0_BRANCH CHANGES STATUS modules/experimental Tag: APACHE_2_0_BRANCH util_ldap.c Log: Allow relative paths for LDAPTrustedCA to be resolved against ServerRoot PR#26602 Have the docs been updated to reflect this? Just to confirm - are docs changes subject to RTC? Regards, Graham --
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c
Brad Nicholes wrote: > But my guess is that this patch should significantly help to resolve the problem that you are seeing. I'll give it a whirl on the problem server, and get back to you whether it fixes it or not. Regards, Graham --
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c
I haven't tested that specific scenario but I can see where this patch would address that problem as well. The problem has been that if a bind failed, the connection is left in an unbound state but the connection record showed that the connection was still bound. As a result, the connection was never rebound or cleaned up. Any attempt to reuse this connecton would result in a failure of all operations. I still suspect that there may be some cases where ldap operation failures are not being cleaned up correctly, so I am continuing to research this. But my guess is that this patch should significantly help to resolve the problem that you are seeing. Brad Brad Nicholes Senior Software Engineer Novell, Inc., the leading provider of Net business solutions http://www.novell.com >>> [EMAIL PROTECTED] Thursday, May 13, 2004 8:51:34 AM >>> [EMAIL PROTECTED] wrote: > Modified:modules/experimental util_ldap.c > Log: > if the call to ldap_simple_bind_s() fails, the connection is left in an unbound state. Make sure that the connection record is updated to show the current state. I have been having a problem with auth_ldap in that it works great under load (10 requests from ab, all successful with varying concurrency), but doesn't work under no load - leave it standing for "a while", and suddenly all requests fail, I suspect due to all the ldap connections having been timed out with no recovery. Does this patch address this problem? Regards, Graham --
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c
[EMAIL PROTECTED] wrote: Modified:modules/experimental util_ldap.c Log: if the call to ldap_simple_bind_s() fails, the connection is left in an unbound state. Make sure that the connection record is updated to show the current state. I have been having a problem with auth_ldap in that it works great under load (10 requests from ab, all successful with varying concurrency), but doesn't work under no load - leave it standing for "a while", and suddenly all requests fail, I suspect due to all the ldap connections having been timed out with no recovery. Does this patch address this problem? Regards, Graham --
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c
You are correct although it isn't as bad as it seems. There are
actually two types of caching going on here. There is the ldap
connection cache and the user credential cache. Because the user
credentials are cached, subsequent requests from the same user are not
actually authenticated directly against LDAP. util_ldap simply checks
it own cache to make sure that the user name and password are good. If
so, then it by passes the LDAP search. Therefore, the values for the
ldap binddn and bindpw would only be replaced when the cached user
credentials expire and a reauthentication requires a new ldap search.
But over time or if the user credential cache is disabled, this
could still be a problem so I have attached a patch that eliminates the
repeated allocations from the pconf pool. Please let me know if you see
any problems with this additional patch. I would like to get this
commited and backported soon.
Brad
Brad Nicholes
Senior Software Engineer
Novell, Inc., the leading provider of Net business solutions
http://www.novell.com
>>> [EMAIL PROTECTED] Tuesday, April 13, 2004 5:44:19 AM >>>
[EMAIL PROTECTED] wrote:
> bnicholes2004/03/31 14:56:08
>
> Modified:modules/experimental util_ldap.c
> Log:
> Update the DN information associated with each LDAP connection
after util_ldap_cache_checkuserid() rebinds the connection.
>
> Revision ChangesPath
> 1.22 +12 -0 httpd-2.0/modules/experimental/util_ldap.c
>
> Index: util_ldap.c
>
===
> RCS file: /home/cvs/httpd-2.0/modules/experimental/util_ldap.c,v
> retrieving revision 1.21
> retrieving revision 1.22
> diff -u -r1.21 -r1.22
> --- util_ldap.c 9 Feb 2004 20:29:18 - 1.21
> +++ util_ldap.c 31 Mar 2004 22:56:08 - 1.22
> @@ -844,6 +844,18 @@
>ldap_msgfree(res);
>return result;
>}
> +else {
> +/*
> + * Since we just bound the connection to the
authenticating user id, update the
> + * ldc->binddn and ldc->bindpw to reflect the change and
also to allow the next
> + * call to util_ldap_connection_open() to handle the
connection reuse appropriately.
> + * Otherwise the next time that this connection is reused,
it will indicate that
> + * it is bound to the original user id specified
ldc->binddn when in fact it is
> + * bound to a completely different user id.
> + */
> +ldc->binddn = apr_pstrdup(st->pool, *binddn);
> +ldc->bindpw = apr_pstrdup(st->pool, bindpw);
isn't st->pool pconf? (or maybe I'm having trouble tracking ldc and st
;) )
can binddn and bindpw be repeatedly replaced with new values, creating
uncontrolled growth of pconf?
util_ldap.c.patch
Description: Binary data
Re: cvs commit: httpd-2.0/modules/experimental util_ldap.c
[EMAIL PROTECTED] wrote:
bnicholes2004/03/31 14:56:08
Modified:modules/experimental util_ldap.c
Log:
Update the DN information associated with each LDAP connection after util_ldap_cache_checkuserid() rebinds the connection.
Revision ChangesPath
1.22 +12 -0 httpd-2.0/modules/experimental/util_ldap.c
Index: util_ldap.c
===
RCS file: /home/cvs/httpd-2.0/modules/experimental/util_ldap.c,v
retrieving revision 1.21
retrieving revision 1.22
diff -u -r1.21 -r1.22
--- util_ldap.c 9 Feb 2004 20:29:18 - 1.21
+++ util_ldap.c 31 Mar 2004 22:56:08 - 1.22
@@ -844,6 +844,18 @@
ldap_msgfree(res);
return result;
}
+else {
+/*
+ * Since we just bound the connection to the authenticating user id, update the
+ * ldc->binddn and ldc->bindpw to reflect the change and also to allow the next
+ * call to util_ldap_connection_open() to handle the connection reuse appropriately.
+ * Otherwise the next time that this connection is reused, it will indicate that
+ * it is bound to the original user id specified ldc->binddn when in fact it is
+ * bound to a completely different user id.
+ */
+ldc->binddn = apr_pstrdup(st->pool, *binddn);
+ldc->bindpw = apr_pstrdup(st->pool, bindpw);
isn't st->pool pconf? (or maybe I'm having trouble tracking ldc and st ;) )
can binddn and bindpw be repeatedly replaced with new values, creating
uncontrolled growth of pconf?
