From ca686e32084d7aed18b01d45f51eb6079c02d9da Mon Sep 17 00:00:00 2001 From: Olivier Houchard Date: Mon, 20 Jul 2026 16:05:51 +0200 Subject: [PATCH] BUG/MEDIUM: server: Properly check for streams before deletion When a server delete command happens, before we actually delete the server, we check if there are any streams still attached to the server, and will refuse the deletion if it is so. Unfortunately, the check was not exhaustive, we would check if there's any stream in the queue, if there are any connection to the server established, and if served is non-zero. But there are at least two cases where a stream will have a reference to the server, but none of those will be true : the first one is the case where we tried a connection to the server, and it failed. At this point we decremented served, but we will still hold a reference to the server in target until a new server has been assigned. The second one happens with cookie persistence, in which case target will be set to the server address way before any connection is attempted, and so way before served is incremented. To fix that, introduce a new per-thread-group counter in the server structure, nb_strm, that counts the streams whose target points to that server. It is incremented when a stream's target is set to a server, and decremented when the target changes to another server, to a non-server, or the stream ends. To make this reliable, all assignments to a stream's target now go through the new stream_set_target() helper, which keeps nb_strm up to date and also clears sv_tgcounters when the target is no longer a server, so we don't keep a dangling reference to the server's counters (its failure counters have already been accounted for by that point). To check if there are still any streams attached to the server, the code just looks at all the per-thread-group counters, and if one of them is non-zero, there are still streams on that server. This will always be accurate because the server deletion command runs with thread isolation. This should be backported up to 3.0. --- include/haproxy/server-t.h | 1 + include/haproxy/stream.h | 29 +++++++++++++++++++++++++++-- src/backend.c | 4 ++-- src/cache.c | 3 +++ src/cli.c | 14 +++----------- src/http_ana.c | 1 + src/http_client.c | 10 +--------- src/server.c | 8 +++++++- src/stream.c | 4 +++- 9 files changed, 48 insertions(+), 26 deletions(-) diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h index c2bd6c7db..75e1cf603 100644 --- a/include/haproxy/server-t.h +++ b/include/haproxy/server-t.h @@ -296,6 +296,7 @@ struct srv_per_tgroup { struct server *next_full; /* next server in the temporary full list */ unsigned int last_other_tgrp_served; /* Last other tgrp we dequeued from */ unsigned int self_served; /* Number of connection we dequeued from our own queue */ + unsigned int nb_strm; /* nb streams in this tgroup referencing this server */ unsigned int dequeuing; /* non-zero = dequeuing in progress (atomic) */ unsigned int next_takeover; /* thread ID to try to steal connections from next time */ struct eb_root *lb_tree; /* For LB algos with split between thread groups, the tree to be used, for each group */ diff --git a/include/haproxy/stream.h b/include/haproxy/stream.h index fe66d6cb4..af5d2f45f 100644 --- a/include/haproxy/stream.h +++ b/include/haproxy/stream.h @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -432,10 +433,34 @@ static inline void stream_report_term_evt(struct stconn *sc, enum strm_term_even sc->term_evts_log = tevt_report_event(sc->term_evts_log, loc, type); } +/* + * Sets the stream's target, and take care of nb_strm and sv_tgcounters if + * the target is a server. + */ +static inline void stream_set_target(struct stream *s, enum obj_type *target) +{ + struct server *o = objt_server(s->target); + struct server *n = objt_server(target); + + if (o != n) { + if (n) { + _HA_ATOMIC_INC(&n->per_tgrp[tgid - 1].nb_strm); + s->sv_tgcounters = n->counters.shared.tg ? n->counters.shared.tg[tgid - 1] : NULL; + } + else + s->sv_tgcounters = NULL; + if (o) + _HA_ATOMIC_DEC(&o->per_tgrp[tgid - 1].nb_strm); + } + s->target = target; +} + +/* Convenience wrapper of stream_set_target() for callers that already hold a + * struct server, so they can pass it directly. + */ static inline void stream_set_srv_target(struct stream *s, struct server *srv) { - s->target = &srv->obj_type; - s->sv_tgcounters = srv->counters.shared.tg[tgid - 1]; + stream_set_target(s, &srv->obj_type); } int stream_set_timeout(struct stream *s, enum act_timeout_name name, int timeout); diff --git a/src/backend.c b/src/backend.c index 4001135ba..388e4ad03 100644 --- a/src/backend.c +++ b/src/backend.c @@ -668,7 +668,7 @@ int assign_server(struct stream *s) */ srv = NULL; - s->target = NULL; + stream_set_target(s, NULL); if ((s->be->lbprm.algo & BE_LB_KIND) != BE_LB_KIND_HI && ((s->sess->flags & SESS_FL_PREFER_LAST) || @@ -3041,7 +3041,7 @@ int tcp_persist_rdp_cookie(struct stream *s, struct channel *req, int an_bit) if (*p != '.') goto no_cookie; - s->target = NULL; + stream_set_target(s, NULL); while (srv) { if (srv->addr.ss_family == AF_INET && port == srv->svc_port && diff --git a/src/cache.c b/src/cache.c index 319396b00..dbee7ae7a 100644 --- a/src/cache.c +++ b/src/cache.c @@ -2725,6 +2725,9 @@ enum act_return http_action_req_cache_use(struct act_rule *rule, struct proxy *p return ACT_RET_CONT; } + /* This runs before any server assignment, so s->target holds no + * server here and there is no nb_strm reference to drop. + */ s->target = &http_cache_applet.obj_type; if ((appctx = sc_applet_create(s->scb, objt_applet(s->target)))) { struct cache_appctx *ctx = applet_reserve_svcctx(appctx, sizeof(*ctx)); diff --git a/src/cli.c b/src/cli.c index 059e256cb..4c79ecbc8 100644 --- a/src/cli.c +++ b/src/cli.c @@ -3615,15 +3615,7 @@ read_again: else target_pid = pcli->next_pid; /* we can connect now */ - s->target = pcli_pid_to_server(target_pid); - if (objt_server(s->target)) { - struct server *srv = __objt_server(s->target); - - if (srv->counters.shared.tg) - s->sv_tgcounters = srv->counters.shared.tg[tgid - 1]; - else - s->sv_tgcounters = NULL; - } + stream_set_target(s, pcli_pid_to_server(target_pid)); if (!s->target) goto server_disconnect; @@ -3788,7 +3780,7 @@ int pcli_wait_for_response(struct stream *s, struct channel *rep, int an_bit) process_srv_queue(__objt_server(s->target)); } - s->target = NULL; + stream_set_target(s, NULL); /* Always release our endpoint */ s->srv_conn = NULL; @@ -3827,7 +3819,7 @@ int pcli_wait_for_response(struct stream *s, struct channel *rep, int an_bit) s->logs.logwait = strm_fe(s)->to_log; s->logs.level = 0; stream_del_srv_conn(s); - s->target = NULL; + stream_set_target(s, NULL); /* re-init store persistence */ s->store_count = 0; s->uniq_id = _HA_ATOMIC_FETCH_ADD(&global.req_count, 1); diff --git a/src/http_ana.c b/src/http_ana.c index 019adaece..75369fe18 100644 --- a/src/http_ana.c +++ b/src/http_ana.c @@ -472,6 +472,7 @@ int http_process_req_common(struct stream *s, struct channel *req, int an_bit, s * reqdeny can still block them. This clearly needs to change in 1.6! */ if (!s->target && http_stats_check_uri(s, txn, px)) { + /* s->target was NULL (checked above), no nb_strm reference to drop */ s->target = &http_stats_applet.obj_type; if (unlikely(!sc_applet_create(s->scb, objt_applet(s->target)))) { s->logs.request_ts = now_ns; diff --git a/src/http_client.c b/src/http_client.c index 3bdbb8080..a4c96e857 100644 --- a/src/http_client.c +++ b/src/http_client.c @@ -958,15 +958,7 @@ int httpclient_applet_init(struct appctx *appctx) } s = appctx_strm(appctx); - s->target = target; - if (objt_server(s->target)) { - struct server *srv = __objt_server(s->target); - - if (srv->counters.shared.tg) - s->sv_tgcounters = __objt_server(s->target)->counters.shared.tg[tgid - 1]; - else - s->sv_tgcounters = NULL; - } + stream_set_target(s, target); /* set the "timeout server" */ s->scb->ioto = hc->timeout_server; diff --git a/src/server.c b/src/server.c index 15dd282ac..4bdcb8be0 100644 --- a/src/server.c +++ b/src/server.c @@ -2095,7 +2095,13 @@ static int srv_parse_strict_maxconn(char **args, int *cur_arg, struct proxy *px, /* Returns 1 if the server has streams pointing to it, and 0 otherwise. */ static int srv_has_streams(struct server *srv) { - return !!_HA_ATOMIC_LOAD(&srv->served); + int i; + + for (i = 0; i < global.nbtgroups; i++) { + if (_HA_ATOMIC_LOAD(&srv->per_tgrp[i].nb_strm)) + return 1; + } + return 0; } /* Shutdown all connections of a server. The caller must pass a termination diff --git a/src/stream.c b/src/stream.c index 862f5576b..76414565d 100644 --- a/src/stream.c +++ b/src/stream.c @@ -770,6 +770,8 @@ void stream_free(struct stream *s) } } + stream_set_target(s, NULL); + pool_free(pool_head_stream, s); /* We may want to free the maximum amount of pools if the proxy is stopping */ @@ -1115,7 +1117,7 @@ enum act_return process_use_service(struct act_rule *rule, struct proxy *px, /* Initialises the applet if it is required. */ if (flags & ACT_OPT_FIRST) { /* Register applet. this function schedules the applet. */ - s->target = &rule->applet.obj_type; + stream_set_target(s, &rule->applet.obj_type); appctx = sc_applet_create(s->scb, objt_applet(s->target)); if (unlikely(!appctx)) return ACT_RET_ERR; -- 2.47.3