MEDIUM: stats: consider that shared stats pointers may be NULL · haproxy/haproxy@5c299de · GitHub
Skip to content

Commit 5c299de

Browse files
DarleletDarlelet
committed
MEDIUM: stats: consider that shared stats pointers may be NULL
This patch looks huge, but it has a very simple goal: protect all accessed to shared stats pointers (either read or writes), because we know consider that these pointers may be NULL. The reason behind this is despite all precautions taken to ensure the pointers shouldn't be NULL when not expected, there are still corner cases (ie: frontends stats used on a backend which no FE cap and vice versa) where we could try to access a memory area which is not allocated. Willy stumbled on such cases while playing with the rings servers upon connection error, which eventually led to process crashes (since 3.3 when shared stats were implemented) Also, we may decide later that shared stats are optional and should be disabled on the proxy to save memory and CPU, and this patch is a step further towards that goal. So in essence, this patch ensures shared stats pointers are always initialized (including NULL), and adds necessary guards before shared stats pointers are de-referenced. Since we already had some checks for backends and listeners stats, and the pointer address retrieval should stay in cpu cache, let's hope that this patch doesn't impact stats performance much.
1 parent 40eb1dd commit 5c299de

23 files changed

Lines changed: 478 additions & 296 deletions

‎include/haproxy/backend.h‎

Lines changed: 2 additions & 1 deletion

‎include/haproxy/counters.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ void counters_be_shared_drop(struct be_counters_shared *counters);
4747
unsigned long now_seconds = ns_to_sec(now_ns); \
4848
int it; \
4949
\
50-
for (it = 1; it < global.nbtgroups; it++) { \
50+
for (it = 1; (it < global.nbtgroups && scounters[it]); it++) { \
5151
unsigned long cur = HA_ATOMIC_LOAD((type *)((char *)scounters[it] + offset));\
5252
if ((now_seconds - cur) < (now_seconds - last)) \
5353
last = cur; \
@@ -74,7 +74,7 @@ void counters_be_shared_drop(struct be_counters_shared *counters);
7474
uint64_t __ret = 0; \
7575
int it; \
7676
\
77-
for (it = 0; it < global.nbtgroups; it++) \
77+
for (it = 0; (it < global.nbtgroups && scounters[it]); it++) \
7878
__ret += rfunc((type *)((char *)scounters[it] + offset)); \
7979
__ret; \
8080
})
@@ -94,7 +94,7 @@ void counters_be_shared_drop(struct be_counters_shared *counters);
9494
uint64_t __ret = 0; \
9595
int it; \
9696
\
97-
for (it = 0; it < global.nbtgroups; it++) \
97+
for (it = 0; (it < global.nbtgroups && scounters[it]); it++) \
9898
__ret += rfunc(&scounters[it]->elem, arg1, arg2); \
9999
__ret; \
100100
})

‎include/haproxy/proxy.h‎

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -144,22 +144,25 @@ static inline void proxy_reset_timeouts(struct proxy *proxy)
144144
/* increase the number of cumulated connections received on the designated frontend */
145145
static inline void proxy_inc_fe_conn_ctr(struct listener *l, struct proxy *fe)
146146
{
147-
_HA_ATOMIC_INC(&fe->fe_counters.shared.tg[tgid - 1]->cum_conn);
148-
if (l && l->counters)
147+
if (fe->fe_counters.shared.tg[tgid - 1])
148+
_HA_ATOMIC_INC(&fe->fe_counters.shared.tg[tgid - 1]->cum_conn);
149+
if (l && l->counters && l->counters->shared.tg[tgid - 1])
149150
_HA_ATOMIC_INC(&l->counters->shared.tg[tgid - 1]->cum_conn);
150-
update_freq_ctr(&fe->fe_counters.shared.tg[tgid - 1]->conn_per_sec, 1);
151+
if (fe->fe_counters.shared.tg[tgid - 1])
152+
update_freq_ctr(&fe->fe_counters.shared.tg[tgid - 1]->conn_per_sec, 1);
151153
HA_ATOMIC_UPDATE_MAX(&fe->fe_counters.cps_max,
152154
update_freq_ctr(&fe->fe_counters._conn_per_sec, 1));
153155
}
154156

