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.

diff --git a/src/cfgparse.c b/src/cfgparse.c
index c18628a8f45856aa1e6e0280279f44a7b83ad967..7752fc72324196fe34c890e7852adf4181825e81 100644 (file)
--- a/src/cfgparse.c
+++ b/src/cfgparse.c
@@ -4712,14 +4712,13 @@ stats_error_parsing:
                        l = (struct list *)&curproxy->tcpcheck_rules;
                        if (l->p != l->n) {
                                tcpcheck = (struct tcpcheck_rule *)l->n;
-                               while (tcpcheck->action == TCPCHK_ACT_COMMENT) {
+                               while (&tcpcheck->list != &curproxy->tcpcheck_rules &&
+                                      tcpcheck->action == TCPCHK_ACT_COMMENT) {
                                        tcpcheck = (struct tcpcheck_rule *)tcpcheck->list.n;
                                }
-                               /* we've reached the end of the list, and the list is full of comments */
-                               if (tcpcheck == (struct tcpcheck_rule *)l)
-                                       tcpcheck = NULL;
 
-                               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 cc29ac84320cc89bd9b122e5f422b951cdda5268..78f022fd3c9b3622af8c65e71cd0b5cc209867c1 100644 (file)
--- a/src/checks.c
+++ b/src/checks.c
@@ -2522,7 +2522,7 @@ static void tcpcheck_main(struct connection *conn)
                next = (struct tcpcheck_rule *)check->current_step->list.n;
 
                /* bypass all comment rules */
-               while (next->action == TCPCHK_ACT_COMMENT)
+               while (&next->list != head && next->action == TCPCHK_ACT_COMMENT)
                        next = (struct tcpcheck_rule *)next->list.n;
 
                /* NULL if we're on the last rule */
@@ -2636,9 +2636,13 @@ static void tcpcheck_main(struct connection *conn)
                        check->current_step = (struct tcpcheck_rule *)check->current_step->list.n;
 
                        /* bypass all comment rules */
-                       while (check->current_step->action == TCPCHK_ACT_COMMENT)
+                       while (&check->current_step->list != head &&
+                               check->current_step->action == TCPCHK_ACT_COMMENT)
                                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 */
@@ -2694,8 +2698,12 @@ static void tcpcheck_main(struct connection *conn)
                        check->current_step = (struct tcpcheck_rule *)check->current_step->list.n;
 
                        /* bypass all comment rules */
-                       while (check->current_step->action == TCPCHK_ACT_COMMENT)
+                       while (&check->current_step->list != head &&
+                               check->current_step->action == TCPCHK_ACT_COMMENT)
                                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))
@@ -2789,9 +2797,13 @@ static void tcpcheck_main(struct connection *conn)
                                        check->current_step = (struct tcpcheck_rule *)check->current_step->list.n;
 
                                        /* bypass all comment rules */
-                                       while (check->current_step->action == TCPCHK_ACT_COMMENT)
+                                       while (&check->current_step->list != head &&
+                                              check->current_step->action == TCPCHK_ACT_COMMENT)
                                                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);
@@ -2805,9 +2817,13 @@ static void tcpcheck_main(struct connection *conn)
                                        check->current_step = (struct tcpcheck_rule *)check->current_step->list.n;
 
                                        /* bypass all comment rules */
-                                       while (check->current_step->action == TCPCHK_ACT_COMMENT)
+                                       while (&check->current_step->list != head &&
+                                              check->current_step->action == TCPCHK_ACT_COMMENT)
                                                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);