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;
}
