From 5ad2b642625b89cdf4f5fd26a598fc480abdc806 Mon Sep 17 00:00:00 2001 From: Aurelien DARRAGON 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.] --- 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 188cb7307..078d8beb6 100644 --- a/src/http_ana.c +++ b/src/http_ana.c @@ -5213,8 +5213,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 59d84b921..9906b2141 100644 --- a/src/stream.c +++ b/src/stream.c @@ -437,8 +437,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);