BUG/MEDIUM: backend: fix possible sockaddr leak on redispatch

A subtle change of target address allocation was introduced with commit
68cf3959b ("MINOR: backend: rewrite alloc of stream target address") in
2.4. Prior to this patch, a target address was allocated by function
assign_server_address() only if none was previously allocated. After
the change, the allocation became unconditional. Most of the time it
makes no difference, except when we pass multiple times through
connect_server() with SF_ADDR_SET cleared.

The most obvious fix would be to avoid allocating that address there
when already set, but the root cause is that since introduction of
dynamically allocated addresses, the SF_ADDR_SET flag lies. It can
be cleared during redispatch or during a queue redistribution without
the address being released.

This patch instead gives back all its correct meaning to SF_ADDR_SET
and guarantees that when not set no address is allocated, by freeing
that address at the few places the flag is cleared. The flag could
even be removed so that only the address is checked but that would
require to touch many areas for no benefit.

The easiest way to test it is to send requests to a proxy with l7
retries enabled, which forwards to a server returning 500:

  defaults
    mode http
    timeout client 1s
    timeout server 1s
    timeout connect 1s
    retry-on all-retryable-errors
    retries 1
    option redispatch

  listen proxy
    bind *:5000
    server app 0.0.0.0:5001

  frontend dummy-app
    bind :5001
    http-request return status 500

Issuing "show pools" on the CLI will show that pool "sockaddr" grows
as requests are redispatched, and remains stable with the fix. Even
"ps" will show that the process' RSS grows by ~160B per request.

This fix will need to be backported to 2.4. Note that before 2.5,
there's no strm->si[1].dst, strm->target_addr must be used instead.

This addresses github issue #1499. Special thanks to Daniil Leontiev
for providing a well-documented reproducer.
This commit is contained in:
Willy Tarreau 2021-12-24 11:27:53 +01:00
parent 9979d0d1ea
commit 266d540549
2 changed files with 5 additions and 0 deletions

View File

@ -342,6 +342,7 @@ static inline void stream_choose_redispatch(struct stream *s)
if (may_dequeue_tasks(objt_server(s->target), s->be))
process_srv_queue(objt_server(s->target));
sockaddr_free(&s->si[1].dst);
s->flags &= ~(SF_DIRECT | SF_ASSIGNED | SF_ADDR_SET);
si->state = SI_ST_REQ;
} else {

View File

@ -600,6 +600,10 @@ int pendconn_dequeue(struct stream *strm)
strm->flags &= ~(SF_DIRECT | SF_ASSIGNED | SF_ADDR_SET);
strm->flags |= p->strm_flags & (SF_DIRECT | SF_ASSIGNED | SF_ADDR_SET);
/* the entry might have been redistributed to another server */
if (!(strm->flags & SF_ADDR_SET))
sockaddr_free(&strm->si[1].dst);
if (p->target) {
/* a server picked this pendconn, it must skip LB */
strm->target = &p->target->obj_type;