From 266d5405490050adeaf414158f7f4b9bad5298bc Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Fri, 24 Dec 2021 11:27:53 +0100 Subject: [PATCH] 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. --- include/haproxy/stream.h | 1 + src/queue.c | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/include/haproxy/stream.h b/include/haproxy/stream.h index 00f8175e19..a60e73f93e 100644 --- a/include/haproxy/stream.h +++ b/include/haproxy/stream.h @@ -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 { diff --git a/src/queue.c b/src/queue.c index 92692ab535..00e03a547c 100644 --- a/src/queue.c +++ b/src/queue.c @@ -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;