From fadc83a84fee174c24604f5b37a5446953474f3a Mon Sep 17 00:00:00 2001 From: Ruediger Pluem Date: Fri, 11 Oct 2019 15:11:40 +0000 Subject: [PATCH] Fix pool concurrency problems Create a subpool of the connection pool for worker scoped DNS resolutions. This is needed to avoid race conditions in using the connection pool by multiple threads during ramp up. Recheck after obtaining the lock if we still need to do things or if they were already done by another thread while we were waiting on the lock. * modules/proxy/proxy_util.c: Create a subpool of the connection pool for worker scoped DNS resolutions and use it. * modules/proxy/mod_proxy.h: Define AP_VOLATILIZE_T and add dns_pool to struct proxy_conn_pool. * modules/proxy/mod_proxy_ftp.c: Use dns_pool and consider that worker->cp->addr is volatile in this location of the code. PR: 63503 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1868296 13f79535-47bb-0310-9956-ffa450edef68 --- CHANGES | 3 + include/ap_mmn.h | 4 +- modules/proxy/mod_proxy.h | 11 +-- modules/proxy/mod_proxy_ftp.c | 4 +- modules/proxy/proxy_util.c | 131 ++++++++++++++++++++-------------- 5 files changed, 93 insertions(+), 60 deletions(-) diff --git a/CHANGES b/CHANGES index d152919575..1bf06ed979 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,9 @@ -*- coding: utf-8 -*- Changes with Apache 2.5.1 + *) mod_proxy: Fix crash by resolving pool concurrency problems. PR 63503 + [Ruediger Pluem, Eric Covener] + *) core: On Windows, fix a start-up crash if is used with a path that is not valid (For example, testing for a file on a flash drive that is not mounted) [Christophe Jaillet] diff --git a/include/ap_mmn.h b/include/ap_mmn.h index af7fe2fb4a..7843a92cbc 100644 --- a/include/ap_mmn.h +++ b/include/ap_mmn.h @@ -613,6 +613,8 @@ * 20190312.2 (2.5.1-dev) Add ap_no2slash_ex() and merge_slashes to * core_server_conf. * 20190312.3 (2.5.1-dev) Add forward_100_continue{,_set} to proxy_dir_conf + * 20190312.4 (2.5.1-dev) Add add dns_pool to proxy_conn_pool and define + * AP_VOLATILIZE_T. */ #define MODULE_MAGIC_COOKIE 0x41503235UL /* "AP25" */ @@ -620,7 +622,7 @@ #ifndef MODULE_MAGIC_NUMBER_MAJOR #define MODULE_MAGIC_NUMBER_MAJOR 20190312 #endif -#define MODULE_MAGIC_NUMBER_MINOR 3 /* 0...n */ +#define MODULE_MAGIC_NUMBER_MINOR 4 /* 0...n */ /** * Determine if the server's current MODULE_MAGIC_NUMBER is at least a diff --git a/modules/proxy/mod_proxy.h b/modules/proxy/mod_proxy.h index 460c02c41a..fbfc5548f1 100644 --- a/modules/proxy/mod_proxy.h +++ b/modules/proxy/mod_proxy.h @@ -287,12 +287,15 @@ typedef struct { /* Connection pool */ struct proxy_conn_pool { - apr_pool_t *pool; /* The pool used in constructor and destructor calls */ - apr_sockaddr_t *addr; /* Preparsed remote address info */ - apr_reslist_t *res; /* Connection resource list */ - proxy_conn_rec *conn; /* Single connection for prefork mpm */ + apr_pool_t *pool; /* The pool used in constructor and destructor calls */ + apr_sockaddr_t *addr; /* Preparsed remote address info */ + apr_reslist_t *res; /* Connection resource list */ + proxy_conn_rec *conn; /* Single connection for prefork mpm */ + apr_pool_t *dns_pool; /* The pool used for worker scoped DNS resolutions */ }; +#define AP_VOLATILIZE_T(T, x) (*(T volatile *)&(x)) + /* worker status bits */ /* * NOTE: Keep up-to-date w/ proxy_wstat_tbl[] diff --git a/modules/proxy/mod_proxy_ftp.c b/modules/proxy/mod_proxy_ftp.c index f6f543ab3a..3ad5efec35 100644 --- a/modules/proxy/mod_proxy_ftp.c +++ b/modules/proxy/mod_proxy_ftp.c @@ -1129,8 +1129,8 @@ static int proxy_ftp_handler(request_rec *r, proxy_worker *worker, } #endif } - connect_addr = worker->cp->addr; - address_pool = worker->cp->pool; + connect_addr = AP_VOLATILIZE_T(apr_sockaddr_t *, worker->cp->addr); + address_pool = worker->cp->dns_pool; } else address_pool = r->pool; diff --git a/modules/proxy/proxy_util.c b/modules/proxy/proxy_util.c index 0075b61927..b5787420c2 100644 --- a/modules/proxy/proxy_util.c +++ b/modules/proxy/proxy_util.c @@ -1460,6 +1460,7 @@ static apr_status_t conn_pool_cleanup(void *theworker) static void init_conn_pool(apr_pool_t *p, proxy_worker *worker) { apr_pool_t *pool; + apr_pool_t *dns_pool; proxy_conn_pool *cp; /* @@ -1470,12 +1471,21 @@ static void init_conn_pool(apr_pool_t *p, proxy_worker *worker) */ apr_pool_create(&pool, p); apr_pool_tag(pool, "proxy_worker_cp"); + /* + * Create a subpool of the connection pool for worker + * scoped DNS resolutions. This is needed to avoid race + * conditions in using the connection pool by multiple + * threads during ramp up. + */ + apr_pool_create(&dns_pool, pool); + apr_pool_tag(dns_pool, "proxy_worker_dns"); /* * Alloc from the same pool as worker. * proxy_conn_pool is permanently attached to the worker. */ cp = (proxy_conn_pool *)apr_pcalloc(p, sizeof(proxy_conn_pool)); cp->pool = pool; + cp->dns_pool = dns_pool; worker->cp = cp; } @@ -2044,68 +2054,73 @@ PROXY_DECLARE(apr_status_t) ap_proxy_initialize_worker(proxy_worker *worker, ser ap_proxy_worker_name(p, worker)); } else { - ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(00927) - "initializing worker %s local", - ap_proxy_worker_name(p, worker)); apr_global_mutex_lock(proxy_mutex); - /* Now init local worker data */ + /* Check again after we got the lock if we are still uninitialized */ + if (!(AP_VOLATILIZE_T(unsigned int, worker->local_status) & PROXY_WORKER_INITIALIZED)) { + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(00927) + "initializing worker %s local", + ap_proxy_worker_name(p, worker)); + /* Now init local worker data */ #if APR_HAS_THREADS - if (worker->tmutex == NULL) { - rv = apr_thread_mutex_create(&(worker->tmutex), APR_THREAD_MUTEX_DEFAULT, p); - if (rv != APR_SUCCESS) { - ap_log_error(APLOG_MARK, APLOG_ERR, rv, s, APLOGNO(00928) - "can not create worker thread mutex"); - apr_global_mutex_unlock(proxy_mutex); - return rv; + if (worker->tmutex == NULL) { + rv = apr_thread_mutex_create(&(worker->tmutex), APR_THREAD_MUTEX_DEFAULT, p); + if (rv != APR_SUCCESS) { + ap_log_error(APLOG_MARK, APLOG_ERR, rv, s, APLOGNO(00928) + "can not create worker thread mutex"); + apr_global_mutex_unlock(proxy_mutex); + return rv; + } } - } #endif - if (worker->cp == NULL) - init_conn_pool(p, worker); - if (worker->cp == NULL) { - ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(00929) - "can not create connection pool"); - apr_global_mutex_unlock(proxy_mutex); - return APR_EGENERAL; - } + if (worker->cp == NULL) + init_conn_pool(p, worker); + if (worker->cp == NULL) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(00929) + "can not create connection pool"); + apr_global_mutex_unlock(proxy_mutex); + return APR_EGENERAL; + } - if (worker->s->hmax) { - rv = apr_reslist_create(&(worker->cp->res), - worker->s->min, worker->s->smax, - worker->s->hmax, worker->s->ttl, - connection_constructor, connection_destructor, - worker, worker->cp->pool); + if (worker->s->hmax) { + rv = apr_reslist_create(&(worker->cp->res), + worker->s->min, worker->s->smax, + worker->s->hmax, worker->s->ttl, + connection_constructor, connection_destructor, + worker, worker->cp->pool); - apr_pool_pre_cleanup_register(worker->cp->pool, worker, - conn_pool_cleanup); + apr_pool_pre_cleanup_register(worker->cp->pool, worker, + conn_pool_cleanup); - ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(00930) - "initialized pool in child %" APR_PID_T_FMT " for (%s) min=%d max=%d smax=%d", - getpid(), worker->s->hostname_ex, worker->s->min, - worker->s->hmax, worker->s->smax); + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(00930) + "initialized pool in child %" APR_PID_T_FMT " for (%s) min=%d max=%d smax=%d", + getpid(), worker->s->hostname_ex, worker->s->min, + worker->s->hmax, worker->s->smax); - /* Set the acquire timeout */ - if (rv == APR_SUCCESS && worker->s->acquire_set) { - apr_reslist_timeout_set(worker->cp->res, worker->s->acquire); - } + /* Set the acquire timeout */ + if (rv == APR_SUCCESS && worker->s->acquire_set) { + apr_reslist_timeout_set(worker->cp->res, worker->s->acquire); + } - } - else { - void *conn; + } + else { + void *conn; - rv = connection_constructor(&conn, worker, worker->cp->pool); - worker->cp->conn = conn; + rv = connection_constructor(&conn, worker, worker->cp->pool); + worker->cp->conn = conn; - ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, s, APLOGNO(00931) - "initialized single connection worker in child %" APR_PID_T_FMT " for (%s)", - getpid(), worker->s->hostname_ex); + ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, s, APLOGNO(00931) + "initialized single connection worker in child %" APR_PID_T_FMT " for (%s)", + getpid(), worker->s->hostname_ex); + } + if (rv == APR_SUCCESS) { + worker->local_status |= (PROXY_WORKER_INITIALIZED); + } } apr_global_mutex_unlock(proxy_mutex); } if (rv == APR_SUCCESS) { worker->s->status |= (PROXY_WORKER_INITIALIZED); - worker->local_status |= (PROXY_WORKER_INITIALIZED); } return rv; } @@ -2559,15 +2574,25 @@ ap_proxy_determine_connection(apr_pool_t *p, request_rec *r, } /* - * Worker can have the single constant backend address. - * The single DNS lookup is used once per worker. - * If dynamic change is needed then set the addr to NULL - * inside dynamic config to force the lookup. + * Recheck addr after we got the lock. This may have changed + * while waiting for the lock. */ - err = apr_sockaddr_info_get(&(worker->cp->addr), - conn->hostname, APR_UNSPEC, - conn->port, 0, - worker->cp->pool); + if (!AP_VOLATILIZE_T(apr_sockaddr_t *, worker->cp->addr)) { + + apr_sockaddr_t *addr; + + /* + * Worker can have the single constant backend address. + * The single DNS lookup is used once per worker. + * If dynamic change is needed then set the addr to NULL + * inside dynamic config to force the lookup. + */ + err = apr_sockaddr_info_get(&addr, + conn->hostname, APR_UNSPEC, + conn->port, 0, + worker->cp->dns_pool); + worker->cp->addr = addr; + } conn->addr = worker->cp->addr; if ((uerr = PROXY_THREAD_UNLOCK(worker)) != APR_SUCCESS) { ap_log_rerror(APLOG_MARK, APLOG_ERR, uerr, r, APLOGNO(00946) "unlock"); -- 2.50.1