From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Wed, 21 Feb 2024 14:54:11 +0000 (+0100)
Subject: BUG/MAJOR: server: fix stream crash due to deleted server
X-Git-Tag: v3.0-dev4~8
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1b8c5abeeb2ab8428be3aa53ff42709d60b83bd7;p=thirdparty%2Fhaproxy.git

BUG/MAJOR: server: fix stream crash due to deleted server

Before a dynamic server can be deleted, a set of preconditions must be
validated to ensure it is not referenced naymore by a stream or a
connection. This is implemented in srv_check_for_deletion().

The various criteria specified were incomplete. This allows a server
instance to be deleted while still be referenced by a stream and a
connection.

This bug was reproduced by using ASAN compilation. A script was used to
add and delete a server every second, while using h2load to generate
traffic with download of 1k objects. Here is the ASAN error.

==140916==ERROR: AddressSanitizer: heap-use-after-free on address 0x520000020080 at pc 0x63cb25679537 bp 0x701529ff5070 sp 0x701529ff5060
READ of size 1 at 0x520000020080 thread T7
    #0 0x63cb25679536 in objt_server include/haproxy/obj_type.h:99
    #1 0x63cb2568f465 in process_stream src/stream.c:1823
    #2 0x63cb25a4a4a2 in run_tasks_from_lists src/task.c:632
    #3 0x63cb25a4bf62 in process_runnable_tasks src/task.c:876
    #4 0x63cb2596a220 in run_poll_loop src/haproxy.c:3050
    #5 0x63cb2596b192 in run_thread_poll_loop src/haproxy.c:3252
    #6 0x701539aa9559  (/usr/lib/libc.so.6+0x8b559) (BuildId: c0caa0b7709d3369ee575fcd7d7d0b0fc48733af)
    #7 0x701539b26a3b  (/usr/lib/libc.so.6+0x108a3b) (BuildId: c0caa0b7709d3369ee575fcd7d7d0b0fc48733af)

To fix this, add <curr_used_conns> to the counters checked in
srv_check_for_deletion().

Outside of this bug, one case which remains sensible is for SF_DIRECT
streams which referenced a server instance early in process_stream()
before connect_server(). This occurs with use-server directive,
force-persist rule or cookie persistence. However, after code
reexamination, the code is considered reliable as process_stream() is
not rescheduled before connect_server() invocation. These observations
have been saved in sess_change_server() documentation to ensure it
remains valid in the future.

This must be backported up to 2.6.
---

diff --git a/src/server.c b/src/server.c
index df24612f52..197600ebd1 100644
--- a/src/server.c
+++ b/src/server.c
@@ -5796,7 +5796,7 @@ int srv_check_for_deletion(const char *bename, const char *svname, struct proxy
 	 * TODO idle connections should not prevent server deletion. A proper
 	 * cleanup function should be implemented to be used here.
 	 */
-	if (srv->cur_sess || srv->curr_idle_conns ||
+	if (srv->curr_used_conns || srv->curr_idle_conns ||
 	    !eb_is_empty(&srv->queue.head) || srv_has_streams(srv)) {
 		msg = "Server still has connections attached to it, cannot remove it.";
 		goto leave;
diff --git a/src/stream.c b/src/stream.c
index 54c70ecba9..40756e6ae6 100644
--- a/src/stream.c
+++ b/src/stream.c
@@ -2681,6 +2681,20 @@ void sess_change_server(struct stream *strm, struct server *newsrv)
 {
 	struct server *oldsrv = strm->srv_conn;
 
+	/* Dynamic servers may be deleted during process lifetime. This
+	 * operation is always conducted under thread isolation. Several
+	 * conditions prevent deletion, one of them is if server streams list
+	 * is not empty. sess_change_server() uses stream_add_srv_conn() to
+	 * ensure the latter condition.
+	 *
+	 * 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().
+	 */
+
 	if (oldsrv == newsrv)
 		return;