155157
/* increase the number of cumulated connections accepted by the designated frontend */
156158
static inline void proxy_inc_fe_sess_ctr(struct listener *l, struct proxy *fe)
157159
{
158-
159-
_HA_ATOMIC_INC(&fe->fe_counters.shared.tg[tgid - 1]->cum_sess);
160-
if (l && l->counters)
160+
if (fe->fe_counters.shared.tg[tgid - 1])
161+
_HA_ATOMIC_INC(&fe->fe_counters.shared.tg[tgid - 1]->cum_sess);
162+
if (l && l->counters && l->counters->shared.tg[tgid - 1])
161163
_HA_ATOMIC_INC(&l->counters->shared.tg[tgid - 1]->cum_sess);
162-
update_freq_ctr(&fe->fe_counters.shared.tg[tgid - 1]->sess_per_sec, 1);
164+
if (fe->fe_counters.shared.tg[tgid - 1])
165+
update_freq_ctr(&fe->fe_counters.shared.tg[tgid - 1]->sess_per_sec, 1);
163166
HA_ATOMIC_UPDATE_MAX(&fe->fe_counters.sps_max,
164167
update_freq_ctr(&fe->fe_counters._sess_per_sec, 1));
165168
}
@@ -174,16 +177,19 @@ static inline void proxy_inc_fe_cum_sess_ver_ctr(struct listener *l, struct prox
174177
http_ver > sizeof(fe->fe_counters.shared.tg[tgid - 1]->cum_sess_ver) / sizeof(*fe->fe_counters.shared.tg[tgid - 1]->cum_sess_ver))
175178
return;
176179

177-
_HA_ATOMIC_INC(&fe->fe_counters.shared.tg[tgid - 1]->cum_sess_ver[http_ver - 1]);
178-
if (l && l->counters)
180+
if (fe->fe_counters.shared.tg[tgid - 1])
181+
_HA_ATOMIC_INC(&fe->fe_counters.shared.tg[tgid - 1]->cum_sess_ver[http_ver - 1]);
182+
if (l && l->counters && l->counters->shared.tg[tgid - 1])
179183
_HA_ATOMIC_INC(&l->counters->shared.tg[tgid - 1]->cum_sess_ver[http_ver - 1]);
180184
}
181185

182186
/* increase the number of cumulated streams on the designated backend */
183187
static inline void proxy_inc_be_ctr(struct proxy *be)
184188
{
185-
_HA_ATOMIC_INC(&be->be_counters.shared.tg[tgid - 1]->cum_sess);
186-
update_freq_ctr(&be->be_counters.shared.tg[tgid - 1]->sess_per_sec, 1);
189+
if (be->be_counters.shared.tg[tgid - 1])
190+
_HA_ATOMIC_INC(&be->be_counters.shared.tg[tgid - 1]->cum_sess);
191+
if (be->be_counters.shared.tg[tgid - 1])
192+
update_freq_ctr(&be->be_counters.shared.tg[tgid - 1]->sess_per_sec, 1);
187193
HA_ATOMIC_UPDATE_MAX(&be->be_counters.sps_max,
188194
update_freq_ctr(&be->be_counters._sess_per_sec, 1));
189195
}
@@ -198,10 +204,12 @@ static inline void proxy_inc_fe_req_ctr(struct listener *l, struct proxy *fe,
198204
if (http_ver >= sizeof(fe->fe_counters.shared.tg[tgid - 1]->p.http.cum_req) / sizeof(*fe->fe_counters.shared.tg[tgid - 1]->p.http.cum_req))
199205
return;
200206

201-
_HA_ATOMIC_INC(&fe->fe_counters.shared.tg[tgid - 1]->p.http.cum_req[http_ver]);
202-
if (l && l->counters)
207+
if (fe->fe_counters.shared.tg[tgid - 1])
208+
_HA_ATOMIC_INC(&fe->fe_counters.shared.tg[tgid - 1]->p.http.cum_req[http_ver]);
209+
if (l && l->counters && l->counters->shared.tg[tgid - 1])
203210
_HA_ATOMIC_INC(&l->counters->shared.tg[tgid - 1]->p.http.cum_req[http_ver]);
204-
update_freq_ctr(&fe->fe_counters.shared.tg[tgid - 1]->req_per_sec, 1);
211+
if (fe->fe_counters.shared.tg[tgid - 1])
212+
update_freq_ctr(&fe->fe_counters.shared.tg[tgid - 1]->req_per_sec, 1);
205213
HA_ATOMIC_UPDATE_MAX(&fe->fe_counters.p.http.rps_max,
206214
update_freq_ctr(&fe->fe_counters.p.http._req_per_sec, 1));
207215
}

