On 6/29/23 5:08 PM, Yann Ylavic wrote:
> 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?
Thanks for the good catch. With regards to mod_proxy_ajp I think we should
use conn->addr instead of worker->cp->addr. We cannot be sure that
worker->cp->addr is really set.
I guess it did not cause any problems so far as in the AJP case we always reuse
connections by default
and hence worker->cp->addr is set.
I think using a refcount could be burdensome as we need to ensure correct
increase and decrease of the refcount and
we might have 3rd party modules that do not play well. Hence I propose a copy
approach (but thinking of it, it might
fail in the same or other ways with 3rd party modules using worker->cp->addr
directly in a similar way like mod_proxy_ajp does today)
I modified some code from your patch especially ap_proxy_worker_addr_get:
Notes:
1. I am aware that apr_sockaddr_info_copy is only available in APR >= 1.6
2. ttlexpired is a pseudocode boolean to incorporate your idea of a TTL.
Details would need to be worked out.
3. If *addrp != NULL the idea is that we have an acceptable apr_sockaddr_t
provided that neither the TTL expired
or worker->cp->addr is NULL and thus we would need to do another lookup.
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,
proxy_conn_rec *conn, /*
Can be NULL */
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;
if (conn) {
p = conn->pool;
addrp = &(conn->addr);
}
if (addr_reusable) {
addr = AP_VOLATILIZE_T(apr_sockaddr_t *, worker->cp->addr);
if (addr && !ttlexpired && *addrp) {
return APR_SUCCESS;
}
if (!addr || ttlexpired) {
#if APR_HAS_THREADS
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");
}
*addrp = NULL;
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
}
}
*addrp = NULL;
/* Need a DNS lookup? */
if (!addr || ttlexpired) {
apr_pool_t *lookup_pool;
if (conn) {
/*
* If we need to do a lookup we should do a fresh connect
afterwards.
* Hence cleanup conn from scratch.
*/
apr_pool_clear(p);
conn = apr_pcalloc(p, sizeof(proxy_conn_rec));
conn->pool = p;
conn->worker = worker;
apr_pool_create(&(conn->scpool), p);
apr_pool_tag(conn->scpool, "proxy_conn_scpool");
}
if (addr_reusable) {
lookup_pool = worker->cp->dns_pool;
apr_pool_clear(lookup_pool);
}
else {
lookup_pool = p;
}
status = apr_sockaddr_info_get(&addr, host, APR_UNSPEC, port, 0,
lookup_pool);
if (addr_reusable && status == APR_SUCCESS) {
AP_VOLATILIZE_T(apr_sockaddr_t *, worker->cp->addr) = addr;
}
}
if (status == APR_SUCCESS) {
if (addr_reusable) {
apr_sockaddr_info_copy(addrp, addr, p);
}
else {
*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;
}
In ap_proxy_determine_connection we could then call
err = ap_proxy_worker_addr_get(worker, NULL,
conn->hostname, conn->port,
r, r->server, conn, NULL);
BTW: I think p was the wrong pool in your and conn->pool needs to be used
instead.
In mod_proxy_hcheck.c we could use
ap_proxy_worker_addr_get(worker, addr,
worker->s->hostname_ex, worker->s->port,
NULL, ctx->s, NULL, p);
There are still some gory details to be worked out, but this would be my high
level approach.
Regards
Rüdiger
