On Tue, Apr 25, 2023 at 1:57 PM <rpl...@apache.org> 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; }