Lay some groundwork for improvements to the connection pool.

  remove unnecessary uldap_connection_cleanup (nothing needed between unbind
  and remove)

  properly remove rebind callback info when credentials change

  maintain a separate pool for the rebind callback storage so it can be cleared
  when the connection is unbound.

(major bump for util_ldap function removal)

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1081005 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/include/ap_mmn.h b/include/ap_mmn.h
index edaac77444c6c8f02606645f6ca70e6b6f482f9c..70f25ce42d0ac2c219f25e8121bd3c5f5973ae07 100644 (file)
--- a/include/ap_mmn.h
+++ b/include/ap_mmn.h
@@ -302,14 +302,18 @@
  * 20110203.1 (2.3.11-dev) Add ap_state_query()
  * 20110203.2 (2.3.11-dev) Add ap_run_pre_read_request() hook and
  *                         ap_parse_form_data() util
+ * 20110312.0 (2.3.12-dev) remove uldap_connection_cleanup and add 
+                           util_ldap_state_t.connectionPoolTTL,
+                           util_ldap_connection_t.freed, and
+                           util_ldap_connection_t.rebind_pool. 
  */
 
 #define MODULE_MAGIC_COOKIE 0x41503234UL /* "AP24" */
 
 #ifndef MODULE_MAGIC_NUMBER_MAJOR
-#define MODULE_MAGIC_NUMBER_MAJOR 20110203
+#define MODULE_MAGIC_NUMBER_MAJOR 20110312
 #endif
-#define MODULE_MAGIC_NUMBER_MINOR 2                     /* 0...n */
+#define MODULE_MAGIC_NUMBER_MINOR 0                     /* 0...n */
 
 /**
  * Determine if the server's current MODULE_MAGIC_NUMBER is at least a

diff --git a/include/util_ldap.h b/include/util_ldap.h
index e6ff6a911cc21f33d9941b24a2e9dde6fae73322..f3639b61bd9352b8835779b8a3c8b42880e58481 100644 (file)
--- a/include/util_ldap.h
+++ b/include/util_ldap.h
@@ -121,6 +121,8 @@ typedef struct util_ldap_connection_t {
 
     int ChaseReferrals;                 /* [on|off] (default = AP_LDAP_CHASEREFERRALS_ON)*/
     int ReferralHopLimit;               /* # of referral hops to follow (default = AP_LDAP_DEFAULT_HOPLIMIT) */
