On 6/19/24 12:00 PM, Yann Ylavic wrote:
> On Wed, Jun 19, 2024 at 9:07 AM Ruediger Pluem <[email protected]> wrote:
>>
>> On 6/18/24 4:20 PM, [email protected] 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.
As far as I read the code it does not.
https://github.com/apache/apr/blob/b0a08c76ceacc2308d7cf1d5a7bb3c9b4665a432/network_io/unix/sockets.c#L423-L433
We copy the data (sa, salen family and port) not a pointer.
> 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.
As said I don't see a dangling pointer because I don't read a pointer copy from
the code.
>
> [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.
>
Regards
Rüdiger
