Re: svn commit: r1909401 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On 7/5/23 2:31 PM, Yann Ylavic wrote:
> On Fri, Jun 30, 2023 at 4:08 PM Yann Ylavic wrote:
>>
>> On Fri, Jun 30, 2023 at 2:35 PM Ruediger Pluem wrote:
>>>
>>> On 6/29/23 5:08 PM, Yann Ylavic wrote:
On Tue, Apr 25, 2023 at 1:57 PM 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)
>>
>> Let me look at what I can come up with and compare with what you
>> propose below (which does not look trivial either from a quick look).
>> That is to say, I need a bit more time to have an opinion here :)
>
> So I went with https://github.com/apache/httpd/pull/367 (sorry, not
> much detailed commits yet), and specifically the first commit for the
> reuse / ttl / force-expiring of the worker address(es).
>
> The new function is (named after ap_proxy_determine_connection):
>
> /*
> * Resolve an address, reusing the one of the worker if any.
> * @param proxy_function calling proxy scheme (http, ajp, ...)
> * @param conn proxy connection the address is used for
> * @param hostname host to resolve (should be the worker's if reusable)
> * @param hostport port to resolve (should be the worker's if reusable)
> * @param r current request (if any)
> * @param s current server (or NULL if r != NULL and ap_proxyerror()
> * should be called on error)
> * @return APR_SUCCESS or an error
> */
> PROXY_DECLARE(apr_status_t) ap_proxy_determine_address(const char
> *proxy_function,
>proxy_conn_rec *conn,
>const char *hostname,
>apr_port_t hostport,
>request_rec *r,
>server_rec *s);
>
> and it takes a proxy_conn_rec only, which simplifies things and which
> we can provide everywhere after all (see the changes in mod_proxy_ftp
> and mod_proxy_hcheck).
>
> A new struct proxy_address (refcounted, TTL'ed) is defined to store an
> address (when needed only, i.e. worker->is_address_reusable). It's
> allocated as a subpool of worker->cp->dns_pool and looks like this:
> struct proxy_address {
> apr_sockaddr_t *addr; /* Remote address info */
> const char *hostname; /* Remote host name */
> apr_port_t hostport;/* Remote host port */
> apr_uint32_t refcount; /* Number
Re: svn commit: r1909401 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On Fri, Jun 30, 2023 at 4:08 PM Yann Ylavic wrote:
>
> On Fri, Jun 30, 2023 at 2:35 PM Ruediger Pluem wrote:
> >
> > On 6/29/23 5:08 PM, Yann Ylavic wrote:
> > > On Tue, Apr 25, 2023 at 1:57 PM 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)
>
> Let me look at what I can come up with and compare with what you
> propose below (which does not look trivial either from a quick look).
> That is to say, I need a bit more time to have an opinion here :)
So I went with https://github.com/apache/httpd/pull/367 (sorry, not
much detailed commits yet), and specifically the first commit for the
reuse / ttl / force-expiring of the worker address(es).
The new function is (named after ap_proxy_determine_connection):
/*
* Resolve an address, reusing the one of the worker if any.
* @param proxy_function calling proxy scheme (http, ajp, ...)
* @param conn proxy connection the address is used for
* @param hostname host to resolve (should be the worker's if reusable)
* @param hostport port to resolve (should be the worker's if reusable)
* @param r current request (if any)
* @param s current server (or NULL if r != NULL and ap_proxyerror()
* should be called on error)
* @return APR_SUCCESS or an error
*/
PROXY_DECLARE(apr_status_t) ap_proxy_determine_address(const char
*proxy_function,
proxy_conn_rec *conn,
const char *hostname,
apr_port_t hostport,
request_rec *r,
server_rec *s);
and it takes a proxy_conn_rec only, which simplifies things and which
we can provide everywhere after all (see the changes in mod_proxy_ftp
and mod_proxy_hcheck).
A new struct proxy_address (refcounted, TTL'ed) is defined to store an
address (when needed only, i.e. worker->is_address_reusable). It's
allocated as a subpool of worker->cp->dns_pool and looks like this:
struct proxy_address {
apr_sockaddr_t *addr; /* Remote address info */
const char *hostname; /* Remote host name */
apr_port_t hostport;/* Remote host port */
apr_uint32_t refcount; /* Number of conns and/or worker using it */
apr_uint32_t expiry;/* Expiry timestamp (se
Re: svn commit: r1909401 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On Fri, Jun 30, 2023 at 2:35 PM Ruediger Pluem wrote: > > On 6/29/23 5:08 PM, Yann Ylavic wrote: > > On Tue, Apr 25, 2023 at 1:57 PM 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) Let me look at what I can come up with and compare with what you propose below (which does not look trivial either from a quick look). That is to say, I need a bit more time to have an opinion here :) > > I modified some code from your patch especially ap_proxy_worker_addr_get: Thanks; Yann.
Re: svn commit: r1909401 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On 6/29/23 5:08 PM, Yann Ylavic wrote:
> On Tue, Apr 25, 2023 at 1:57 PM 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 n
Re: svn commit: r1909401 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On Tue, Apr 25, 2023 at 1:57 PM 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 addrpreturned address pointer
+ * @param host host to resolve (the worker's if reusable)
+ * @param host port to resolve (the worker's if reusable)
+ * @param rcurrent request (if any)
+ * @param scurrent server (if any)
+ * @param ppool 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-
