From 61882cc68c8336016f158f74c0944e1b047c6f5f Mon Sep 17 00:00:00 2001
From: Aurelien DARRAGON <adarragon@haproxy.com>
Date: Fri, 18 Nov 2022 09:17:29 +0100
Subject: [PATCH] BUG/MINOR: http_ana/txn: don't re-initialize txn and req var
 lists

In http_create_txn(): vars_init_head() was performed on both s->vars_txn
and s->var_reqres lists.

But this is wrong, these two lists are already initialized upon stream
creation in stream_new().
Moreover, between stream_new() and http_create_txn(), some variable may
be defined (e.g.: by the frontend), resulting in lists not being empty.

Because of this "extra" list initialization, already defined variables
can be lost.
This causes txn dependant code not being able to access previously defined
variables as well as memory leak because http_destroy_txn() relies on these
lists to perform the purge.

This proved to be the case when a frontend sets variables and lua sample
fetch is used in backend section as described in GH #1935.
Many thanks to Darragh O'Toole for his detailed report.

Removing extra var_init_head (x2) in http_create_txn() to fix the issue.
Adding somme comments in the code in an attempt to prevent future misuses
of s->var_reqres, and s->var_txn lists.

It should be backported in every stable version.
(This is an old bug that seems to exist since 1.6-dev6)

[cf: On 2.0 and 1.8, for the legacy HTTP code, vars_init() are used during
     the TXN cleanup, when the stream is reused. So, these calls must be
     moved from http_init_txn() to http_reset_txn() and not removed.]

(cherry picked from commit 5ad2b642625b89cdf4f5fd26a598fc480abdc806)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

Conflict: NA
Reference: https://git.haproxy.org/?p=haproxy-2.6.git;a=commit;h=61882cc68c8336016f158f74c0944e1b047c6f5f
---
 src/http_ana.c |  6 ++++--
 src/stream.c   | 11 ++++++++++-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/src/http_ana.c b/src/http_ana.c
index 2b2cfdc56..379b480a8 100644
--- a/src/http_ana.c
+++ b/src/http_ana.c
@@ -5224,8 +5224,10 @@ struct http_txn *http_create_txn(struct stream *s)
 
 	txn->auth.method = HTTP_AUTH_UNKNOWN;
 
-	vars_init_head(&s->vars_txn,    SCOPE_TXN);
-	vars_init_head(&s->vars_reqres, SCOPE_REQ);
+	/* here we don't want to re-initialize s->vars_txn and s->vars_reqres
+	 * variable lists, because they were already initialized upon stream
+	 * creation in stream_new(), and thus may already contain some variables
+	 */
 
 	return txn;
 }
diff --git a/src/stream.c b/src/stream.c
index c04dd565c..7eaf03995 100644
--- a/src/stream.c
+++ b/src/stream.c
@@ -435,8 +435,17 @@ struct stream *stream_new(struct session *sess, struct stconn *sc, struct buffer
 	s->req_cap = NULL;
 	s->res_cap = NULL;
 
-	/* Initialise all the variables contexts even if not used.
+	/* Initialize all the variables contexts even if not used.
 	 * This permits to prune these contexts without errors.
+	 *
+	 * We need to make sure that those lists are not re-initialized
+	 * by stream-dependant underlying code because we could lose
+	 * track of already defined variables, leading to data inconsistency
+	 * and memory leaks...
+	 *
+	 * For reference: we had a very old bug caused by vars_txn and
+	 * vars_reqres being accidentally re-initialized in http_create_txn()
+	 * (https://github.com/haproxy/haproxy/issues/1935)
 	 */
 	vars_init_head(&s->vars_txn,    SCOPE_TXN);
 	vars_init_head(&s->vars_reqres, SCOPE_REQ);
-- 
2.33.0