+    apr_time_t freed;                   /* the time this conn was placed back in the pool */
+    apr_pool_t *rebind_pool;            /* frequently cleared pool for rebind data */
 } util_ldap_connection_t;
 
 typedef struct util_ldap_config_t {
@@ -163,7 +165,7 @@ typedef struct util_ldap_state_t {
     struct timeval *opTimeout;
 
     int debug_level;                    /* SDK debug level */
-
+    int connectionPoolTTL;
 } util_ldap_state_t;
 
 /* Used to store arrays of attribute labels/values. */
@@ -208,15 +210,6 @@ APR_DECLARE_OPTIONAL_FN(void,uldap_connection_close,(util_ldap_connection_t *ldc
  */
 APR_DECLARE_OPTIONAL_FN(apr_status_t,uldap_connection_unbind,(void *param));
 
-/**
- * Cleanup a connection to an LDAP server
- * @param ldc A structure containing the expanded details of the server
- *            that was connected.
- * @tip This functions unbinds and closes the connection to the LDAP server
- * @fn apr_status_t util_ldap_connection_cleanup(util_ldap_connection_t *ldc)
- */
-APR_DECLARE_OPTIONAL_FN(apr_status_t,uldap_connection_cleanup,(void *param));
-
 /**
  * Find a connection in a list of connections
  * @param r The request record

diff --git a/modules/ldap/util_ldap.c b/modules/ldap/util_ldap.c
index a105881704c4600f0b94bb65f35ca04f32acb96f..8a9132f8ec1cf547e6ab1759e49aa7f1367e496f 100644 (file)
--- a/modules/ldap/util_ldap.c
+++ b/modules/ldap/util_ldap.c
@@ -65,6 +65,7 @@
 
 module AP_MODULE_DECLARE_DATA ldap_module;
 static const char *ldap_cache_mutex_type = "ldap-cache";
+static apr_status_t uldap_connection_unbind(void *param);
 
 #define LDAP_CACHE_LOCK() do {                                  \
     if (st->util_ldap_cache_lock)                               \
@@ -76,8 +77,6 @@ static const char *ldap_cache_mutex_type = "ldap-cache";
         apr_global_mutex_unlock(st->util_ldap_cache_lock);      \
 } while (0)
 
-static apr_status_t util_ldap_connection_remove (void *param);
-
 static void util_ldap_strdup (char **str, const char *newstr)
 {
     if (*str) {
@@ -144,19 +143,12 @@ static int util_ldap_handler(request_rec *r)
 static void uldap_connection_close(util_ldap_connection_t *ldc)
 {
 
-    /*
-     * QUESTION:
-     *
-     * Is it safe leaving bound connections floating around between the
-     * different modules? Keeping the user bound is a performance boost,
-     * but it is also a potential security problem - maybe.
-     *
-     * For now we unbind the user when we finish with a connection, but
-     * we don't have to...
-     */
-
+     /* We leave bound LDAP connections floating around in our pool,
+      * but always check/fix the binddn/bindpw when we take them out
+      * of the pool
+      */
      if (!ldc->keep) { 
-         util_ldap_connection_remove(ldc);
+         uldap_connection_unbind(ldc);
      }
      else { 
          /* mark our connection as available for reuse */
@@ -182,52 +174,18 @@ static apr_status_t uldap_connection_unbind(void *param)
             ldc->ldap = NULL;
         }
         ldc->bound = 0;
-    }
 
-    return APR_SUCCESS;
-}
-
-
-/*
- * Clean up an LDAP connection by unbinding and unlocking the connection.
- * This cleanup does not remove the util_ldap_connection_t from the 
- * per-virtualhost list of connections, does not remove the storage
- * for the util_ldap_connection_t or its data, and is NOT run automatically.
- */
-static apr_status_t uldap_connection_cleanup(void *param)
-{
-    util_ldap_connection_t *ldc = param;
-
-    if (ldc) {
-        /* Release the rebind info for this connection. No more referral rebinds required. */
+        /* 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);
         }
-
-        /* unbind and disconnect from the LDAP server */
-        uldap_connection_unbind(ldc);
-
-        /* free the username and password */
-        if (ldc->bindpw) {
-            free((void*)ldc->bindpw);
-            ldc->bindpw = NULL;
-        }
-        if (ldc->binddn) {
-            free((void*)ldc->binddn);
-            ldc->binddn = NULL;
-        }
-        /* ldc->reason is allocated from r->pool */
-        if (ldc->reason) {
-            ldc->reason = NULL;
-        }
-        /* unlock this entry */
-        uldap_connection_close(ldc);
-
     }
 
     return APR_SUCCESS;
 }
 
+
 /*
  * util_ldap_connection_remove frees all storage associated with the LDAP
  * connection and removes it completely from the per-virtualhost list of
@@ -263,9 +221,6 @@ static apr_status_t util_ldap_connection_remove (void *param) {
         prev = l;
     }
 
-    /* Some unfortunate duplication between this method
-     * and uldap_connection_cleanup()
-    */
     if (ldc->bindpw) {
         free((void*)ldc->bindpw);
     }
@@ -339,7 +294,7 @@ static int uldap_connection_init(request_rec *r,
 
     if (ldc->ChaseReferrals == AP_LDAP_CHASEREFERRALS_ON) {
         /* Now that we have an ldap struct, add it to the referral list for rebinds. */
-        rc = apr_ldap_rebind_add(ldc->pool, ldc->ldap, ldc->binddn, ldc->bindpw);
+        rc = apr_ldap_rebind_add(ldc->rebind_pool, ldc->ldap, ldc->binddn, ldc->bindpw);
         if (rc != APR_SUCCESS) {
             ap_log_error(APLOG_MARK, APLOG_ERR, rc, r->server,
                     "LDAP: Unable to add rebind cross reference entry. Out of memory?");
@@ -732,7 +687,13 @@ static util_ldap_connection_t *
                 !compare_client_certs(dc->client_certs, l->client_certs))
             {
                 /* the bind credentials have changed */
-                l->bound = 0;
+                uldap_connection_unbind(l);
+                        
+                /* forget the rebind info for this conn */
+                if (l->ChaseReferrals == AP_LDAP_CHASEREFERRALS_ON) {
+                    apr_ldap_rebind_remove(l->ldap);
+                }
+
                 util_ldap_strdup((char**)&(l->binddn), binddn);
                 util_ldap_strdup((char**)&(l->bindpw), bindpw);
                 break;
@@ -801,6 +762,17 @@ static util_ldap_connection_t *
 
         l->keep = 1;
 
+        if (l->ChaseReferrals == AP_LDAP_CHASEREFERRALS_ON) { 
+            if (apr_pool_create(&(l->rebind_pool), l->pool) != APR_SUCCESS) {
+                ap_log_rerror(APLOG_MARK, APLOG_CRIT, 0, r,
+                              "util_ldap: Failed to create memory pool");
+#if APR_HAS_THREADS
+                apr_thread_mutex_unlock(st->mutex);
+#endif
+                return NULL;
+            }
+        }
+
         if (p) {
             p->next = l;
         }
@@ -2949,7 +2921,6 @@ static void util_ldap_register_hooks(apr_pool_t *p)
     APR_REGISTER_OPTIONAL_FN(uldap_connection_open);
     APR_REGISTER_OPTIONAL_FN(uldap_connection_close);
     APR_REGISTER_OPTIONAL_FN(uldap_connection_unbind);
-    APR_REGISTER_OPTIONAL_FN(uldap_connection_cleanup);
     APR_REGISTER_OPTIONAL_FN(uldap_connection_find);
     APR_REGISTER_OPTIONAL_FN(uldap_cache_comparedn);
     APR_REGISTER_OPTIONAL_FN(uldap_cache_compare);