On Wed, Jun 19, 2024 at 9:07 AM Ruediger Pluem <rpl...@apache.org> wrote:
>
> On 6/18/24 4:20 PM, yla...@apache.org wrote:
> >
> > + /* Release the old conn address */
> > + if (conn->address) {
> > + /* conn->address->addr cannot be released while it's used
> > by
> > + * conn->sock->remote_addr, thus if the old address is
> > still
> > + * the one used at apr_socket_connect() time AND the socket
> > + * shouldn't be closed because the newly resolved address
> > is
> > + * the same. In this case let's (re)attach the old address
> > to
> > + * the socket lifetime (scpool), in any other case just
> > release
> > + * it now.
> > + */
> > + int addr_alive = 0,
> > + conn_alive = (conn->sock && conn->addr &&
> > + proxy_addrs_equal(conn->addr,
> > address->addr));
> > + if (conn_alive) {
> > + apr_sockaddr_t *remote_addr = NULL;
> > + apr_socket_addr_get(&remote_addr, APR_REMOTE,
> > conn->sock);
> > + addr_alive = (conn->addr == remote_addr);
>
> How can this ever be true? The remote_addr is always allocated from
> conn->scpool. How can
> conn->addr ever be allocated from conn->scpool? I think it always gets
> allocated from
> either a subpool of worker->cp->dns_pool, from conn->pool or from
> conn->uds_pool.
>
> > + }
All the complications[1] in this commit come from the fact that
apr_socket_connect(sock, addr) does a simple pointer copy of addr into
sock->remote_addr, which apr_socket_addr_get(APR_REMOTE) will return.
And in ap_proxy_connect_backend() we indeed do the same[2] as
apr_socket_connect(conn->sock, conn->addr).
So I think it does not matter which pool conn->addr was allocated
from[3] when apr_socket_connect() does a pointer copy?
[1] When a new conn->[address->]addr is determined and it's the same
address as the old one and we want to keep the socket open, we cannot
simply release the old conn->address without leaving a dangling
pointer in sock->remote_addr. That's why conn->[address->]addr is
reattached to conn->scpool to be cleaned up only when the socket is
closed.
[2] Well, not really the same because we'll walk all the
conn->addr[->next] to find one that works, so the conn->addr ==
remote_addr test should actually be a loop to find one of the
conn->addr[->next]. Will fix this!
[3] From worker_address_resolve() it's actually a subpool of
worker->cp->dns_pool since worker->s->is_address_reusable here.
Regards;
Yann.
