From 97fccc87f1297d189ee80735e5b8746c34956eda Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Wed, 13 May 2015 12:08:21 +0200 Subject: [PATCH 7/8] BUG/MAJOR: checks: always check for end of list before proceeding This is the most important fix of this series. There's a risk of endless loop and crashes caused by the fact that we go past the head of the list when skipping to next rule, without checking if it's still a valid element. Most of the time, the ->action field is checked, which points to the proxy's check_req pointer (generally NULL), meaning the element is confused with a TCPCHK_ACT_SEND action. The situation was accidently made worse with the addition of tcp-check comment since it also skips list elements. However, since the action that makes it go forward is TCPCHK_ACT_COMMENT (3), there's little chance to see this as a valid pointer, except on 64-bit machines where it can match the end of a check_req string pointer. This fix heavily depends on previous cleanup and both must be backported to 1.5 where the bug is present. (cherry picked from commit f2c87353a7f8160930b5f342bb6d6ad0991ee3d1) [wt: this patch differs significantly from 1.6 since we don't have comments] --- src/cfgparse.c | 4 +++- src/checks.c | 12 ++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/cfgparse.c b/src/cfgparse.c index 746c7eb..dba59d1 100644 --- a/src/cfgparse.c +++ b/src/cfgparse.c @@ -4368,7 +4368,9 @@ stats_error_parsing: l = (struct list *)&curproxy->tcpcheck_rules; if (l->p != l->n) { tcpcheck = (struct tcpcheck_rule *)l->n; - if (tcpcheck && tcpcheck->action != TCPCHK_ACT_CONNECT) { + + if (&tcpcheck->list != &curproxy->tcpcheck_rules + && tcpcheck->action != TCPCHK_ACT_CONNECT) { Alert("parsing [%s:%d] : first step MUST also be a 'connect' when there is a 'connect' step in the tcp-check ruleset.\n", file, linenum); err_code |= ERR_ALERT | ERR_FATAL; diff --git a/src/checks.c b/src/checks.c index a0c42f2..e13d561 100644 --- a/src/checks.c +++ b/src/checks.c @@ -2057,6 +2057,9 @@ static void tcpcheck_main(struct connection *conn) /* allow next rule */ check->current_step = (struct tcpcheck_rule *)check->current_step->list.n; + if (&check->current_step->list == head) + break; + /* don't do anything until the connection is established */ if (!(conn->flags & CO_FL_CONNECTED)) { /* update expire time, should be done by process_chk */ @@ -2110,6 +2113,9 @@ static void tcpcheck_main(struct connection *conn) /* go to next rule and try to send */ check->current_step = (struct tcpcheck_rule *)check->current_step->list.n; + + if (&check->current_step->list == head) + break; } /* end 'send' */ else if (check->current_step->action == TCPCHK_ACT_EXPECT) { if (unlikely(check->result == CHK_RES_FAILED)) @@ -2196,6 +2202,9 @@ static void tcpcheck_main(struct connection *conn) /* allow next rule */ check->current_step = (struct tcpcheck_rule *)check->current_step->list.n; + if (&check->current_step->list == head) + break; + if (check->current_step->action == TCPCHK_ACT_EXPECT) goto tcpcheck_expect; __conn_data_stop_recv(conn); @@ -2208,6 +2217,9 @@ static void tcpcheck_main(struct connection *conn) /* allow next rule */ check->current_step = (struct tcpcheck_rule *)check->current_step->list.n; + if (&check->current_step->list == head) + break; + if (check->current_step->action == TCPCHK_ACT_EXPECT) goto tcpcheck_expect; __conn_data_stop_recv(conn); -- 2.0.5