]> git.ipfire.org Git - thirdparty/haproxy.git/commitdiff
projects / thirdparty / haproxy.git / commitdiff
? search:
summary | shortlog | log | commit | commitdiff | tree
raw | patch | inline | side by side (parent: 3c4f40a)
BUG/MINOR: mux-h2: use a safe list_for_each_entry in h2_send()
authorWilly Tarreau <w@1wt.eu>
Fri, 10 Jan 2020 16:01:29 +0000 (17:01 +0100)
committerWilly Tarreau <w@1wt.eu>
Fri, 10 Jan 2020 16:18:32 +0000 (17:18 +0100)
h2_send() uses list_for_each_entry() to scan paused streams and resume
them, but happily deletes any leftover from a previous failed unsubscribe,
which is obviously not safe and would corrupt the list. In practice this
is a proof that this doesn't happen, but it's not the best way to prove it.
In order to fix this and reduce the maintenance burden caused by code
duplication (this list walk exists at 3 places), let's introduce a new
function h2_resume_each_sending_h2s() doing exactly this and use it at
all 3 places.

This bug was introduced as a side effect of fix 998410a41b ("BUG/MEDIUM:
h2: Revamp the way send subscriptions works.") so it should be backported
as far as 1.9.

src/mux_h2.c patch | blob | blame | history

diff --git a/src/mux_h2.c b/src/mux_h2.c
index 6ec8d6c0250247c55d6f294466797095ac8e01bb..c4572d553c264e9615b47478b0572b1e8dd26733 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -3248,13 +3248,41 @@ static void h2_process_demux(struct h2c *h2c)
        TRACE_LEAVE(H2_EV_H2C_WAKE, h2c->conn);
 }
 
+/* resume each h2s eligible for sending in list head <head> */
+static void h2_resume_each_sending_h2s(struct h2c *h2c, struct list *head)
+{
+       struct h2s *h2s, *h2s_back;
+
+       TRACE_ENTER(H2_EV_H2C_SEND|H2_EV_H2S_WAKE, h2c->conn);
+
+       list_for_each_entry_safe(h2s, h2s_back, head, list) {
+               if (h2c->mws <= 0 ||
+                   h2c->flags & H2_CF_MUX_BLOCK_ANY ||
+                   h2c->st0 >= H2_CS_ERROR)
+                       break;
+
+               h2s->flags &= ~H2_SF_BLK_ANY;
+               /* For some reason, the upper layer failed to subscribe again,
+                * so remove it from the send_list
+                */
+               if (!h2s->send_wait) {
+                       LIST_DEL_INIT(&h2s->list);
+                       continue;
+               }
+
+               h2s->send_wait->events &= ~SUB_RETRY_SEND;
+               LIST_ADDQ(&h2c->sending_list, &h2s->sending_list);
+               tasklet_wakeup(h2s->send_wait->tasklet);
+       }
+
+       TRACE_LEAVE(H2_EV_H2C_SEND|H2_EV_H2S_WAKE, h2c->conn);
+}
+
 /* process Tx frames from streams to be multiplexed. Returns > 0 if it reached
  * the end.
  */
 static int h2_process_mux(struct h2c *h2c)
 {
-       struct h2s *h2s, *h2s_back;
-
        TRACE_ENTER(H2_EV_H2C_WAKE, h2c->conn);
 
        if (unlikely(h2c->st0 < H2_CS_FRAME_H)) {
@@ -3287,47 +3315,8 @@ static int h2_process_mux(struct h2c *h2c)
         * waiting there were already elected for immediate emission but were
         * blocked just on this.
         */
-
-       list_for_each_entry_safe(h2s, h2s_back, &h2c->fctl_list, list) {
-               if (h2c->mws <= 0 || h2c->flags & H2_CF_MUX_BLOCK_ANY ||
-                   h2c->st0 >= H2_CS_ERROR)
-                       break;
-
-               if (LIST_ADDED(&h2s->sending_list))
-                       continue;
-
-               h2s->flags &= ~H2_SF_BLK_ANY;
-               /* For some reason, the upper layer failed to subsribe again,
-                * so remove it from the send_list
-                */
-               if (!h2s->send_wait) {
-                       LIST_DEL_INIT(&h2s->list);
-                       continue;
-               }
-               h2s->send_wait->events &= ~SUB_RETRY_SEND;
-               LIST_ADDQ(&h2c->sending_list, &h2s->sending_list);
-               tasklet_wakeup(h2s->send_wait->tasklet);
-       }
-
-       list_for_each_entry_safe(h2s, h2s_back, &h2c->send_list, list) {
-               if (h2c->st0 >= H2_CS_ERROR || h2c->flags & H2_CF_MUX_BLOCK_ANY)
-                       break;
-
-               if (LIST_ADDED(&h2s->sending_list))
-                       continue;
-
-               /* For some reason, the upper layer failed to subsribe again,
-                * so remove it from the send_list
-                */
-               if (!h2s->send_wait) {
-                       LIST_DEL_INIT(&h2s->list);
-                       continue;
-               }
-               h2s->flags &= ~H2_SF_BLK_ANY;
-               h2s->send_wait->events &= ~SUB_RETRY_SEND;
-               LIST_ADDQ(&h2c->sending_list, &h2s->sending_list);
-               tasklet_wakeup(h2s->send_wait->tasklet);
-       }
+       h2_resume_each_sending_h2s(h2c, &h2c->fctl_list);
+       h2_resume_each_sending_h2s(h2c, &h2c->send_list);
 
  fail:
        if (unlikely(h2c->st0 >= H2_CS_ERROR)) {
@@ -3511,30 +3500,9 @@ static int h2_send(struct h2c *h2c)
        /* We're not full anymore, so we can wake any task that are waiting
         * for us.
         */
-       if (!(h2c->flags & (H2_CF_MUX_MFULL | H2_CF_DEM_MROOM)) && h2c->st0 >= H2_CS_FRAME_H) {
-               struct h2s *h2s;
-
-               list_for_each_entry(h2s, &h2c->send_list, list) {
-                       if (h2c->st0 >= H2_CS_ERROR || h2c->flags & H2_CF_MUX_BLOCK_ANY)
-                               break;
-
-                       if (LIST_ADDED(&h2s->sending_list))
-                               continue;
+       if (!(h2c->flags & (H2_CF_MUX_MFULL | H2_CF_DEM_MROOM)) && h2c->st0 >= H2_CS_FRAME_H)
+               h2_resume_each_sending_h2s(h2c, &h2c->send_list);
 
-                       /* For some reason, the upper layer failed to subsribe again,
-                        * so remove it from the send_list
-                        */
-                       if (!h2s->send_wait) {
-                               LIST_DEL_INIT(&h2s->list);
-                               continue;
-                       }
-                       h2s->flags &= ~H2_SF_BLK_ANY;
-                       h2s->send_wait->events &= ~SUB_RETRY_SEND;
-                       TRACE_DEVEL("waking up pending stream", H2_EV_H2C_SEND|H2_EV_H2S_WAKE, h2c->conn, h2s);
-                       tasklet_wakeup(h2s->send_wait->tasklet);
-                       LIST_ADDQ(&h2c->sending_list, &h2s->sending_list);
-               }
-       }
        /* We're done, no more to send */
        if (!br_data(h2c->mbuf)) {
                TRACE_DEVEL("leaving with everything sent", H2_EV_H2C_SEND, h2c->conn);
Mirror of https://github.com/haproxy/haproxy.git