Le 19/07/2017 à 22:18, Christopher Faulet a écrit :
Because these last weeks, there were several regressions on this part (the end of the HTTP transaction), I prefer to be careful this time. Every time I fixed a bug in one side, this broke something else from another one...
And because I said that, it happens. I introduced a major bug. It is possible to have an infinite loop during states synchronization...
Willy, I produced the patch for the upstream: 0001-BUG-MAJOR-http-Fix-possible-infinity-loop-in-http_sy.patch I also modified the patch for HAProxy 1.7. Sorry guys for this very silly bug ! -- Christopher
>From 66cfddf44ef95ae6504910739711984ab5d89239 Mon Sep 17 00:00:00 2001 From: Christopher Faulet <[email protected]> Date: Tue, 18 Jul 2017 11:18:46 +0200 Subject: [PATCH] BUG: Fix bug about wrong termination state on some requests introduced in 1.7.5 This pacth contains following fixes coming from the upstream. - * - BUG/MAJOR: http: Fix possible infinity loop in http_sync_(req|res)_state In commit "MINOR: http: Switch requests/responses in TUNNEL mode only by checking txn flags", it is possible to have an infinite loop on HTTP_MSG_CLOSING state. - * - MINOR: http: Reorder/rewrite checks in http_resync_states The previous patch removed the forced symmetry of the TUNNEL mode during the state synchronization. Here, we take care to remove body analyzer only on the channel in TUNNEL mode. In fact, today, this change has no effect because both sides are switched in same time. But this way, with some changes, it will be possible to keep body analyzer on a side (to finish the states synchronization) with the other one in TUNNEL mode. WARNING: This patch will be used to fix a bug. The fix will be commited in a very next commit. So if the fix is backported, this one must be backported too. - * - MINOR: http: Switch requests/responses in TUNNEL mode only by checking txn flags Today, the only way to have a request or a response in HTTP_MSG_TUNNEL state is to have the flag TX_CON_WANT_TUN set on the transaction. So this is a symmetric state. Both the request and the response are switch in same time in this state. This can be done only by checking transaction flags instead of relying on the other side state. This is the purpose of this patch. This way, if for any reason we need to switch only one side in TUNNEL mode, it will be possible. And to prepare asymmetric cases, we check channel flags in DONE _AND_ TUNNEL states. WARNING: This patch will be used to fix a bug. The fix will be commited in a very next commit. So if the fix is backported, this one must be backported too. - * - BUG/MEDIUM: http: Switch HTTP responses in TUNNEL mode when body length is undefined When the body length of a HTTP response is undefined, the HTTP parser is blocked in the body parsing. Before HAProxy 1.7, in this case, because AN_RES_HTTP_XFER_BODY is never set, there is no visible effect. When the server closes its connection to terminate the response, HAProxy catches it as a normal closure. Since 1.7, we always set this analyzer to enter at least once in http_response_forward_body. But, in the present case, when the server connection is closed, http_response_forward_body is called one time too many. The response is correctly sent to the client, but an error is catched and logged with "SD--" flags. To reproduce the bug, you can use the configuration "tests/test-fsm.cfg". The tests 3 and 21 hit the bug. Idea to fix the bug is to switch the response in TUNNEL mode without switching the request. This is possible because of previous patches. First, we need to detect responses with undefined body length during states synchronization. Excluding tunnelled transactions, when the response length is undefined, TX_CON_WANT_CLO is always set on the transaction. So, when states are synchronized, if TX_CON_WANT_CLO is set, the response is switched in TUNNEL mode and the request remains unchanged. Then, in http_msg_forward_body, we add a specific check to switch the response in DONE mode if the body length is undefined and if there is no data filter. This patch depends on following previous commits: * MINOR: http: Switch requests/responses in TUNNEL mode only by checking txn flags * MINOR: http: Reorder/rewrite checks in http_resync_states This patch must be backported in 1.7 with 2 previous ones. --- src/proto_http.c | 154 ++++++++++++++++++++++++++++--------------------------- 1 file changed, 78 insertions(+), 76 deletions(-) diff --git a/src/proto_http.c b/src/proto_http.c index 94c8d639..1a36dc6f 100644 --- a/src/proto_http.c +++ b/src/proto_http.c @@ -5294,7 +5294,7 @@ int http_sync_req_state(struct stream *s) unsigned int old_flags = chn->flags; unsigned int old_state = txn->req.msg_state; - if (unlikely(txn->req.msg_state < HTTP_MSG_BODY)) + if (unlikely(txn->req.msg_state < HTTP_MSG_DONE)) return 0; if (txn->req.msg_state == HTTP_MSG_DONE) { @@ -5338,13 +5338,6 @@ int http_sync_req_state(struct stream *s) goto wait_other_side; } - if (txn->rsp.msg_state == HTTP_MSG_TUNNEL) { - /* if any side switches to tunnel mode, the other one does too */ - channel_auto_read(chn); - txn->req.msg_state = HTTP_MSG_TUNNEL; - goto wait_other_side; - } - /* When we get here, it means that both the request and the * response have finished receiving. Depending on the connection * mode, we'll have to wait for the last bytes to leave in either @@ -5361,7 +5354,16 @@ int http_sync_req_state(struct stream *s) * let's enforce it now that we're not expecting any new * data to come. The caller knows the stream is complete * once both states are CLOSED. + * + * However, there is an exception if the response + * length is undefined. In this case, we need to wait + * the close from the server. The response will be + * switched in TUNNEL mode until the end. */ + if (!(txn->rsp.flags & HTTP_MSGF_XFER_LEN) && + txn->rsp.msg_state != HTTP_MSG_CLOSED) + goto check_channel_flags; + if (!(chn->flags & (CF_SHUTW|CF_SHUTW_NOW))) { channel_shutr_now(chn); channel_shutw_now(chn); @@ -5377,20 +5379,7 @@ int http_sync_req_state(struct stream *s) } } - if (chn->flags & (CF_SHUTW|CF_SHUTW_NOW)) { - /* if we've just closed an output, let's switch */ - s->si[1].flags |= SI_FL_NOLINGER; /* we want to close ASAP */ - - if (!channel_is_empty(chn)) { - txn->req.msg_state = HTTP_MSG_CLOSING; - goto http_msg_closing; - } - else { - txn->req.msg_state = HTTP_MSG_CLOSED; - goto http_msg_closed; - } - } - goto wait_other_side; + goto check_channel_flags; } if (txn->req.msg_state == HTTP_MSG_CLOSING) { @@ -5405,8 +5394,8 @@ int http_sync_req_state(struct stream *s) else if (chn->flags & CF_SHUTW) { txn->req.err_state = txn->req.msg_state; txn->req.msg_state = HTTP_MSG_ERROR; - goto wait_other_side; } + goto wait_other_side; } if (txn->req.msg_state == HTTP_MSG_CLOSED) { @@ -5419,6 +5408,16 @@ int http_sync_req_state(struct stream *s) goto wait_other_side; } + check_channel_flags: + /* Here, we are in HTTP_MSG_DONE or HTTP_MSG_TUNNEL */ + if (chn->flags & (CF_SHUTW|CF_SHUTW_NOW)) { + /* if we've just closed an output, let's switch */ + s->si[1].flags |= SI_FL_NOLINGER; /* we want to close ASAP */ + txn->req.msg_state = HTTP_MSG_CLOSING; + goto http_msg_closing; + } + + wait_other_side: return txn->req.msg_state != old_state || chn->flags != old_flags; } @@ -5438,7 +5437,7 @@ int http_sync_res_state(struct stream *s) unsigned int old_flags = chn->flags; unsigned int old_state = txn->rsp.msg_state; - if (unlikely(txn->rsp.msg_state < HTTP_MSG_BODY)) + if (unlikely(txn->rsp.msg_state < HTTP_MSG_DONE)) return 0; if (txn->rsp.msg_state == HTTP_MSG_DONE) { @@ -5461,14 +5460,6 @@ int http_sync_res_state(struct stream *s) goto wait_other_side; } - if (txn->req.msg_state == HTTP_MSG_TUNNEL) { - /* if any side switches to tunnel mode, the other one does too */ - channel_auto_read(chn); - txn->rsp.msg_state = HTTP_MSG_TUNNEL; - chn->flags |= CF_NEVER_WAIT; - goto wait_other_side; - } - /* When we get here, it means that both the request and the * response have finished receiving. Depending on the connection * mode, we'll have to wait for the last bytes to leave in either @@ -5489,8 +5480,16 @@ int http_sync_res_state(struct stream *s) * let's enforce it now that we're not expecting any new * data to come. The caller knows the stream is complete * once both states are CLOSED. + * + * However, there is an exception if the response length + * is undefined. In this case, we switch in TUNNEL mode. */ - if (!(chn->flags & (CF_SHUTW|CF_SHUTW_NOW))) { + if (!(txn->rsp.flags & HTTP_MSGF_XFER_LEN)) { + channel_auto_read(chn); + txn->rsp.msg_state = HTTP_MSG_TUNNEL; + chn->flags |= CF_NEVER_WAIT; + } + else if (!(chn->flags & (CF_SHUTW|CF_SHUTW_NOW))) { channel_shutr_now(chn); channel_shutw_now(chn); } @@ -5506,18 +5505,7 @@ int http_sync_res_state(struct stream *s) txn->rsp.msg_state = HTTP_MSG_TUNNEL; } - if (chn->flags & (CF_SHUTW|CF_SHUTW_NOW)) { - /* if we've just closed an output, let's switch */ - if (!channel_is_empty(chn)) { - txn->rsp.msg_state = HTTP_MSG_CLOSING; - goto http_msg_closing; - } - else { - txn->rsp.msg_state = HTTP_MSG_CLOSED; - goto http_msg_closed; - } - } - goto wait_other_side; + goto check_channel_flags; } if (txn->rsp.msg_state == HTTP_MSG_CLOSING) { @@ -5535,8 +5523,8 @@ int http_sync_res_state(struct stream *s) s->be->be_counters.cli_aborts++; if (objt_server(s->target)) objt_server(s->target)->counters.cli_aborts++; - goto wait_other_side; } + goto wait_other_side; } if (txn->rsp.msg_state == HTTP_MSG_CLOSED) { @@ -5548,6 +5536,14 @@ int http_sync_res_state(struct stream *s) goto wait_other_side; } + check_channel_flags: + /* Here, we are in HTTP_MSG_DONE or HTTP_MSG_TUNNEL */ + if (chn->flags & (CF_SHUTW|CF_SHUTW_NOW)) { + /* if we've just closed an output, let's switch */ + txn->rsp.msg_state = HTTP_MSG_CLOSING; + goto http_msg_closing; + } + wait_other_side: /* We force the response to leave immediately if we're waiting for the * other side, since there is no pending shutdown to push it out. @@ -5577,34 +5573,27 @@ int http_resync_states(struct stream *s) /* OK, both state machines agree on a compatible state. * There are a few cases we're interested in : - * - HTTP_MSG_TUNNEL on either means we have to disable both analysers * - HTTP_MSG_CLOSED on both sides means we've reached the end in both * directions, so let's simply disable both analysers. - * - HTTP_MSG_CLOSED on the response only means we must abort the - * request. - * - HTTP_MSG_CLOSED on the request and HTTP_MSG_DONE on the response - * with server-close mode means we've completed one request and we - * must re-initialize the server connection. + * - HTTP_MSG_CLOSED on the response only or HTTP_MSG_ERROR on either + * means we must abort the request. + * - HTTP_MSG_TUNNEL on either means we have to disable analyser on + * corresponding channel. + * - HTTP_MSG_DONE or HTTP_MSG_CLOSED on the request and HTTP_MSG_DONE + * on the response with server-close mode means we've completed one + * request and we must re-initialize the server connection. */ - - if (txn->req.msg_state == HTTP_MSG_TUNNEL || - txn->rsp.msg_state == HTTP_MSG_TUNNEL || - (txn->req.msg_state == HTTP_MSG_CLOSED && - txn->rsp.msg_state == HTTP_MSG_CLOSED)) { + if (txn->req.msg_state == HTTP_MSG_CLOSED && + txn->rsp.msg_state == HTTP_MSG_CLOSED) { s->req.analysers &= AN_REQ_FLT_END; channel_auto_close(&s->req); channel_auto_read(&s->req); s->res.analysers &= AN_RES_FLT_END; channel_auto_close(&s->res); channel_auto_read(&s->res); - if (txn->req.msg_state == HTTP_MSG_TUNNEL && HAS_REQ_DATA_FILTERS(s)) - s->req.analysers |= AN_REQ_FLT_XFER_DATA; - if (txn->rsp.msg_state == HTTP_MSG_TUNNEL && HAS_RSP_DATA_FILTERS(s)) - s->res.analysers |= AN_RES_FLT_XFER_DATA; - } - else if ((txn->req.msg_state >= HTTP_MSG_DONE && - (txn->rsp.msg_state == HTTP_MSG_CLOSED || (s->res.flags & CF_SHUTW))) || - txn->rsp.msg_state == HTTP_MSG_ERROR || + } + else if (txn->rsp.msg_state == HTTP_MSG_CLOSED || + txn->rsp.msg_state == HTTP_MSG_ERROR || txn->req.msg_state == HTTP_MSG_ERROR) { s->res.analysers &= AN_RES_FLT_END; channel_auto_close(&s->res); @@ -5615,6 +5604,23 @@ int http_resync_states(struct stream *s) channel_auto_read(&s->req); channel_truncate(&s->req); } + else if (txn->req.msg_state == HTTP_MSG_TUNNEL || + txn->rsp.msg_state == HTTP_MSG_TUNNEL) { + if (txn->req.msg_state == HTTP_MSG_TUNNEL) { + s->req.analysers &= AN_REQ_FLT_END; + if (HAS_REQ_DATA_FILTERS(s)) + s->req.analysers |= AN_REQ_FLT_XFER_DATA; + } + if (txn->rsp.msg_state == HTTP_MSG_TUNNEL) { + s->res.analysers &= AN_RES_FLT_END; + if (HAS_RSP_DATA_FILTERS(s)) + s->res.analysers |= AN_RES_FLT_XFER_DATA; + } + channel_auto_close(&s->req); + channel_auto_read(&s->req); + channel_auto_close(&s->res); + channel_auto_read(&s->res); + } else if ((txn->req.msg_state == HTTP_MSG_DONE || txn->req.msg_state == HTTP_MSG_CLOSED) && txn->rsp.msg_state == HTTP_MSG_DONE && @@ -6963,14 +6969,6 @@ int http_response_forward_body(struct stream *s, struct channel *res, int an_bit if ((msg->flags & HTTP_MSGF_TE_CHNK) || (msg->flags & HTTP_MSGF_COMPRESSING)) res->flags |= CF_EXPECT_MORE; - /* If there is neither content-length, nor transfer-encoding header - * _AND_ there is no data filtering, we can safely forward all data - * indefinitely. */ - if (!(msg->flags & HTTP_MSGF_XFER_LEN) && !HAS_DATA_FILTERS(s, res)) { - buffer_flush(res->buf); - channel_forward_forever(res); - } - /* the stream handler will take care of timeouts and errors */ return 0; @@ -7047,9 +7045,13 @@ http_msg_forward_body(struct stream *s, struct http_msg *msg) goto missing_data_or_waiting; } - /* The server still sending data that should be filtered */ - if (!(msg->flags & HTTP_MSGF_XFER_LEN) && !(chn->flags & CF_SHUTR)) - goto missing_data_or_waiting; + /* This check can only be true for a response. HTTP_MSGF_XFER_LEN is + * always set for a request. */ + if (!(msg->flags & HTTP_MSGF_XFER_LEN)) { + /* The server still sending data that should be filtered */ + if (!(chn->flags & CF_SHUTR) && HAS_DATA_FILTERS(s, chn)) + goto missing_data_or_waiting; + } msg->msg_state = HTTP_MSG_ENDING; -- 2.13.3
>From f17638ebb1ea691f072fc355cbcae5b22df18438 Mon Sep 17 00:00:00 2001 From: Christopher Faulet <[email protected]> Date: Thu, 20 Jul 2017 11:05:10 +0200 Subject: [PATCH] BUG/MAJOR: http: Fix possible infinity loop in http_sync_(req|res)_state In commit "MINOR: http: Switch requests/responses in TUNNEL mode only by checking txn flags", it is possible to have an infinite loop on HTTP_MSG_CLOSING state. --- src/proto_http.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/proto_http.c b/src/proto_http.c index f9dc8a1b..e72e7e96 100644 --- a/src/proto_http.c +++ b/src/proto_http.c @@ -5413,8 +5413,8 @@ int http_sync_req_state(struct stream *s) else if (chn->flags & CF_SHUTW) { txn->req.err_state = txn->req.msg_state; txn->req.msg_state = HTTP_MSG_ERROR; - goto wait_other_side; } + goto wait_other_side; } if (txn->req.msg_state == HTTP_MSG_CLOSED) { @@ -5542,8 +5542,8 @@ int http_sync_res_state(struct stream *s) s->be->be_counters.cli_aborts++; if (objt_server(s->target)) objt_server(s->target)->counters.cli_aborts++; - goto wait_other_side; } + goto wait_other_side; } if (txn->rsp.msg_state == HTTP_MSG_CLOSED) { -- 2.13.3
