From 394bd4eb39e60d33ed3e2a7449c00153cfe3d351 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Tue, 24 Oct 2023 18:31:55 +0200 Subject: [PATCH] BUG/MAJOR: backend: fix idle conn crash under low FD Since the following commit, idle conns are stored in a list as secondary storage to retrieve them in usage order : 5afcb686b93c3811bd859a331efd6a8341a61218 MAJOR: connection: purge idle conn by last usage The list usage has been extended wherever connections lookup are done both on idle and safe trees. This reduced the code size by replacing a two tree loops by a single list loop. LIST_ELEM() is used in this context to retrieve the first idle list element from the server list head. However, macro usage was wrong due to an extra '&' operator which returns an invalid connection reference. This will most of the time caused a crash on conn_delete_from_tree() or affiliated functions. This bug only occurs if the FD pool is exhausted and some idle connections are selected to be killed. It can be reproduced using the following config and h2load command : $ h2load -t 8 -c 800 -m 10 -n 800 "http://127.0.0.1:21080/?s=10k" global maxconn 100 defaults mode http timeout connect 20s timeout client 20s timeout server 20s listen li bind :21080 proto h2 server nginx 127.99.0.1:30080 proto h1 This bug has been introduced by the above commit. Thus no need to backport this fix. Note that LIST_ELEM() macro usage was slightly adjusted also in srv_migrate_conns_to_remove(). The function used toremove_list instead of idle_list connection list element. This is not a bug as they are stored in the same union. However, the new code is clearer as it intends to move connection from the idle_list only into the toremove_list mt-list. --- src/backend.c | 4 ++-- src/server.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/backend.c b/src/backend.c index c6317bb26..359bd499f 100644 --- a/src/backend.c +++ b/src/backend.c @@ -1515,7 +1515,7 @@ static int connect_server(struct stream *s) /* First, try from our own idle list */ HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); if (!LIST_ISEMPTY(&srv->per_thr[tid].idle_conn_list)) { - tokill_conn = LIST_ELEM(&srv->per_thr[tid].idle_conn_list.n, struct connection *, idle_list); + tokill_conn = LIST_ELEM(srv->per_thr[tid].idle_conn_list.n, struct connection *, idle_list); conn_delete_from_tree(tokill_conn); HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); @@ -1544,7 +1544,7 @@ static int connect_server(struct stream *s) continue; if (!LIST_ISEMPTY(&srv->per_thr[i].idle_conn_list)) { - tokill_conn = LIST_ELEM(&srv->per_thr[i].idle_conn_list.n, struct connection *, idle_list); + tokill_conn = LIST_ELEM(srv->per_thr[i].idle_conn_list.n, struct connection *, idle_list); conn_delete_from_tree(tokill_conn); } HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[i].idle_conns_lock); diff --git a/src/server.c b/src/server.c index e7ce3d70b..37e8beba0 100644 --- a/src/server.c +++ b/src/server.c @@ -5949,7 +5949,7 @@ static int srv_migrate_conns_to_remove(struct list *list, struct mt_list *toremo if (toremove_nb != -1 && i >= toremove_nb) break; - conn = LIST_ELEM(list->n, struct connection *, toremove_list); + conn = LIST_ELEM(list->n, struct connection *, idle_list); conn_delete_from_tree(conn); MT_LIST_APPEND(toremove_list, &conn->toremove_list); i++;