From 79551d02351a54da0bebf37f58c7b80b135348d0 Mon Sep 17 00:00:00 2001 From: Alexander Stephan Date: Fri, 3 Jul 2026 08:35:47 +0000 Subject: [PATCH 1/5] BUG/MEDIUM: counters: preserve shared.tg pointer on 'clear counters all' proxy_stats_clear_counters() did a blanket memset() on the be_counters / fe_counters / listener counter structs when clearing all counters. This zeroed the embedded shared.tg pointer array, which every hot path dereferences with no NULL check (for example _HA_ATOMIC_INC(&srv->counters.shared.tg[tgid-1]->cum_lbconn) in process_srv_queue()). After 'clear counters all' the next request on that server would segfault. The memset pattern predates the switch to per-thread-group shared counters (commit 5495c8844 "MEDIUM: counters: Dynamically allocate per-thread group counters") and wasn't updated when shared.tg gained pointer-array semantics. Additionally, memset'ing only the outer struct never actually reset the accumulated counters in the pointed-at per-tgroup structs, so 'clear counters all' silently failed to match its documented "same effect as restarting" behaviour for cumulative counters (bytes, sessions, requests, ...). Introduce two helpers in src/counters.c: void counters_fe_reset(struct fe_counters *counters); void counters_be_reset(struct be_counters *counters); Both iterate shared.tg[0 .. nbtgroups-1] and zero the *contents* of each per-tgroup struct via memset, then zero the local (non-shared) fields of the outer struct. The shared.tg pointer array, each shared.tg[it] pointer, and shared.flags (COUNTERS_SHARED_F_LOCAL is set at boot and reflects allocation ownership, not counter state) are preserved. Refactor proxy_stats_clear_counters() to call these helpers instead of the inline memset. This fixes both the segfault after 'clear counters all' and the silent no-op on cumulative counters, and provides a shared primitive for a subsequent 'clear counters server /' command. In SHM stats-file mode, zeroing the per-tgroup structs affects every process attached to the same object; this matches the intended "reset everywhere" semantic of clear counters and is unchanged from the outer-memset intent. This bug should be backported wherever the per-thread-group shared counter refactor is present. --- include/haproxy/counters.h | 9 +++ .../stats/clear_counters_all_survives.vtc | 75 +++++++++++++++++++ src/counters.c | 60 +++++++++++++++ src/stats-proxy.c | 8 +- 4 files changed, 148 insertions(+), 4 deletions(-) create mode 100644 reg-tests/stats/clear_counters_all_survives.vtc diff --git a/include/haproxy/counters.h b/include/haproxy/counters.h index 599b732ff..1a8391bd0 100644 --- a/include/haproxy/counters.h +++ b/include/haproxy/counters.h @@ -36,6 +36,15 @@ int counters_be_shared_prepare(struct be_counters_shared *counters, const struct void counters_fe_shared_drop(struct fe_counters_shared *counters); void counters_be_shared_drop(struct be_counters_shared *counters); +/* Safe counter reset: zero all counter fields while preserving the shared.tg + * pointer array and per-tgroup allocations (which the hot path dereferences + * without a NULL check). The shared.flags field is also preserved because + * COUNTERS_SHARED_F_LOCAL is set once at init and reflects allocation + * ownership, not counter state. + */ +void counters_fe_reset(struct fe_counters *counters); +void counters_be_reset(struct be_counters *counters); + /* time oriented helper: get last time (relative to current time) on a given * array, for member (one member per thread group) which is * assumed to be unsigned long type. diff --git a/reg-tests/stats/clear_counters_all_survives.vtc b/reg-tests/stats/clear_counters_all_survives.vtc new file mode 100644 index 000000000..bba3c7c52 --- /dev/null +++ b/reg-tests/stats/clear_counters_all_survives.vtc @@ -0,0 +1,75 @@ +varnishtest "'clear counters all' preserves the shared.tg pointer" + +# Regression test for a segfault triggered by 'clear counters all' after the +# switch to per-thread-group shared counters. A blanket memset on the outer +# counters struct zeroed sv->counters.shared.tg, a pointer that every hot +# path dereferences with no NULL check, so the next request after the clear +# would crash. With counters_be_reset() / counters_fe_reset() the pointer +# is preserved and only the pointed-at counter contents are cleared. +# +# This test drives traffic, clears counters, then drives more traffic. If +# the fix regresses, the second traffic burst crashes the HAProxy process +# and vtest reports the connection as failed. + +feature ignore_unknown_macro + +#REGTEST_TYPE=devel + +server s1 { + rxreq + txresp +} -start + +server s1_more { + rxreq + txresp +} -start + +haproxy h1 -conf { + global + .if feature(THREAD) + thread-groups 1 + .endif + + defaults + mode http + timeout connect "${HAPROXY_TEST_TIMEOUT-5s}" + timeout client "${HAPROXY_TEST_TIMEOUT-5s}" + timeout server "${HAPROXY_TEST_TIMEOUT-5s}" + + frontend fe + bind "fd@${feS}" + default_backend be + + backend be + server s1 ${s1_addr}:${s1_port} +} -start + +# First burst of traffic to accumulate counters on the server. +client c1 -connect ${h1_feS_sock} { + txreq + rxresp + expect resp.status == 200 +} -run + +# Clear all counters via the CLI. Before the fix, this would null +# shared.tg and cause the *next* request to crash the process. +haproxy h1 -cli { + send "clear counters all" + expect ~ ".*" +} + +# Rebind backend to a fresh varnishtest server since the first one +# consumed its single rxreq/txresp. +haproxy h1 -cli { + send "set server be/s1 addr ${s1_more_addr} port ${s1_more_port}" + expect ~ ".*" +} + +# Second burst of traffic. If the process crashed on the counter clear, +# the connection is refused and vtest fails. +client c2 -connect ${h1_feS_sock} { + txreq + rxresp + expect resp.status == 200 +} -run diff --git a/src/counters.c b/src/counters.c index 7e75154ec..1572cf851 100644 --- a/src/counters.c +++ b/src/counters.c @@ -149,3 +149,63 @@ int counters_be_shared_prepare(struct be_counters_shared *shared, const struct g { return _counters_shared_prepare((struct counters_shared *)shared, guid, 1, errmsg); } + +/* Reset the contents of the per-thread-group shared counters attached to + * , while preserving the shared.tg pointer array and the per-tgroup + * allocations themselves. This is the safe way to reset counters at runtime: + * the hot path dereferences shared.tg[tgid-1] with no NULL check, so nulling + * that pointer (as a blanket memset on the outer struct would do) segfaults on + * the next request. The shared.flags field is also preserved because + * COUNTERS_SHARED_F_LOCAL is set once at init and reflects allocation + * ownership, not counter state. + * + * In shared-memory (SHM stats file) mode, zeroing the per-tgroup structs + * affects every process attached to the same object. This is the intended + * "reset everywhere" semantic of clear counters. + */ +static void _counters_shared_reset(struct counters_shared *shared, size_t tg_size) +{ + int it; + + if (!shared || !shared->tg) + return; + + for (it = 0; it < global.nbtgroups; it++) { + if (shared->tg[it]) + memset(shared->tg[it], 0, tg_size); + } +} + +/* Reset all counter fields of while preserving the shared.tg + * pointer array and the per-tgroup allocations. See _counters_shared_reset() + * for the pointer-lifetime rationale. Both the per-tgroup accumulated + * counters (bytes, sessions, requests, errors, response codes, ...) and the + * local max/rate counters are cleared. + */ +void counters_fe_reset(struct fe_counters *counters) +{ + if (!counters) + return; + + _counters_shared_reset((struct counters_shared *)&counters->shared, + sizeof(struct fe_counters_shared_tg)); + + /* Zero all local (non-shared) fields. The shared sub-struct is the + * first member; skip it and clear the rest. + */ + memset((char *)counters + sizeof(counters->shared), 0, + sizeof(*counters) - sizeof(counters->shared)); +} + +/* Reset all counter fields of . See counters_fe_reset(). */ +void counters_be_reset(struct be_counters *counters) +{ + if (!counters) + return; + + _counters_shared_reset((struct counters_shared *)&counters->shared, + sizeof(struct be_counters_shared_tg)); + + memset((char *)counters + sizeof(counters->shared), 0, + sizeof(*counters) - sizeof(counters->shared)); +} diff --git a/src/stats-proxy.c b/src/stats-proxy.c index a4eda63f6..d938f1c1d 100644 --- a/src/stats-proxy.c +++ b/src/stats-proxy.c @@ -1652,8 +1652,8 @@ void proxy_stats_clear_counters(int clrall, struct list *stat_modules) for (px = proxies_list; px; px = px->next) { if (clrall) { - memset(&px->be_counters, 0, sizeof(px->be_counters)); - memset(&px->fe_counters, 0, sizeof(px->fe_counters)); + counters_be_reset(&px->be_counters); + counters_fe_reset(&px->fe_counters); } else { px->be_counters.conn_max = 0; @@ -1674,7 +1674,7 @@ void proxy_stats_clear_counters(int clrall, struct list *stat_modules) for (sv = px->srv; sv; sv = sv->next) if (clrall) - memset(&sv->counters, 0, sizeof(sv->counters)); + counters_be_reset(&sv->counters); else { sv->counters.cur_sess_max = 0; sv->counters.nbpend_max = 0; @@ -1688,7 +1688,7 @@ void proxy_stats_clear_counters(int clrall, struct list *stat_modules) list_for_each_entry(li, &px->conf.listeners, by_fe) if (li->counters) { if (clrall) - memset(li->counters, 0, sizeof(*li->counters)); + counters_fe_reset(li->counters); else li->counters->conn_max = 0; } -- 2.43.7