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 <b>/<s>' 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.
/* 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 */
/* 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 */
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
* <scounter> array, for <elem> member (one member per thread group) which is
* assumed to be unsigned long type.
--- /dev/null
+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
{
return _counters_shared_prepare((struct counters_shared *)shared, guid, 1, errmsg);
}
+
+/* Reset the contents of the per-thread-group shared counters attached to
+ * <shared>, 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 <counters> 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 <counters>. 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));
+}
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;
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;
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;
}
projects / thirdparty / haproxy.git / commitdiff