‎include/haproxy/server.h‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -207,16 +207,19 @@ static inline void server_index_id(struct proxy *px, struct server *srv)
207207
/* increase the number of cumulated streams on the designated server */
208208
static inline void srv_inc_sess_ctr(struct server *s)
209209
{
210-
_HA_ATOMIC_INC(&s->counters.shared.tg[tgid - 1]->cum_sess);
211-
update_freq_ctr(&s->counters.shared.tg[tgid - 1]->sess_per_sec, 1);
210+
if (s->counters.shared.tg[tgid - 1]) {
211+
_HA_ATOMIC_INC(&s->counters.shared.tg[tgid - 1]->cum_sess);
212+
update_freq_ctr(&s->counters.shared.tg[tgid - 1]->sess_per_sec, 1);
213+
}
212214
HA_ATOMIC_UPDATE_MAX(&s->counters.sps_max,
213215
update_freq_ctr(&s->counters._sess_per_sec, 1));
214216
}
215217

216218
/* set the time of last session on the designated server */
217219
static inline void srv_set_sess_last(struct server *s)
218220
{
219-
HA_ATOMIC_STORE(&s->counters.shared.tg[tgid - 1]->last_sess, ns_to_sec(now_ns));
221+
if (s->counters.shared.tg[tgid - 1])
222+
HA_ATOMIC_STORE(&s->counters.shared.tg[tgid - 1]->last_sess, ns_to_sec(now_ns));
220223
}
221224

222225
/* returns the current server throttle rate between 0 and 100% */

‎include/haproxy/stream.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -361,9 +361,10 @@ static inline void stream_choose_redispatch(struct stream *s)
361361
s->flags &= ~(SF_DIRECT | SF_ASSIGNED);
362362
s->scb->state = SC_ST_REQ;
363363
} else {
364-
if (objt_server(s->target))
364+
if (s->sv_tgcounters)
365365
_HA_ATOMIC_INC(&s->sv_tgcounters->retries);
366-
_HA_ATOMIC_INC(&s->be_tgcounters->retries);
366+
if (s->be_tgcounters)
367+
_HA_ATOMIC_INC(&s->be_tgcounters->retries);
367368
s->scb->state = SC_ST_ASS;
368369
}
369370

‎src/backend.c‎

Lines changed: 39 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -825,8 +825,10 @@ int assign_server(struct stream *s)
825825
goto out;
826826
}
827827
else if (srv != prev_srv) {
828-
_HA_ATOMIC_INC(&s->be_tgcounters->cum_lbconn);
829-
_HA_ATOMIC_INC(&srv->counters.shared.tg[tgid - 1]->cum_lbconn);
828+
if (s->be_tgcounters)
829+
_HA_ATOMIC_INC(&s->be_tgcounters->cum_lbconn);
830+
if (srv->counters.shared.tg[tgid - 1])
831+
_HA_ATOMIC_INC(&srv->counters.shared.tg[tgid - 1]->cum_lbconn);
830832
}
831833
stream_set_srv_target(s, srv);
832834
}
@@ -1000,11 +1002,15 @@ int assign_server_and_queue(struct stream *s)
10001002
s->txn->flags |= TX_CK_DOWN;
10011003
}
10021004
s->flags |= SF_REDISP;
1003-
_HA_ATOMIC_INC(&prev_srv->counters.shared.tg[tgid - 1]->redispatches);
1004-
_HA_ATOMIC_INC(&s->be_tgcounters->redispatches);
1005+
if (prev_srv->counters.shared.tg[tgid - 1])
1006+
_HA_ATOMIC_INC(&prev_srv->counters.shared.tg[tgid - 1]->redispatches);
1007+
if (s->be_tgcounters)
1008+
_HA_ATOMIC_INC(&s->be_tgcounters->redispatches);
10051009
} else {
1006-
_HA_ATOMIC_INC(&prev_srv->counters.shared.tg[tgid - 1]->retries);
1007-
_HA_ATOMIC_INC(&s->be_tgcounters->retries);
1010+
if (prev_srv->counters.shared.tg[tgid - 1])
1011+
_HA_ATOMIC_INC(&prev_srv->counters.shared.tg[tgid - 1]->retries);
1012+
if (s->be_tgcounters)
1013+
_HA_ATOMIC_INC(&s->be_tgcounters->retries);
10081014
}
10091015
}
10101016
}
@@ -2119,12 +2125,14 @@ int connect_server(struct stream *s)
21192125
s->scb->flags |= SC_FL_NOLINGER;
21202126

