On Fri, Jun 30, 2023 at 2:35 PM Ruediger Pluem <rpl...@apache.org> wrote: > > 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)
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.
