On Tue, Apr 25, 2023 at 1:57 PM <[email protected]> wrote:
>
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Tue Apr 25 11:57:22 2023
> @@ -2790,7 +2790,11 @@ ap_proxy_determine_connection(apr_pool_t
> * 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.
> + *
> + * Clear the dns_pool before to avoid a memory leak in
> case
> + * we did the lookup again.
> */
> + apr_pool_clear(worker->cp->dns_pool);
> err = apr_sockaddr_info_get(&addr,
> conn->hostname, APR_UNSPEC,
> conn->port, 0,
I'm not sure we can clear the dns_pool, some conn->addr allocated on
it may be still in use by other threads.
While reviewing your backport proposal, I noticed that
worker->cp->addr was used concurrently in several places in mod_proxy
with no particular care, so I started to code a follow up, but it
needs that apr_pool_clear() too somewhere to avoid the leak and
figured it may not be safe (like the above).
I attach the patch here to show what I mean by insecure
worker->cp->addr usage if we start to modify it concurrently, though
more work is needed it seems..
If I am correct we need to refcount the worker->cp->addr (or rather a
worker->address struct). I had a patch which does that to handle a
proxy address_ttl parameter (above which address is renewed), I can
try to revive and mix it with the attached one, in a PR. WDYT?
Regards;
Yann.
diff --git a/modules/proxy/mod_proxy.h b/modules/proxy/mod_proxy.h
index 04fee22742..d55381de64 100644
--- a/modules/proxy/mod_proxy.h
+++ b/modules/proxy/mod_proxy.h
@@ -1041,6 +1041,25 @@ PROXY_DECLARE(int) ap_proxy_post_request(proxy_worker *worker,
request_rec *r,
proxy_server_conf *conf);
+/**
+ * Resolve an address, reusing the one of the worker if any.
+ * @param worker worker the address is used for
+ * @param addrp returned address pointer
+ * @param host host to resolve (the worker's if reusable)
+ * @param host port to resolve (the worker's if reusable)
+ * @param r current request (if any)
+ * @param s current server (if any)
+ * @param p pool to allocate the address (ignored for reusable worker)
+ * @return APR_SUCCESS or an error
+ */
+PROXY_DECLARE(apr_status_t) ap_proxy_worker_addr_get(proxy_worker *worker,
+ apr_sockaddr_t **addrp,
+ const char *host,
+ apr_port_t port,
+ request_rec *r,
+ server_rec *s,
+ apr_pool_t *p);
+
/**
* Determine backend hostname and port
* @param p memory pool used for processing
diff --git a/modules/proxy/mod_proxy_ajp.c b/modules/proxy/mod_proxy_ajp.c
index cedf71379c..34f13cbaf1 100644
--- a/modules/proxy/mod_proxy_ajp.c
+++ b/modules/proxy/mod_proxy_ajp.c
@@ -236,8 +236,7 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r,
if (status != APR_SUCCESS) {
conn->close = 1;
ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r, APLOGNO(00868)
- "request failed to %pI (%s:%d)",
- conn->worker->cp->addr,
+ "request failed to %pI (%s:%d)", conn->addr,
conn->worker->s->hostname_ex,
(int)conn->worker->s->port);
if (status == AJP_EOVERFLOW)
@@ -334,8 +333,7 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r,
conn->close = 1;
apr_brigade_destroy(input_brigade);
ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r, APLOGNO(00876)
- "send failed to %pI (%s:%d)",
- conn->worker->cp->addr,
+ "send failed to %pI (%s:%d)", conn->addr,
conn->worker->s->hostname_ex,
(int)conn->worker->s->port);
/*
@@ -376,8 +374,7 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r,
conn->close = 1;
apr_brigade_destroy(input_brigade);
ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r, APLOGNO(00878)
- "read response failed from %pI (%s:%d)",
- conn->worker->cp->addr,
+ "read response failed from %pI (%s:%d)", conn->addr,
conn->worker->s->hostname_ex,
(int)conn->worker->s->port);
@@ -676,8 +673,7 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r,
}
else {
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00892)
- "got response from %pI (%s:%d)",
- conn->worker->cp->addr,
+ "got response from %pI (%s:%d)", conn->addr,
conn->worker->s->hostname_ex,
(int)conn->worker->s->port);
@@ -701,8 +697,7 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r,
if (backend_failed) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r, APLOGNO(00893)
- "dialog to %pI (%s:%d) failed",
- conn->worker->cp->addr,
+ "dialog to %pI (%s:%d) failed", conn->addr,
conn->worker->s->hostname_ex,
(int)conn->worker->s->port);
/*
@@ -846,9 +841,8 @@ static int proxy_ajp_handler(request_rec *r, proxy_worker *worker,
if (status != APR_SUCCESS) {
backend->close = 1;
ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r, APLOGNO(00897)
- "cping/cpong failed to %pI (%s:%d)",
- worker->cp->addr, worker->s->hostname_ex,
- (int)worker->s->port);
+ "cping/cpong failed to %pI (%s:%d)", backend->addr,
+ worker->s->hostname_ex, (int)worker->s->port);
status = HTTP_SERVICE_UNAVAILABLE;
retry++;
continue;
diff --git a/modules/proxy/mod_proxy_ftp.c b/modules/proxy/mod_proxy_ftp.c
index 3de0f805e8..6ce842c1ed 100644
--- a/modules/proxy/mod_proxy_ftp.c
+++ b/modules/proxy/mod_proxy_ftp.c
@@ -976,12 +976,8 @@ static int proxy_ftp_handler(request_rec *r, proxy_worker *worker,
proxy_conn_rec *backend;
apr_socket_t *sock, *local_sock, *data_sock = NULL;
apr_sockaddr_t *connect_addr = NULL;
- apr_status_t rv;
conn_rec *origin, *data = NULL;
apr_status_t err = APR_SUCCESS;
-#if APR_HAS_THREADS
- apr_status_t uerr = APR_SUCCESS;
-#endif
apr_bucket_brigade *bb;
char *buf, *connectname;
apr_port_t connectport;
@@ -1005,8 +1001,8 @@ static int proxy_ftp_handler(request_rec *r, proxy_worker *worker,
/* stuff for PASV mode */
int connect = 0, use_port = 0;
char dates[APR_RFC822_DATE_LEN];
+ apr_status_t rv;
int status;
- apr_pool_t *address_pool;
/* is this for us? */
if (proxyhost) {
@@ -1120,43 +1116,17 @@ static int proxy_ftp_handler(request_rec *r, proxy_worker *worker,
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01036)
"connecting %s to %s:%d", url, connectname, connectport);
- if (worker->s->is_address_reusable) {
- if (!worker->cp->addr) {
-#if APR_HAS_THREADS
- if ((err = PROXY_THREAD_LOCK(worker)) != APR_SUCCESS) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, err, r, APLOGNO(01037) "lock");
- return HTTP_INTERNAL_SERVER_ERROR;
- }
-#endif
- }
- connect_addr = AP_VOLATILIZE_T(apr_sockaddr_t *, worker->cp->addr);
- address_pool = worker->cp->dns_pool;
- }
- else
- address_pool = r->pool;
-
- /* do a DNS lookup for the destination host */
- if (!connect_addr)
- err = apr_sockaddr_info_get(&(connect_addr),
- connectname, APR_UNSPEC,
- connectport, 0,
- address_pool);
- if (worker->s->is_address_reusable && !worker->cp->addr) {
- worker->cp->addr = connect_addr;
-#if APR_HAS_THREADS
- if ((uerr = PROXY_THREAD_UNLOCK(worker)) != APR_SUCCESS) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, uerr, r, APLOGNO(01038) "unlock");
- }
-#endif
- }
/*
* get all the possible IP addresses for the destname and loop through
* them until we get a successful connection
*/
+ err = ap_proxy_worker_addr_get(worker, &connect_addr,
+ connectname, connectport,
+ r, r->server, r->pool);
if (APR_SUCCESS != err) {
- return ap_proxyerror(r, HTTP_BAD_GATEWAY, apr_pstrcat(p,
- "DNS lookup failure for: ",
- connectname, NULL));
+ return ap_proxyerror(r, HTTP_BAD_GATEWAY,
+ apr_pstrcat(p, "DNS lookup failure for: ",
+ connectname, NULL));
}
/* check if ProxyBlock directive on this host */
diff --git a/modules/proxy/mod_proxy_hcheck.c b/modules/proxy/mod_proxy_hcheck.c
index eb3c713bf9..280dc18451 100644
--- a/modules/proxy/mod_proxy_hcheck.c
+++ b/modules/proxy/mod_proxy_hcheck.c
@@ -551,25 +551,24 @@ static proxy_worker *hc_get_hcworker(sctx_t *ctx, proxy_worker *worker,
static int hc_determine_connection(sctx_t *ctx, proxy_worker *worker,
apr_sockaddr_t **addr, apr_pool_t *p)
{
- apr_status_t rv = APR_SUCCESS;
+ apr_status_t rv;
+
/*
* normally, this is done in ap_proxy_determine_connection().
* TODO: Look at using ap_proxy_determine_connection() with a
* fake request_rec
*/
- if (worker->cp->addr) {
- *addr = worker->cp->addr;
- }
- else {
- rv = apr_sockaddr_info_get(addr, worker->s->hostname_ex,
- APR_UNSPEC, worker->s->port, 0, p);
- if (rv != APR_SUCCESS) {
- ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ctx->s, APLOGNO(03249)
- "DNS lookup failure for: %s:%d",
- worker->s->hostname_ex, (int)worker->s->port);
- }
+ rv = ap_proxy_worker_addr_get(worker, addr,
+ worker->s->hostname_ex, worker->s->port,
+ NULL, ctx->s, p);
+ if (rv != APR_SUCCESS) {
+ ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, ctx->s, APLOGNO(03249)
+ "DNS lookup failure for: %s:%d",
+ worker->s->hostname_ex, (int)worker->s->port);
+ return !OK;
}
- return (rv == APR_SUCCESS ? OK : !OK);
+
+ return OK;
}
static apr_status_t hc_init_worker(sctx_t *ctx, proxy_worker *worker)
@@ -587,10 +586,13 @@ static apr_status_t hc_init_worker(sctx_t *ctx, proxy_worker *worker)
ap_log_error(APLOG_MARK, APLOG_EMERG, rv, ctx->s, APLOGNO(03250) "Cannot init worker");
return rv;
}
- if (worker->s->is_address_reusable && !worker->s->disablereuse &&
- hc_determine_connection(ctx, worker, &worker->cp->addr,
+ /* Resolve worker->cp->addr now if it's reusable */
+ if (worker->s->is_address_reusable && !worker->s->disablereuse) {
+ apr_sockaddr_t *dummy = NULL;
+ if (hc_determine_connection(ctx, worker, &dummy,
worker->cp->pool) != OK) {
- rv = APR_EGENERAL;
+ rv = APR_EGENERAL;
+ }
}
}
return rv;
@@ -620,8 +622,6 @@ static int hc_get_backend(const char *proxy_function, proxy_conn_rec **backend,
int status;
status = ap_proxy_acquire_connection(proxy_function, backend, hc, ctx->s);
if (status == OK) {
- (*backend)->addr = hc->cp->addr;
- (*backend)->hostname = hc->s->hostname_ex;
if (strcmp(hc->s->scheme, "https") == 0 || strcmp(hc->s->scheme, "wss") == 0 ) {
if (!ap_ssl_has_outgoing_handlers()) {
ap_log_error(APLOG_MARK, APLOG_WARNING, 0, ctx->s, APLOGNO(03252)
diff --git a/modules/proxy/mod_proxy_http.c b/modules/proxy/mod_proxy_http.c
index 4a8bab1bd6..21bd3d1640 100644
--- a/modules/proxy/mod_proxy_http.c
+++ b/modules/proxy/mod_proxy_http.c
@@ -2154,16 +2154,16 @@ static int proxy_http_handler(request_rec *r, proxy_worker *worker,
*/
status = ap_proxy_http_request(req);
if (status != OK) {
- proxy_run_detach_backend(r, backend);
if (req->do_100_continue && status == HTTP_SERVICE_UNAVAILABLE) {
ap_log_rerror(APLOG_MARK, APLOG_INFO, status, r, APLOGNO(01115)
"HTTP: 100-Continue failed to %pI (%s:%d)",
- worker->cp->addr, worker->s->hostname_ex,
- (int)worker->s->port);
+ backend->addr, backend->hostname, backend->port);
+ proxy_run_detach_backend(r, backend);
backend->close = 1;
retry++;
continue;
}
+ proxy_run_detach_backend(r, backend);
break;
}
diff --git a/modules/proxy/proxy_util.c b/modules/proxy/proxy_util.c
index 750714560f..08f0e9089d 100644
--- a/modules/proxy/proxy_util.c
+++ b/modules/proxy/proxy_util.c
@@ -2620,6 +2620,79 @@ PROXY_DECLARE(int) ap_proxy_release_connection(const char *proxy_function,
return OK;
}
+PROXY_DECLARE(apr_status_t) ap_proxy_worker_addr_get(proxy_worker *worker,
+ apr_sockaddr_t **addrp,
+ const char *host,
+ apr_port_t port,
+ request_rec *r,
+ server_rec *s,
+ apr_pool_t *p)
+{
+ apr_sockaddr_t *addr = NULL;
+ apr_status_t status = APR_SUCCESS;
+ int addr_reusable = (worker->s->is_address_reusable
+ && !worker->s->disablereuse);
+#if APR_HAS_THREADS
+ int worker_locked = 0;
+#endif
+ apr_status_t rv;
+
+ *addrp = NULL;
+
+ if (addr_reusable) {
+ addr = AP_VOLATILIZE_T(apr_sockaddr_t *, worker->cp->addr);
+#if APR_HAS_THREADS
+ if (!addr) {
+ if ((rv = PROXY_THREAD_LOCK(worker))) {
+ if (r) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO()
+ "proxy lock");
+ }
+ else {
+ ap_log_error(APLOG_MARK, APLOG_ERR, rv, s, APLOGNO()
+ "proxy lock");
+ }
+ return rv;
+ }
+ /* Reload under the lock (might have been resolved in the meantime) */
+ addr = AP_VOLATILIZE_T(apr_sockaddr_t *, worker->cp->addr);
+ worker_locked = 1;
+ }
+#endif
+ }
+
+ /* Need a DNS lookup? */
+ if (!addr) {
+ if (addr_reusable) {
+ p = worker->cp->dns_pool;
+ apr_pool_clear(p);
+ }
+ status = apr_sockaddr_info_get(&addr, host, APR_UNSPEC, port, 0, p);
+ if (addr_reusable && status == APR_SUCCESS) {
+ AP_VOLATILIZE_T(apr_sockaddr_t *, worker->cp->addr) = addr;
+ }
+ }
+ if (status == APR_SUCCESS) {
+ *addrp = addr;
+ }
+
+#if APR_HAS_THREADS
+ if (worker_locked && (rv = PROXY_THREAD_UNLOCK(worker))) {
+ if (r) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO()
+ "proxy unlock");
+ }
+ else {
+ ap_log_error(APLOG_MARK, APLOG_ERR, rv, s, APLOGNO()
+ "proxy unlock");
+ }
+ return rv;
+ }
+#endif
+
+ return status;
+}
+
PROXY_DECLARE(int)
ap_proxy_determine_connection(apr_pool_t *p, request_rec *r,
proxy_server_conf *conf,
@@ -2634,9 +2707,6 @@ ap_proxy_determine_connection(apr_pool_t *p, request_rec *r,
{
int server_port;
apr_status_t err = APR_SUCCESS;
-#if APR_HAS_THREADS
- apr_status_t uerr = APR_SUCCESS;
-#endif
const char *uds_path;
/*
@@ -2714,6 +2784,7 @@ ap_proxy_determine_connection(apr_pool_t *p, request_rec *r,
}
else {
int will_reuse = worker->s->is_address_reusable && !worker->s->disablereuse;
+
if (!conn->hostname || !will_reuse) {
if (proxyname) {
conn->hostname = apr_pstrdup(conn->pool, proxyname);
@@ -2755,69 +2826,14 @@ ap_proxy_determine_connection(apr_pool_t *p, request_rec *r,
conn->hostname = apr_pstrdup(conn->pool, uri->hostname);
conn->port = uri->port;
}
- if (!will_reuse) {
- /*
- * Only do a lookup if we should not reuse the backend address.
- * Otherwise we will look it up once for the worker.
- */
- err = apr_sockaddr_info_get(&(conn->addr),
- conn->hostname, APR_UNSPEC,
- conn->port, 0,
- conn->pool);
- }
+
socket_cleanup(conn);
conn->close = 0;
}
- if (will_reuse) {
- /*
- * Looking up the backend address for the worker only makes sense if
- * we can reuse the address.
- *
- * As we indicate in the comment below that for retriggering a DNS
- * lookup worker->cp->addr should be set to NULL we need to avoid
- * a race that worker->cp->addr switches to NULL after we checked
- * it to be non NULL but before we assign it to conn->addr in an
- * else tree which would leave it to NULL and likely cause a
- * segfault later.
- */
- conn->addr = worker->cp->addr;
- if (!conn->addr) {
- if ((err = PROXY_THREAD_LOCK(worker)) != APR_SUCCESS) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, err, r, APLOGNO(00945) "lock");
- return HTTP_INTERNAL_SERVER_ERROR;
- }
- /*
- * Recheck addr after we got the lock. This may have changed
- * while waiting for the lock.
- */
- conn->addr = AP_VOLATILIZE_T(apr_sockaddr_t *, worker->cp->addr);
- if (!conn->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.
- *
- * Clear the dns_pool before to avoid a memory leak in case
- * we did the lookup already in the past.
- */
- apr_pool_clear(worker->cp->dns_pool);
- err = apr_sockaddr_info_get(&addr,
- conn->hostname, APR_UNSPEC,
- conn->port, 0,
- worker->cp->dns_pool);
- conn->addr = addr;
- worker->cp->addr = addr;
- }
- if ((uerr = PROXY_THREAD_UNLOCK(worker)) != APR_SUCCESS) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, uerr, r, APLOGNO(00946) "unlock");
- }
- }
- }
+ err = ap_proxy_worker_addr_get(worker, &conn->addr,
+ conn->hostname, conn->port,
+ r, r->server, p);
}
/* Close a possible existing socket if we are told to do so */
if (conn->close) {
@@ -3217,6 +3233,8 @@ PROXY_DECLARE(int) ap_proxy_connect_backend(const char *proxy_function,
apr_sockaddr_t *local_addr;
apr_socket_t *newsock;
void *sconf = s->module_config;
+ int addr_reusable = (worker->s->is_address_reusable
+ && !worker->s->disablereuse);
int did_dns_lookup = 0;
proxy_server_conf *conf =
(proxy_server_conf *) ap_get_module_config(sconf, &proxy_module);
@@ -3363,15 +3381,15 @@ PROXY_DECLARE(int) ap_proxy_connect_backend(const char *proxy_function,
* might have changed. Hence try a DNS lookup to see if this
* helps.
*/
- if (!backend_addr && !did_dns_lookup && worker->cp->addr) {
+ if (!backend_addr && addr_reusable && !did_dns_lookup) {
/*
* In case of an error backend_addr will be NULL which
* is enough to leave the loop.
*/
- apr_sockaddr_info_get(&backend_addr,
- conn->hostname, APR_UNSPEC,
- conn->port, 0,
- conn->pool);
+ AP_VOLATILIZE_T(apr_sockaddr_t *, worker->cp->addr) = NULL;
+ rv = ap_proxy_worker_addr_get(worker, &backend_addr,
+ conn->hostname, conn->port,
+ NULL, s, conn->pool);
did_dns_lookup = 1;
}
continue;
@@ -3467,19 +3485,6 @@ PROXY_DECLARE(int) ap_proxy_connect_backend(const char *proxy_function,
rv = APR_EINVAL;
}
- if ((rv == APR_SUCCESS) && did_dns_lookup) {
- /*
- * A local DNS lookup caused a successful connect. Trigger to update
- * the worker cache next time.
- * We don't care handling any locking errors. If something fails we
- * just continue with the existing cache value.
- */
- if (PROXY_THREAD_LOCK(worker) == APR_SUCCESS) {
- worker->cp->addr = NULL;
- PROXY_THREAD_UNLOCK(worker);
- }
- }
-
return rv == APR_SUCCESS ? OK : DECLINED;
}
