On Sat, May 11, 2019 at 09:56:18AM +0200, Willy Tarreau wrote:
> I'm back to auditing the code to figure how we can free an h2s without
> first detaching it from the lists. I hope to have yet another patch to
> propose to you.

So I'm seeing something which bothers me in the code. Since I'm not at
ease with these parts I may say stupid things. But overall the idea is
the following :

- everywhere before releasing an h2s we remove it from its lists, so it
  cannot exist in a pool while still belonging to a list ; that's fine.
  => it means the element we're looping on is still live in the connection.

- at every place we're adding an element to the list, we take care of
  removing it first, or checking that it was not attached to a list. Every
  single place except one :

  static void h2c_update_all_ws(struct h2c *h2c, int diff)
  (...)
                if (h2s->mws > 0 && (h2s->flags & H2_SF_BLK_SFCTL)) {
                        h2s->flags &= ~H2_SF_BLK_SFCTL;
                        if (h2s->send_wait)
                                LIST_ADDQ(&h2c->send_list, &h2s->list);

                }

  Instead the check is made on send_wait being non-null.

- in h2s_frt_make_resp_data() we test this send_wait pointer before deciding
  to remove the element from the list in case we're blocking on flow control :

        if (size <= 0) {
                h2s->flags |= H2_SF_BLK_SFCTL;
                if (h2s->send_wait) {
                        LIST_DEL(&h2s->list);
                        LIST_INIT(&h2s->list);
                }
                goto end;
        }

  So if for any reason send_wait is not set, nobody removes the element
  from the list.

- this function h2s_frt_make_resp_data() is called from h2_snd_buf(), which
  starts by resetting h2s->send_wait. And it doesn't delete the stream from
  the list only in the case where it managed to send some data.

So what I'm wondering is if something like the following scenario may happen :

1) a stream wants to send data, it calls h2_snd_buf() a first time. The
   output buffer is full, the call fails

2) the caller (si_cs_send()) detects the error and calls h2_subscribe() to
   attach the stream to the send_list

3) some room is made in the buffer, the list is walked over and this stream
   is woken up.

4) h2_snd_buf() is called again to try to send, send_wait is immediately reset

5) h2s_frt_make_resp_data() detects that the stream's flow control credit
   is exhausted and declines. It sets H2_SF_BLK_SFCTL but doesn't delete
   the element from the send list since send_wait is NULL

6) another stream still has credit and fills the connection's buffer with
   its data.

7) the server the first stream is associated to delivers more data while the
   stream is still waiting for a window update, and attempts to send again
   via h2_snd_buf()

8) h2_snd_buf() doesn't detect that the stream is blocked because the check
   on sending_list doesn't match, so it tries again and sets send_wait to
   NULL again, then tries to send. It fails again because the connection's
   buffer is full

9) si_cs_send() above calls h2_subsribe() again to set send_wait and try
   to add the stream to the send_list. But since the stream has the SFCTL
   flag it's not added again, and it remains in the send_list where it
   still is.

10) a window update finally arrives for this stream (or an initial window
    size update for all streams). h2c_handle_window_update() is called,
    sees the stream has the SFCTL flag, removes it and appends it to the
    end of the send_list, where it already was.

11) h2_process_mux() scans the list, finds the first reference to the list
    element (where it was first added), and wakes it up. When going to the
    next one however, it directly jumps to the one that was added past the
    second addition, ignoring the orphans in the list that sit between the
    first place of the element and its last one.

12) h2_snd_buf() is called again for this element, then deletes it from the
    list, which has the consequence of unlinking the element from its last
    predecessor and successors, but not from the first one. Once the element
    is deleted, it's initialized and its next points to itself (an empty
    list head).

13) h2_process_mux() scans the list again to process more streams, and
    reaches the first predecessor of this element, then visits the next
    which is this element again, looping onto itself.

I certainly made a few reasoning mistakes above but I don't see anything
in the code preventing this case from happening.

Thus I'd like you to try the attached patch which is supposed to prevent
this scenario from happening. At least I've verified that it doesn't
break the h2spec test suite.

Thanks,
Willy
diff --git a/src/mux_h2.c b/src/mux_h2.c
index 67a297a..e2b1533 100644
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -1484,9 +1484,10 @@ static void h2c_update_all_ws(struct h2c *h2c, int diff)
 
                if (h2s->mws > 0 && (h2s->flags & H2_SF_BLK_SFCTL)) {
                        h2s->flags &= ~H2_SF_BLK_SFCTL;
-                       if (h2s->send_wait)
+                       if (h2s->send_wait) {
+                               LIST_DEL(&h2s->list);
                                LIST_ADDQ(&h2c->send_list, &h2s->list);
-
+                       }
                }
 
                node = eb32_next(node);
@@ -1791,8 +1792,10 @@ static int h2c_handle_window_update(struct h2c *h2c, 
struct h2s *h2s)
                h2s->mws += inc;
                if (h2s->mws > 0 && (h2s->flags & H2_SF_BLK_SFCTL)) {
                        h2s->flags &= ~H2_SF_BLK_SFCTL;
-                       if (h2s->send_wait)
+                       if (h2s->send_wait) {
+                               LIST_DEL(&h2s->list);
                                LIST_ADDQ(&h2c->send_list, &h2s->list);
+                       }
 
                }
        }
@@ -4144,10 +4147,7 @@ static size_t h2s_frt_make_resp_data(struct h2s *h2s, 
const struct buffer *buf,
 
        if (size <= 0) {
                h2s->flags |= H2_SF_BLK_SFCTL;
-               if (h2s->send_wait) {
-                       LIST_DEL(&h2s->list);
-                       LIST_INIT(&h2s->list);
-               }
+               LIST_DEL_INIT(&h2s->list);
                goto end;
        }
 
@@ -4897,10 +4897,7 @@ static size_t h2s_htx_frt_make_resp_data(struct h2s 
*h2s, struct buffer *buf, si
 
        if (h2s->mws <= 0) {
                h2s->flags |= H2_SF_BLK_SFCTL;
-               if (h2s->send_wait) {
-                       LIST_DEL(&h2s->list);
-                       LIST_INIT(&h2s->list);
-               }
+               LIST_DEL_INIT(&h2s->list);
                goto end;
        }
 

Reply via email to