From: Alexander Stephan Date: Fri, 3 Jul 2026 08:35:47 +0000 (+0000) Subject: BUG/MEDIUM: counters: preserve shared.tg pointer on 'clear counters all' X-Git-Url: http://git.ipfire.org/gitweb/?a=commitdiff_plain;h=219b80d79e9519ff3de34c25233c2b2264df8f6d;p=thirdparty%2Fhaproxy.git 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. --- diff --git a/include/haproxy/counters-t.h b/include/haproxy/counters-t.h index b606428a8..85dacb3d2 100644 --- a/include/haproxy/counters-t.h +++ b/include/haproxy/counters-t.h @@ -107,6 +107,11 @@ struct fe_counters_shared { /* counters used by listeners and frontends */ struct fe_counters { + /* shared must stay the first member: counters_fe_reset() zeroes + * everything after it in one memset, so any new field added below is + * reset automatically. It holds the shared.tg pointer array which the + * hot path dereferences without a NULL check and must never be zeroed. + */ struct fe_counters_shared shared; /* shared counters */ unsigned int conn_max; /* max # of active sessions */ @@ -167,6 +172,11 @@ struct be_counters_shared { /* counters used by servers and backends */ struct be_counters { + /* shared must stay the first member: counters_be_reset() zeroes + * everything after it in one memset, so any new field added below is + * reset automatically. It holds the shared.tg pointer array which the + * hot path dereferences without a NULL check and must never be zeroed. + */ struct be_counters_shared shared; /* shared counters */ unsigned int conn_max; /* max # of active sessions */ 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..5473c5471 100644 --- a/src/counters.c +++ b/src/counters.c @@ -149,3 +149,68 @@ 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. Note this deliberately is NOT a + * plain memset() over the whole struct: shared is the first member and + * holds the shared.tg pointer array that the hot path dereferences + * without a NULL check, so blanket-zeroing it would segfault the next + * request. We skip exactly the shared sub-struct and clear the rest; + * every field after it is numeric, so any field added later is reset + * automatically without touching this code. + */ + 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 51bd17a33..edc809885 100644 --- a/src/stats-proxy.c +++ b/src/stats-proxy.c @@ -1669,8 +1669,8 @@ void proxy_stats_clear_counters(int clrall, struct list *stat_modules) list_for_each_entry(px, &main_proxies, el) { 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; @@ -1691,7 +1691,7 @@ void proxy_stats_clear_counters(int clrall, struct list *stat_modules) list_for_each_entry(sv, &px->servers, el_px) { 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; @@ -1706,7 +1706,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; }