Hi,
This is a hardening proposal, not a bug report. We are *not* claiming a crash in
stock HAProxy — our source analysis (below) concludes the current code is safe.
What we would like to discuss is whether the invariant that keeps it safe is
worth making explicit in the code, since today it is only guaranteed by the
shape
of the analyser/connect flow and a comment, and one of our out-of-tree patches
tripped over it.
We include a reference diff so the proposal is concrete, plus a lighter-weight
alternative. We'd rather have the design discussion than push a patch.
# Context: an SF_DIRECT stream holds `s->target` before `served` is incremented
The runtime deletion guard `srv_check_for_deletion()` (`src/server.c`) refuses
deletion only while the server still looks "in use" via three counters:
```c
/* src/server.c (srv_check_for_deletion) */
if (_HA_ATOMIC_LOAD(&srv->curr_used_conns) ||
_HA_ATOMIC_LOAD(&srv->queueslength) || srv_has_streams(srv)) { ... }
```
with `srv_has_streams()` being purely `!!_HA_ATOMIC_LOAD(&srv->served)`.
On the SF_DIRECT / persistence path, a stream publishes `s->target` before any
of
those three counters reflect it:
```c
/* src/stream.c ~1231 (process_server_rules, use-server rule) */
s->flags |= SF_DIRECT | SF_ASSIGNED;
s->target = &srv->obj_type;
/* src/stream.c ~1296 (sticking_rule_find_target) */
s->flags |= SF_DIRECT | SF_ASSIGNED;
s->target = &srv->obj_type;
/* src/http_ana.c ~3460 (http_manage_client_side_cookies) */
s->flags |= SF_DIRECT | SF_ASSIGNED;
s->target = &srv->obj_type;
```
At that instant the stream holds a live `struct server *` while `served`,
`curr_used_conns` and `queueslength` are all still zero for it. `served` is not
incremented until later, in `assign_server_and_queue()` (`backend.c:1057` CAS
branch or `backend.c:1083` INC branch), immediately before
`sess_change_server()`
at `backend.c:1086`.
`sess_change_server()` documents exactly this and declares it safe:
```c
/* src/stream.c:2749-2754 */
* A race condition could exist for stream which referenced a server
* instance (s->target) without registering itself in its server list.
* This is notably the case for SF_DIRECT streams which referenced a
* server earlier during process_stream(). However at this time the
* code is deemed safe as process_stream() cannot be rescheduled before
* invocation of sess_change_server().
```
# Why the comment is correct today
We traced this and agree the invariant holds in stock HAProxy. The two ways the
window could conceivably open are both closed:
1. **The analyser that sets `s->target` never yields.** All three target-setting
sites are in code that always returns 1 (or is a void helper called from such
code): `process_server_rules()` returns 1 at `stream.c:1244`,
`process_sticking_rules()` returns 1 at `stream.c:1365`, and
`http_manage_client_side_cookies()` is a void helper whose caller returns 1
after it. So the stream never sleeps *between* setting `s->target` and
reaching the connect path.
2. **A later analyser returning 0 does not defer the connect either.**
`channel_auto_connect(req)` is called at `stream.c:2014`, *before* the analyser
loop. So even if e.g. `http_request_forward_body` returns 0 (body still
pending) and breaks the analyser loop, `CF_AUTO_CONNECT` is already set, the
`SC_ST_INI→REQ` transition fires (`stream.c:2332`), and the connect do/while
(`stream.c:2364`) runs `back_handle_st_req → srv_redispatch_connect →
assign_server_and_queue` (served++) → `back_try_conn_req → connect_server` —
all within the *same* `process_stream()` pass. There is no task yield between
`s->target =` and `served++`.
So `del server` (which isolates via `thread_isolate_full()` and can only land at
a rescheduling point on other threads) cannot slip in between the target
assignment and the `served` increment: the whole span is straight-line within
one
`process_stream()` invocation.
# The concern: the invariant is implicit
The safety here is an emergent property of (a) every target-setting analyser
returning 1, and (b) `channel_auto_connect()` being ordered before the analyser
loop. Neither is asserted or enforced. A future change — a new persistence
analyser that can yield after setting `s->target`, or a reordering of the
connect
trigger — could silently reopen the window described in the
`sess_change_server()`
comment, and the failure mode is an ugly cross-thread use-after-free in
`alloc_dst_address()` (`backend.c:893`, `**ss = srv->addr`) that only shows up
under `del server` load.
Concretely, we hit this downstream: we carry an out-of-tree patch that relaxes
the
`SRV_F_NON_PURGEABLE` guard so config-declared servers can be deleted at
runtime.
With SF_DIRECT persistence in play, that let a `del server` free a server a
stream
still had in `s->target`, and a worker crashed in `alloc_dst_address()` a few
seconds later on another thread. We are *not* asking you to support that patch —
it removed a guard stock HAProxy relies on, so this is arguably our own doing.
But
it made us notice that the only thing standing between the current code and that
crash is the implicit invariant above.
# Proposal A (reference diff): pin the server with the existing refcount
The mechanism already exists. `new_server()` takes a reference on every server
(static included) via `srv_take()` (`server.c:3049`), and `del server` already
separates *detach* from *free*: `_srv_detach()` + tree removals + `srv->flags |=
SRV_F_DELETED` run under isolation, and the actual free only happens in
`srv_drop()` once the refcount reaches zero (`server.c:6421-6428`,
`server.c:3155`).
So we can hold one extra reference for exactly as long as a stream points at the
server — `srv_take()` when `s->target = &srv->obj_type`, `srv_drop()` in
`stream_free()` — and `del server` needs no change: it still succeeds and
detaches
immediately, the free just defers until the last in-flight stream lets go. Since
`srv_take`/`srv_drop` are always added in balanced pairs and `srv_drop()` only
frees at zero, there is no underflow risk against the `new_server()`/`del
server`
reference.
Lifecycle note: after `del server` the object may briefly outlive the
`Server deleted.` reply, but it is already detached and its `lb_tree` is NULL (a
server must be in maintenance to be deleted, and the maintenance transition
removes it from the LB tree), so no new request can select it — only the streams
that already hold `s->target` keep using it until teardown. We also checked that
the leastconn/first `server_drop_conn` callbacks (`fwlc_srv_reposition`,
`fas_srv_reposition`) are no-ops once `lb_tree == NULL` (`lb_fwlc.c:335`,
`lb_fas.c:68`), so the deferred free does not reintroduce an LB-side
dereference.
The diff covers the six `s->target = &srv->obj_type` sites on the request path
(there is a seventh server assignment, `s->target = &newserv->obj_type` at
`backend.c:1157`, but it is immediately followed by `sess_change_server()` so it
carries no window and needs no ref). Full diff in Appendix A; the core is:
```c
void stream_set_srv_ref(struct stream *s, struct server *srv)
{
struct server *old = s->srv_ref;
if (srv == old)
return;
if (srv)
srv_take(srv);
s->srv_ref = srv;
if (old)
srv_drop(old);
}
```
called right after each `s->target = &srv->obj_type`, and `stream_set_srv_ref(s,
NULL)` in `stream_free()`.
# Proposal B (lighter): just assert the invariant
We're aware Proposal A adds two atomic ops per request on a hot path, to protect
against a window that is currently closed — that may not be a trade-off you
want.
A much cheaper option is to *document and enforce* the invariant instead of
pinning: e.g. a `BUG_ON()` in `srv_check_for_deletion()` (or a debug-only check
on
the assign→connect path) that catches any future code which reaches a
rescheduling point with `s->target` set to a server but `served == 0`. That
keeps
the hot path free of atomics and turns a silent UAF into a loud, early assertion
if someone ever reopens the window.
# Question
Do you consider the implicit invariant worth hardening at all — and if so, do
you
prefer pinning (A), an assertion (B), or something else (e.g. a dedicated
"assigned" counter consulted by `srv_check_for_deletion()`)? If you'd rather
leave
it as-is and rely on the comment, that's a perfectly good answer too — we just
wanted to surface it.
Thanks for your time,
## Appendix A — reference diff (against 3.2.18)
Reference only. Adds a per-stream `srv_ref` pinned via the existing
`srv_take()`/`srv_drop()` refcount for as long as the stream references the
server
as its target, released in `stream_free()`. Covers all six
`s->target = &srv->obj_type` sites on the request path.
```diff
--- orig/include/haproxy/stream-t.h
+++ new/include/haproxy/stream-t.h
@@ -252,6 +252,7 @@
struct proxy *be; /* the proxy this stream depends on for the server side */
struct server *srv_conn; /* stream already has a slot on a server and is not in
queue */
+ struct server *srv_ref; /* server pinned via srv_take() while it is our
target; released in stream_free() */
struct pendconn *pend_pos; /* if not NULL, points to the pending position in
the pending queue */
struct http_txn *txn; /* current HTTP transaction being processed. Should
become a list. */
--- orig/include/haproxy/stream.h
+++ new/include/haproxy/stream.h
@@ -66,6 +66,7 @@
/* shutdown the stream from itself */
void stream_shutdown_self(struct stream *stream, int why);
+void stream_set_srv_ref(struct stream *s, struct server *srv);
void stream_dump_and_crash(enum obj_type *obj, int rate);
void strm_dump_to_buffer(struct buffer *buf, const struct stream *strm, const
char *pfx, uint32_t anon_key);
--- orig/src/stream.c
+++ new/src/stream.c
@@ -499,6 +499,7 @@
stream_init_srv_conn(s);
s->target = sess->fe->default_target;
+ s->srv_ref = NULL;
s->pend_pos = NULL;
s->priority_class = 0;
@@ -602,6 +603,25 @@
/*
* frees the context associated to a stream. It must have been removed first.
*/
+/* Pin server <srv> (may be NULL) with a refcount for as long as it is
+ * referenced as this stream's target. Without this, a concurrent runtime
+ * "del server" can free the server object while an in-flight stream still
+ * points at it (SF_ASSIGNED set but the stream not yet registered in the
+ * server's stream list, so served==0). The reference is released in
stream_free(). */
+void stream_set_srv_ref(struct stream *s, struct server *srv)
+{
+ struct server *old = s->srv_ref;
+
+ if (srv == old)
+ return;
+ if (srv)
+ srv_take(srv);
+ s->srv_ref = srv;
+ if (old)
+ srv_drop(old);
+}
+
void stream_free(struct stream *s)
{
struct session *sess = strm_sess(s);
@@ -628,6 +648,7 @@
process_srv_queue(__objt_server(s->target));
}
+ stream_set_srv_ref(s, NULL);
if (unlikely(s->srv_conn)) {
struct server *oldsrv = s->srv_conn;
/* the stream still has a reserved slot on a server, but
@@ -1230,6 +1251,7 @@
(s->flags & SF_FORCE_PRST)) {
s->flags |= SF_DIRECT | SF_ASSIGNED;
s->target = &srv->obj_type;
+ stream_set_srv_ref(s, srv);
break;
}
/* if the server is not UP, let's go on with next rules
@@ -1294,6 +1316,7 @@
(px->options & PR_O_PERSIST) || (s->flags & SF_FORCE_PRST)) {
s->flags |= SF_DIRECT | SF_ASSIGNED;
s->target = &srv->obj_type;
+ stream_set_srv_ref(s, srv);
}
}
--- orig/src/backend.c
+++ new/src/backend.c
@@ -660,6 +660,7 @@
if (!(conn->flags & CO_FL_WAIT_XPRT)) {
srv = tmpsrv;
s->target = &srv->obj_type;
+ stream_set_srv_ref(s, srv);
if (conn->flags & CO_FL_SESS_IDLE) {
conn->flags &= ~CO_FL_SESS_IDLE;
s->sess->idle_conns--;
@@ -828,6 +829,7 @@
_HA_ATOMIC_INC(&srv->counters.cum_lbconn);
}
s->target = &srv->obj_type;
+ stream_set_srv_ref(s, srv);
}
else if (s->be->options & (PR_O_DISPATCH | PR_O_TRANSP)) {
s->target = &s->be->obj_type;
@@ -2962,6 +2964,7 @@
/* we found the server and it is usable */
s->flags |= SF_DIRECT | SF_ASSIGNED;
s->target = &srv->obj_type;
+ stream_set_srv_ref(s, srv);
break;
}
}
--- orig/src/http_ana.c
+++ new/src/http_ana.c
@@ -3458,6 +3458,7 @@
txn->flags |= (srv->cur_state != SRV_ST_STOPPED) ? TX_CK_VALID : TX_CK_DOWN;
s->flags |= SF_DIRECT | SF_ASSIGNED;
s->target = &srv->obj_type;
+ stream_set_srv_ref(s, srv);
break;
} else {
/* we found a server, but it's down,
```
## Appendix B — `haproxy -vv`
Our build carries out-of-tree patches (including the `SRV_F_NON_PURGEABLE`
relaxation mentioned above); the version string does not advertise them. This is
otherwise a standard 3.2.18 build.
```
HAProxy version 3.2.18-99697c190 2026/05/06 - https://haproxy.org/
Status: long-term supported branch - will stop receiving fixes around Q2 2030.
Running on: Linux 6.6.137 aarch64
Build options :
TARGET = linux-glibc
CC = cc
CFLAGS = -O2 -g -fwrapv -fvect-cost-model=very-cheap
OPTIONS = USE_THREAD=1 USE_OPENSSL=1 USE_LUA=1 USE_ZLIB=1 USE_PCRE2=1
Built with multi-threading support (MAX_TGROUPS=32, MAX_THREADS=1024,
default=2).
Running with nbthread 4.
```
이영광 Lee YoungKwang
Hyper Connect Platform
경기도 성남시 분당구 분당내곡로 131 판교테크원 타워1 (우)13529
Tel 031-600-6124 Mobile 01046260216
Email [email protected]
위 전자우편 및 그에 포함된 정보는 위에 기재된 수신인만을 위해 발송되는 것으로서 보안을 유지해야 하는 정보 및 법률상 또는 다른 사유로
인하여 공개가 금지된 정보가 들어 있을 수 있습니다.
귀하가 이 전자우편의 지정 수신인이 아니면 이를 무단으로 보유, 복제, 전송, 배포, 공개할 수 없으며, 일부의 내용이라도 보유, 복제,
배포, 공개해서는 안됩니다.
그러므로, 잘못 수신된 경우에는 즉시 네이버 클라우드 개인정보보호([email protected])로 연락하여
주시고, 원본 및 사본과 그에 따른 첨부 문서를 모두 삭제하여 주시기 바랍니다. 협조하여 주셔서 감사합니다.
This email and the information contained in this email are intended solely for
the recipient(s) addressed above and may contain information that is
confidential and/or privileged or whose disclosure is prohibited by law or
other reasons.
If you are not the intended recipient of this email, please be advised that any
unauthorized storage, duplication, dissemination, distribution or disclosure of
all or part of this email is strictly prohibited.
If you received this email in error, please immediately contact NAVER Cloud
Privacy([email protected]) and delete this email and any copies
and attachments from your system. Thank you for your cooperation.