21212127
if (s->flags & SF_SRV_REUSED) {
2122-
_HA_ATOMIC_INC(&s->be_tgcounters->reuse);
2123-
if (srv)
2128+
if (s->be_tgcounters)
2129+
_HA_ATOMIC_INC(&s->be_tgcounters->reuse);
2130+
if (s->sv_tgcounters)
21242131
_HA_ATOMIC_INC(&s->sv_tgcounters->reuse);
21252132
} else {
2126-
_HA_ATOMIC_INC(&s->be_tgcounters->connect);
2127-
if (srv)
2133+
if (s->be_tgcounters)
2134+
_HA_ATOMIC_INC(&s->be_tgcounters->connect);
2135+
if (s->sv_tgcounters)
21282136
_HA_ATOMIC_INC(&s->sv_tgcounters->connect);
21292137
}
21302138

@@ -2316,8 +2324,10 @@ int srv_redispatch_connect(struct stream *s)
23162324
s->conn_err_type = STRM_ET_QUEUE_ERR;
23172325
}
23182326

2319-
_HA_ATOMIC_INC(&s->sv_tgcounters->failed_conns);
2320-
_HA_ATOMIC_INC(&s->be_tgcounters->failed_conns);
2327+
if (s->sv_tgcounters)
2328+
_HA_ATOMIC_INC(&s->sv_tgcounters->failed_conns);
2329+
if (s->be_tgcounters)
2330+
_HA_ATOMIC_INC(&s->be_tgcounters->failed_conns);
23212331
return 1;
23222332

23232333
case SRV_STATUS_NOSRV:
@@ -2326,7 +2336,8 @@ int srv_redispatch_connect(struct stream *s)
23262336
s->conn_err_type = STRM_ET_CONN_ERR;
23272337
}
23282338

2329-
_HA_ATOMIC_INC(&s->be_tgcounters->failed_conns);
2339+
if (s->be_tgcounters)
2340+
_HA_ATOMIC_INC(&s->be_tgcounters->failed_conns);
23302341
return 1;
23312342

23322343
case SRV_STATUS_QUEUED:
@@ -2354,9 +2365,10 @@ int srv_redispatch_connect(struct stream *s)
23542365
srv_inc_sess_ctr(srv);
23552366
if (srv)
23562367
srv_set_sess_last(srv);
2357-
if (srv)
2368+
if (s->sv_tgcounters)
23582369
_HA_ATOMIC_INC(&s->sv_tgcounters->failed_conns);
2359-
_HA_ATOMIC_INC(&s->be_tgcounters->failed_conns);
2370+
if (s->be_tgcounters)
2371+
_HA_ATOMIC_INC(&s->be_tgcounters->failed_conns);
23602372

23612373
/* release other streams waiting for this server */
23622374
if (may_dequeue_tasks(srv, s->be))
@@ -2429,9 +2441,10 @@ void back_try_conn_req(struct stream *s)
24292441
srv_inc_sess_ctr(srv);
24302442
if (srv)
24312443
srv_set_sess_last(srv);
2432-
if (srv)
2444+
if (s->sv_tgcounters)
24332445
_HA_ATOMIC_INC(&s->sv_tgcounters->failed_conns);
2434-
_HA_ATOMIC_INC(&s->be_tgcounters->failed_conns);
2446+
if (s->be_tgcounters)
2447+
_HA_ATOMIC_INC(&s->be_tgcounters->failed_conns);
24352448

24362449
/* release other streams waiting for this server */
24372450
sess_change_server(s, NULL);
@@ -2496,9 +2509,10 @@ void back_try_conn_req(struct stream *s)
24962509
/* we may need to know the position in the queue for logging */
24972510
pendconn_cond_unlink(s->pend_pos);
24982511

2499-
if (srv)
2512+
if (s->sv_tgcounters)
25002513
_HA_ATOMIC_INC(&s->sv_tgcounters->failed_conns);
2501-
_HA_ATOMIC_INC(&s->be_tgcounters->failed_conns);
2514+
if (s->be_tgcounters)
2515+
_HA_ATOMIC_INC(&s->be_tgcounters->failed_conns);
25022516
sc_abort(sc);
25032517
sc_shutdown(sc);
25042518
req->flags |= CF_WRITE_TIMEOUT;
@@ -2752,9 +2766,10 @@ void back_handle_st_cer(struct stream *s)
27522766
s->conn_err_type = STRM_ET_CONN_ERR;
27532767
}
27542768

2755-
if (objt_server(s->target))
2769+
if (s->sv_tgcounters)
27562770
_HA_ATOMIC_INC(&s->sv_tgcounters->failed_conns);
2757-
_HA_ATOMIC_INC(&s->be_tgcounters->failed_conns);
2771+
if (s->be_tgcounters)
2772+
_HA_ATOMIC_INC(&s->be_tgcounters->failed_conns);
27582773
sess_change_server(s, NULL);
27592774
if (may_dequeue_tasks(objt_server(s->target), s->be))
27602775
process_srv_queue(objt_server(s->target));
@@ -2785,9 +2800,10 @@ void back_handle_st_cer(struct stream *s)
27852800
if (!s->conn_err_type)
27862801
s->conn_err_type = STRM_ET_CONN_OTHER;
27872802

