From: Willy Tarreau <w@1wt.eu>
Date: Fri, 17 Nov 2023 09:56:33 +0000 (+0100)
Subject: BUG/MEDIUM: mux-h1: fail earlier on malloc in takeover()
X-Git-Tag: v2.9-dev10~28
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=95fd2d68018d749ff0ad92b4b69d4b3fd75a82e4;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: mux-h1: fail earlier on malloc in takeover()

This is the h1 equivalent of previous "BUG/MEDIUM: mux-h2: fail earlier
on malloc in takeover()".

Connection takeover was implemented for H1 in 2.2 by commit f12ca9f8f1
("MEDIUM: mux_h1: Implement the takeover() method."). It does have one
corner case related to memory allocation failure: in case the task or
tasklet allocation fails, the connection gets released synchronously.

Unfortunately the situation is bad there, because the lower layers are
already switched to the new thread while the tasklet is either NULL or
still the old one, and calling h1_release() will call some unsubscribe
and and possibly other things whose safety is not guaranteed (and the
ambiguity here alone is sufficient to be careful). There are even code
paths where the thread will try to grab the lock of its own idle conns
list, believing the connection is there while it has no useful effect.
However, if the owner thread was doing the same at the same moment, and
ended up trying to pick from the current thread (which could happen if
picking a connection for a different name), the two could even deadlock.

Contrary to mux-h2, a few tests were not sufficient to try to crash the
process, but there's nothing that indicates it couldn't happen based on
the description above.

This patch takes a simple but radically different approach. Instead of
starting to migrate the connection before risking to face allocation
failures, it first pre-allocates a new task and tasklet, then assigns
them to the connection if the migration succeeds, otherwise it just
frees them. This way it's no longer needed to manipulate the connection
until it's fully migrated, and as a bonus this means the connection will
continue to exist and the use-after-free condition is solved at the same
time.

This should be backported to 2.2. Thanks to Fred for the initial analysis
of the problem!
---

diff --git a/src/mux_h1.c b/src/mux_h1.c
index 8980fa11c0..9f16e56bac 100644
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -4998,9 +4998,19 @@ static int h1_takeover(struct connection *conn, int orig_tid)
 {
 	struct h1c *h1c = conn->ctx;
 	struct task *task;
+	struct task *new_task;
+	struct tasklet *new_tasklet;
+
+	/* Pre-allocate tasks so that we don't have to roll back after the xprt
+	 * has been migrated.
+	 */
+	new_task = task_new_here();
+	new_tasklet = tasklet_new();
+	if (!new_task || !new_tasklet)
+		goto fail;
 
 	if (fd_takeover(conn->handle.fd, conn) != 0)
-		return -1;
+		goto fail;
 
 	if (conn->xprt->takeover && conn->xprt->takeover(conn, conn->xprt_ctx, orig_tid) != 0) {
 		/* We failed to takeover the xprt, even if the connection may
@@ -5010,44 +5020,49 @@ static int h1_takeover(struct connection *conn, int orig_tid)
 		 */
 		conn->flags |= CO_FL_ERROR;
 		tasklet_wakeup_on(h1c->wait_event.tasklet, orig_tid);
-		return -1;
+		goto fail;
 	}
 
 	if (h1c->wait_event.events)
 		h1c->conn->xprt->unsubscribe(h1c->conn, h1c->conn->xprt_ctx,
 		    h1c->wait_event.events, &h1c->wait_event);
-	/* To let the tasklet know it should free itself, and do nothing else,
-	 * set its context to NULL.
-	 */
-	h1c->wait_event.tasklet->context = NULL;
-	tasklet_wakeup_on(h1c->wait_event.tasklet, orig_tid);
 
 	task = h1c->task;
 	if (task) {
+		/* only assign a task if there was already one, otherwise
+		 * the preallocated new task will be released.
+		 */
 		task->context = NULL;
 		h1c->task = NULL;
 		__ha_barrier_store();
 		task_kill(task);
 
-		h1c->task = task_new_here();
-		if (!h1c->task) {
-			h1_release(h1c);
-			return -1;
-		}
+		h1c->task = new_task;
+		new_task = NULL;
 		h1c->task->process = h1_timeout_task;
 		h1c->task->context = h1c;
 	}
-	h1c->wait_event.tasklet = tasklet_new();
-	if (!h1c->wait_event.tasklet) {
-		h1_release(h1c);
-		return -1;
-	}
+
+	/* To let the tasklet know it should free itself, and do nothing else,
+	 * set its context to NULL.
+	 */
+	h1c->wait_event.tasklet->context = NULL;
+	tasklet_wakeup_on(h1c->wait_event.tasklet, orig_tid);
+
+	h1c->wait_event.tasklet = new_tasklet;
 	h1c->wait_event.tasklet->process = h1_io_cb;
 	h1c->wait_event.tasklet->context = h1c;
 	h1c->conn->xprt->subscribe(h1c->conn, h1c->conn->xprt_ctx,
 		                   SUB_RETRY_RECV, &h1c->wait_event);
 
+	if (new_task)
+		__task_free(new_task);
 	return 0;
+ fail:
+	if (new_task)
+		__task_free(new_task);
+	tasklet_free(new_tasklet);
+	return -1;
 }