git.openwrt.org Git - feed/packages.git/blob

   1 commit 27ebcefd41b3e44395c3fe71939ef98b03f98e7b
   2 Author: Christopher Faulet <cfaulet@haproxy.com>
   3 Date:   Fri Oct 25 10:21:01 2019 +0200
   4
   5     BUG/MAJOR: stream-int: Don't receive data from mux until SI_ST_EST is reached
   6
   7     This bug is pretty pernicious and have serious consequences : In 2.1, an
   8     infinite loop in process_stream() because the backend stream-interface remains
   9     in the ready state (SI_ST_RDY). In 2.0, a call in loop to process_stream()
  10     because the stream-interface remains blocked in the connect state
  11     (SI_ST_CON). In both cases, it happens after a connection retry attempt. In 1.9,
  12     it seems to not happen. But it may be just by chance or just because it is
  13     harder to get right conditions to trigger the bug. However, reading the code,
  14     the bug seems to exist too.
  15
  16     Here is how the bug happens in 2.1. When we try to establish a new connection to
  17     a server, the corresponding stream-interface is first set to the connect state
  18     (SI_ST_CON). When the underlying connection is known to be connected (the flag
  19     CO_FL_CONNECTED set), the stream-interface is switched to the ready state
  20     (SI_ST_RDY). It is a transient state between the connect state (SI_ST_CON) and
  21     the established state (SI_ST_EST). It must be handled on the next call to
  22     process_stream(), which is responsible to operate the transition. During all
  23     this time, errors can occur. A connection error or a client abort. The transient
  24     state SI_ST_RDY was introduced to let a chance to process_stream() to catch
  25     these errors before considering the connection as fully established.
  26     Unfortunatly, if a read0 is catched in states SI_ST_CON or SI_ST_RDY, it is
  27     possible to have a shutdown without transition to SI_ST_DIS (in fact, here,
  28     SI_ST_CON is swichted to SI_ST_RDY). This happens if the request was fully
  29     received and analyzed. In this case, the flag SI_FL_NOHALF is set on the backend
  30     stream-interface. If an error is also reported during the connect, the behavior
  31     is undefined because an error is returned to the client and a connection retry
  32     is performed. So on the next connection attempt to the server, if another error
  33     is reported, a client abort is detected. But the shutdown for writes was already
  34     done. So the transition to the state SI_ST_DIS is impossible. We stay in the
  35     state SI_ST_RDY. Because it is a transient state, we loop in process_stream() to
  36     perform the transition.
  37
  38     It is hard to understand how the bug happens reading the code and even harder to
  39     explain. But there is a trivial way to hit the bug by sending h2 requests to a
  40     server only speaking h1. For instance, with the following config :
  41
  42       listen tst
  43         bind *:80
  44         server www 127.0.0.1:8000 proto h2 # in reality, it is a HTTP/1.1 server
  45
  46     It is a configuration error, but it is an easy way to observe the bug. Note it
  47     may happen with a valid configuration.
  48
  49     So, after a careful analyzis, it appears that si_cs_recv() should never be
  50     called for a not fully established stream-interface. This way the connection
  51     retries will be performed before reporting an error to the client. Thus, if a
  52     shutdown is performed because a read0 is handled, the stream-interface is
  53     inconditionnaly set to the transient state SI_ST_DIS.
  54
  55     This patch must be backported to 2.0 and 1.9. However on these versions, this
  56     patch reveals a design flaw about connections and a bad way to perform the
  57     connection retries. We are working on it.
  58
  59     (cherry picked from commit 04400bc7875fcc362495b0f25e75ba6fc2f44850)
  60     Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
  61
  62 diff --git a/src/stream_interface.c b/src/stream_interface.c
  63 index ef0fea7f..211fe2d7 100644
  64 --- a/src/stream_interface.c
  65 +++ b/src/stream_interface.c
  66 @@ -1215,6 +1215,10 @@ int si_cs_recv(struct conn_stream *cs)
  67         int read_poll = MAX_READ_POLL_LOOPS;
  68         int flags = 0;
  69
  70 +       /* If not established yet, do nothing. */
  71 +       if (si->state != SI_ST_EST)
  72 +               return 0;
  73 +
  74         /* If another call to si_cs_recv() failed, and we subscribed to
  75          * recv events already, give up now.
  76          */
  77 @@ -1293,8 +1297,6 @@ int si_cs_recv(struct conn_stream *cs)
  78                         ic->total += ret;
  79                         cur_read += ret;
  80                         ic->flags |= CF_READ_PARTIAL;
  81 -                       if (si->state == SI_ST_CON)
  82 -                               si->state = SI_ST_RDY;
  83                 }
  84
  85                 if (cs->flags & CS_FL_EOS)
  86 @@ -1391,8 +1393,6 @@ int si_cs_recv(struct conn_stream *cs)
  87
  88                 ic->flags |= CF_READ_PARTIAL;
  89                 ic->total += ret;
  90 -               if (si->state == SI_ST_CON)
  91 -                       si->state = SI_ST_RDY;
  92
  93                 if ((ic->flags & CF_READ_DONTWAIT) || --read_poll <= 0) {
  94                         /* we're stopped by the channel's policy */
  95 @@ -1544,16 +1544,7 @@ static void stream_int_read0(struct stream_interface *si)
  96
  97         si_done_get(si);
  98
  99 -       /* Don't change the state to SI_ST_DIS yet if we're still
 100 -        * in SI_ST_CON, otherwise it means sess_establish() hasn't
 101 -        * been called yet, and so the analysers would not run. However
 102 -        * it's fine to switch to SI_ST_RDY as we have really validated
 103 -        * the connection.
 104 -        */
 105 -       if (si->state == SI_ST_EST)
 106 -               si->state = SI_ST_DIS;
 107 -       else if (si->state == SI_ST_CON)
 108 -               si->state = SI_ST_RDY;
 109 +       si->state = SI_ST_DIS;
 110         si->exp = TICK_ETERNITY;
 111         return;
 112  }