2788-
if (objt_server(s->target))
2803+
if (s->sv_tgcounters)
27892804
_HA_ATOMIC_INC(&s->sv_tgcounters->internal_errors);
2790-
_HA_ATOMIC_INC(&s->be_tgcounters->internal_errors);
2805+
if (s->be_tgcounters)
2806+
_HA_ATOMIC_INC(&s->be_tgcounters->internal_errors);
27912807
sess_change_server(s, NULL);
27922808
if (may_dequeue_tasks(objt_server(s->target), s->be))
27932809
process_srv_queue(objt_server(s->target));

‎src/cache.c‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2132,10 +2132,14 @@ enum act_return http_action_req_cache_use(struct act_rule *rule, struct proxy *p
21322132
if (s->txn->flags & TX_CACHE_IGNORE)
21332133
return ACT_RET_CONT;
21342134

2135-
if (px == strm_fe(s))
2136-
_HA_ATOMIC_INC(&px->fe_counters.shared.tg[tgid - 1]->p.http.cache_lookups);
2137-
else
2138-
_HA_ATOMIC_INC(&px->be_counters.shared.tg[tgid - 1]->p.http.cache_lookups);
2135+
if (px == strm_fe(s)) {
2136+
if (px->fe_counters.shared.tg[tgid - 1])
2137+
_HA_ATOMIC_INC(&px->fe_counters.shared.tg[tgid - 1]->p.http.cache_lookups);
2138+
}
2139+
else {
2140+
if (px->be_counters.shared.tg[tgid - 1])
2141+
_HA_ATOMIC_INC(&px->be_counters.shared.tg[tgid - 1]->p.http.cache_lookups);
2142+
}
21392143

21402144
cache_tree = get_cache_tree_from_hash(cache, read_u32(s->txn->cache_hash));
21412145

@@ -2221,10 +2225,14 @@ enum act_return http_action_req_cache_use(struct act_rule *rule, struct proxy *p
22212225
ctx->send_notmodified =
22222226
should_send_notmodified_response(cache, htxbuf(&s->req.buf), res);
22232227

2224-
if (px == strm_fe(s))
2225-
_HA_ATOMIC_INC(&px->fe_counters.shared.tg[tgid - 1]->p.http.cache_hits);
2226-
else
2227-
_HA_ATOMIC_INC(&px->be_counters.shared.tg[tgid - 1]->p.http.cache_hits);
2228+
if (px == strm_fe(s)) {
2229+
if (px->fe_counters.shared.tg[tgid - 1])
2230+
_HA_ATOMIC_INC(&px->fe_counters.shared.tg[tgid - 1]->p.http.cache_hits);
2231+
}
2232+
else {
2233+
if (px->be_counters.shared.tg[tgid - 1])
2234+
_HA_ATOMIC_INC(&px->be_counters.shared.tg[tgid - 1]->p.http.cache_hits);
2235+
}
22282236
return ACT_RET_CONT;
22292237
} else {
22302238
s->target = NULL;

‎src/check.c‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -513,7 +513,8 @@ void set_server_check_status(struct check *check, short status, const char *desc
513513
if ((!(check->state & CHK_ST_AGENT) ||
514514
(check->status >= HCHK_STATUS_L57DATA)) &&
515515
(check->health > 0)) {
516-
_HA_ATOMIC_INC(&s->counters.shared.tg[tgid - 1]->failed_checks);
516+
if (s->counters.shared.tg[tgid - 1])
517+
_HA_ATOMIC_INC(&s->counters.shared.tg[tgid - 1]->failed_checks);
517518
report = 1;
518519
check->health--;
519520
if (check->health < check->rise)
@@ -740,7 +741,8 @@ void __health_adjust(struct server *s, short status)
740741
HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
741742

742743
HA_ATOMIC_STORE(&s->consecutive_errors, 0);
743-
_HA_ATOMIC_INC(&s->counters.shared.tg[tgid - 1]->failed_hana);
744+
if (s->counters.shared.tg[tgid - 1])
745+
_HA_ATOMIC_INC(&s->counters.shared.tg[tgid - 1]->failed_hana);
744746

745747
if (s->check.fastinter) {
746748
/* timer might need to be advanced, it might also already be

‎src/fcgi-app.c‎

Lines changed: 6 additions & 4 deletions

0 commit comments

Comments
 (0)