From: Eric Covener Date: Sat, 12 Mar 2011 21:18:21 +0000 (+0000) Subject: Lay some groundwork for improvements to the connection pool. X-Git-Tag: 2.3.12~245 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=03c032b4e467f43db1af596f4085888f11324b76;p=thirdparty%2Fapache%2Fhttpd.git 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 edaac77444c..70f25ce42d0 100644 --- 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 e6ff6a911cc..f3639b61bd9 100644 --- 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 a105881704c..8a9132f8ec1 100644 --- 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);