Hi Lee,
On Fri, Jul 10, 2026 at 11:03:00AM +0900, Lee YoungKwang wrote:
> 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.
Nice analysis, thank you. Indeed as you guessed, option A is too expensive,
it would bring us a few years back in terms of performance on large systems.
Option B could be sufficient for something that currently cannot happen (or
at least that we suppose cannot).
In parallel, I suggest you have a look at recent fix ca686e3208
("BUG/MEDIUM: server: Properly check for streams before deletion"), as I
suspect it might address the same problem. In this case we did an atomic
op but only per thread-group, as is already done for a few other counters,
which amortizes the cost. It's never much pleasant but it was a reasonable
tradeoff that did not exhibit a performance regression for now (a per-thread
variant was also attempted and we could turn to that one if a performance
issue were to be noticed).
> Thanks for your time,
>
>
> ## Appendix A -- reference diff (against 3.2.18)
Please note, your mail client has completely corrupted your patches,
see below. For future exchanges, you might prefer to attach them (or
to fix the mail client's setup):
> 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.
> */
(...)
Thanks!
Willy
