BUG/MEDIUM: h1: prevent a crash on HTTP/2 upgrade

Change h1_process() to return -2 when the mux is destroyed but the
connection is not, so that we can differentiate between "both mux and
connection were destroyed" and "only the mux was destroyed".
It can happen that only the mux gets destroyed, and the connection is
still alive, if we did upgrade it to HTTP/2.
In h1_wake(), if the connection is alive, then return 0, as the wake
methods should only return -1 if the connection is dead.
This fixes a bug where the ssl xprt would consider the connection
destroyed, and thus would consider its tasklet should die, and return
NULL, and its TASK_RUNNING flag would never be removed, leading to an
infinite loop later on. This would happen anytime an HTTP/2 upgrade was
successful.

This should be backported up to 2.8. While the bug by commit
00f43b7c8b136515653bcb2fc014b0832ec32d61, it was not triggered before
only by chance, and exists in previous releases too.

diff --git a/src/mux_h1.c b/src/mux_h1.c
index 0b533d758c532a0434f94d53817d5527fcbf6e29..7221434643a0991f4fc0925196569a3f2cd49f4e 100644 (file)
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -1210,7 +1210,7 @@ static int h1s_finish_detach(struct h1s *h1s)
                         * subscribe for reads waiting for new data
                         */
                        if (unlikely(b_data(&h1c->ibuf))) {
-                               if (h1_process(h1c) == -1) {
+                               if (h1_process(h1c) < 0) {
                                        /* h1c was released, don't reuse it anymore */
                                        goto released;
                                }
@@ -4078,12 +4078,14 @@ static int h1_send(struct h1c *h1c)
 }
 
 /* callback called on any event by the connection handler.
- * It applies changes and returns zero, or < 0 if it wants immediate
- * destruction of the connection.
+ * It applies changes and returns zero, -1 if everything was destroyed,
+ * and -2 if the mux was destroyed, but the connection is still alive because
+ * it was upgraded to H2.
  */
 static int h1_process(struct h1c * h1c)
 {
        struct connection *conn = h1c->conn;
+       int ret = -1;
 
        TRACE_ENTER(H1_EV_H1C_WAKE, conn);
 
@@ -4296,11 +4298,20 @@ static int h1_process(struct h1c * h1c)
                return 0;
        }
        else {
-               h1_release(h1c);
+               int relret;
+
+               relret = h1_release(h1c);
+               /*
+                * Okay if we just released the mux, but the connection itself
+                * is alive, because we upgraded to H2, we have to let the
+                * caller know.
+                */
+               if (relret == -1)
+                       ret = -2;
                TRACE_DEVEL("leaving after releasing the connection", H1_EV_H1C_WAKE);
        }
   released:
-       return -1;
+       return ret;
 }
 
 struct task *h1_io_cb(struct task *t, void *ctx, unsigned int state)
@@ -4406,7 +4417,13 @@ static int h1_wake(struct connection *conn)
 
                if (h1c->state == H1_CS_UPGRADING || h1c->state == H1_CS_RUNNING)
                        h1_alert(h1s);
-       }
+       /*
+        * If we just destroyed the mux, but the connection is still alive,
+        * because the mux has been upgraded to H2, then from the caller's
+        * point of view, everything is okay, so return 0.
+        */
+       } else if (ret == -2)
+               ret = 0;
        return ret;
 }
 
@@ -4585,7 +4602,7 @@ static void h1_detach(struct sedesc *sd)
                 * subscribe for reads waiting for new data
                 */
                if (unlikely(b_data(&h1c->ibuf))) {
-                       if (h1_process(h1c) == -1)
+                       if (h1_process(h1c) < 0)
                                goto end;
                }
                else