From 93e02d8b73f3dc31e85c5029def8647d5e5e9136 Mon Sep 17 00:00:00 2001 From: Christopher Faulet Date: Fri, 8 Mar 2019 14:18:50 +0100 Subject: [PATCH] MINOR: proto-http/proto-htx: Make error handling clearer during data forwarding It is just a cleanup. Error handling is grouped at the end HTTP data analysers. This patch must be backported to 1.9 because it is used by another patch to fix a bug. --- src/proto_http.c | 158 ++++++++++++++++++----------------------------- src/proto_htx.c | 153 ++++++++++++++++++--------------------------- 2 files changed, 120 insertions(+), 191 deletions(-) diff --git a/src/proto_http.c b/src/proto_http.c index 4c9e609b35..0dd8289904 100644 --- a/src/proto_http.c +++ b/src/proto_http.c @@ -3965,6 +3965,7 @@ int http_request_forward_body(struct stream *s, struct channel *req, int an_bit) struct session *sess = s->sess; struct http_txn *txn = s->txn; struct http_msg *msg = &s->txn->req; + short status = 0; int ret; if (IS_HTX_STRM(s)) @@ -3990,7 +3991,6 @@ int http_request_forward_body(struct stream *s, struct channel *req, int an_bit) msg->err_state = msg->msg_state; msg->msg_state = HTTP_MSG_ERROR; http_resync_states(s); - return 1; } /* Note that we don't have to send 100-continue back because we don't @@ -4054,7 +4054,7 @@ int http_request_forward_body(struct stream *s, struct channel *req, int an_bit) if (req->flags & CF_SHUTW) { /* request errors are most likely due to the * server aborting the transfer. */ - goto aborted_xfer; + goto return_srv_abort; } if (msg->err_pos >= 0) http_capture_bad_message(sess->fe, s, msg, msg->err_state, s->be); @@ -4085,28 +4085,13 @@ int http_request_forward_body(struct stream *s, struct channel *req, int an_bit) missing_data_or_waiting: /* stop waiting for data if the input is closed before the end */ - if (msg->msg_state < HTTP_MSG_ENDING && req->flags & CF_SHUTR) { - if (!(s->flags & SF_ERR_MASK)) - s->flags |= SF_ERR_CLICL; - if (!(s->flags & SF_FINST_MASK)) { - if (txn->rsp.msg_state < HTTP_MSG_ERROR) - s->flags |= SF_FINST_H; - else - s->flags |= SF_FINST_D; - } - - _HA_ATOMIC_ADD(&sess->fe->fe_counters.cli_aborts, 1); - _HA_ATOMIC_ADD(&s->be->be_counters.cli_aborts, 1); - if (objt_server(s->target)) - _HA_ATOMIC_ADD(&objt_server(s->target)->counters.cli_aborts, 1); - - goto return_bad_req_stats_ok; - } + if (msg->msg_state < HTTP_MSG_ENDING && req->flags & CF_SHUTR) + goto return_cli_abort; waiting: /* waiting for the last bits to leave the buffer */ if (req->flags & CF_SHUTW) - goto aborted_xfer; + goto return_srv_abort; /* When TE: chunked is used, we need to get there again to parse remaining * chunks even if the client has closed, so we don't want to set CF_DONTCLOSE. @@ -4132,60 +4117,48 @@ int http_request_forward_body(struct stream *s, struct channel *req, int an_bit) return 0; - return_bad_req: /* let's centralize all bad requests */ + return_cli_abort: + _HA_ATOMIC_ADD(&sess->fe->fe_counters.cli_aborts, 1); + _HA_ATOMIC_ADD(&s->be->be_counters.cli_aborts, 1); + if (objt_server(s->target)) + _HA_ATOMIC_ADD(&objt_server(s->target)->counters.cli_aborts, 1); + if (!(s->flags & SF_ERR_MASK)) + s->flags |= SF_ERR_CLICL; + status = 400; + goto return_error; + + return_srv_abort: + _HA_ATOMIC_ADD(&sess->fe->fe_counters.srv_aborts, 1); + _HA_ATOMIC_ADD(&s->be->be_counters.srv_aborts, 1); + if (objt_server(s->target)) + _HA_ATOMIC_ADD(&objt_server(s->target)->counters.srv_aborts, 1); + if (!(s->flags & SF_ERR_MASK)) + s->flags |= SF_ERR_SRVCL; + status = 502; + goto return_error; + + return_bad_req: /* let's centralize all bad requests */ _HA_ATOMIC_ADD(&sess->fe->fe_counters.failed_req, 1); if (sess->listener->counters) _HA_ATOMIC_ADD(&sess->listener->counters->failed_req, 1); - - return_bad_req_stats_ok: - txn->req.err_state = txn->req.msg_state; - txn->req.msg_state = HTTP_MSG_ERROR; - if (txn->status) { - /* Note: we don't send any error if some data were already sent */ - http_reply_and_close(s, txn->status, NULL); - } else { - txn->status = 400; - http_reply_and_close(s, txn->status, http_error_message(s)); - } - req->analysers &= AN_REQ_FLT_END; - s->res.analysers &= AN_RES_FLT_END; /* we're in data phase, we want to abort both directions */ - if (!(s->flags & SF_ERR_MASK)) - s->flags |= SF_ERR_PRXCOND; - if (!(s->flags & SF_FINST_MASK)) { - if (txn->rsp.msg_state < HTTP_MSG_ERROR) - s->flags |= SF_FINST_H; - else - s->flags |= SF_FINST_D; - } - return 0; + s->flags |= SF_ERR_CLICL; + status = 400; - aborted_xfer: + return_error: txn->req.err_state = txn->req.msg_state; txn->req.msg_state = HTTP_MSG_ERROR; - if (txn->status) { + if (txn->status > 0) { /* Note: we don't send any error if some data were already sent */ http_reply_and_close(s, txn->status, NULL); } else { - txn->status = 502; + txn->status = status; http_reply_and_close(s, txn->status, http_error_message(s)); } req->analysers &= AN_REQ_FLT_END; s->res.analysers &= AN_RES_FLT_END; /* we're in data phase, we want to abort both directions */ - - _HA_ATOMIC_ADD(&sess->fe->fe_counters.srv_aborts, 1); - _HA_ATOMIC_ADD(&s->be->be_counters.srv_aborts, 1); - if (objt_server(s->target)) - _HA_ATOMIC_ADD(&objt_server(s->target)->counters.srv_aborts, 1); - - if (!(s->flags & SF_ERR_MASK)) - s->flags |= SF_ERR_SRVCL; - if (!(s->flags & SF_FINST_MASK)) { - if (txn->rsp.msg_state < HTTP_MSG_ERROR) - s->flags |= SF_FINST_H; - else - s->flags |= SF_FINST_D; - } + if (!(s->flags & SF_FINST_MASK)) + s->flags |= ((txn->rsp.msg_state < HTTP_MSG_ERROR) ? SF_FINST_H : SF_FINST_D); return 0; } @@ -5222,7 +5195,7 @@ int http_response_forward_body(struct stream *s, struct channel *res, int an_bit if ((res->flags & (CF_READ_ERROR|CF_READ_TIMEOUT|CF_WRITE_ERROR|CF_WRITE_TIMEOUT)) || ((res->flags & CF_SHUTW) && (res->to_forward || co_data(res))) || - !s->req.analysers) { + !s->req.analysers) { /* Output closed while we were sending data. We must abort and * wake the other side up. */ @@ -5269,7 +5242,7 @@ int http_response_forward_body(struct stream *s, struct channel *res, int an_bit if (res->flags & CF_SHUTW) { /* response errors are most likely due to the * client aborting the transfer. */ - goto aborted_xfer; + goto return_cli_abort; } if (msg->err_pos >= 0) http_capture_bad_message(s->be, s, msg, msg->err_state, strm_fe(s)); @@ -5281,7 +5254,7 @@ int http_response_forward_body(struct stream *s, struct channel *res, int an_bit missing_data_or_waiting: if (res->flags & CF_SHUTW) - goto aborted_xfer; + goto return_cli_abort; /* stop waiting for data if the input is closed before the end. If the * client side was already closed, it means that the client has aborted, @@ -5290,17 +5263,10 @@ int http_response_forward_body(struct stream *s, struct channel *res, int an_bit */ if (msg->msg_state < HTTP_MSG_ENDING && res->flags & CF_SHUTR) { if ((s->req.flags & (CF_SHUTR|CF_SHUTW)) == (CF_SHUTR|CF_SHUTW)) - goto aborted_xfer; + goto return_cli_abort; /* If we have some pending data, we continue the processing */ - if (!ci_data(res)) { - if (!(s->flags & SF_ERR_MASK)) - s->flags |= SF_ERR_SRVCL; - _HA_ATOMIC_ADD(&sess->fe->fe_counters.srv_aborts, 1); - _HA_ATOMIC_ADD(&s->be->be_counters.srv_aborts, 1); - if (objt_server(s->target)) - _HA_ATOMIC_ADD(&objt_server(s->target)->counters.srv_aborts, 1); - goto return_bad_res_stats_ok; - } + if (!ci_data(res)) + goto return_srv_abort; } /* we need to obey the req analyser, so if it leaves, we must too */ @@ -5333,42 +5299,40 @@ int http_response_forward_body(struct stream *s, struct channel *res, int an_bit /* the stream handler will take care of timeouts and errors */ return 0; - return_bad_res: /* let's centralize all bad responses */ - _HA_ATOMIC_ADD(&s->be->be_counters.failed_resp, 1); + return_srv_abort: + _HA_ATOMIC_ADD(&sess->fe->fe_counters.srv_aborts, 1); + _HA_ATOMIC_ADD(&s->be->be_counters.srv_aborts, 1); if (objt_server(s->target)) - _HA_ATOMIC_ADD(&objt_server(s->target)->counters.failed_resp, 1); + _HA_ATOMIC_ADD(&objt_server(s->target)->counters.srv_aborts, 1); + if (!(s->flags & SF_ERR_MASK)) + s->flags |= SF_ERR_SRVCL; + goto return_error; - return_bad_res_stats_ok: - txn->rsp.err_state = txn->rsp.msg_state; - txn->rsp.msg_state = HTTP_MSG_ERROR; - /* don't send any error message as we're in the body */ - http_reply_and_close(s, txn->status, NULL); - res->analysers &= AN_RES_FLT_END; - s->req.analysers &= AN_REQ_FLT_END; /* we're in data phase, we want to abort both directions */ + return_cli_abort: + _HA_ATOMIC_ADD(&sess->fe->fe_counters.cli_aborts, 1); + _HA_ATOMIC_ADD(&s->be->be_counters.cli_aborts, 1); if (objt_server(s->target)) - health_adjust(__objt_server(s->target), HANA_STATUS_HTTP_HDRRSP); + _HA_ATOMIC_ADD(&objt_server(s->target)->counters.cli_aborts, 1); + if (!(s->flags & SF_ERR_MASK)) + s->flags |= SF_ERR_CLICL; + goto return_error; + return_bad_res: + _HA_ATOMIC_ADD(&s->be->be_counters.failed_resp, 1); + if (objt_server(s->target)) { + _HA_ATOMIC_ADD(&objt_server(s->target)->counters.failed_resp, 1); + health_adjust(__objt_server(s->target), HANA_STATUS_HTTP_RSP); + } if (!(s->flags & SF_ERR_MASK)) - s->flags |= SF_ERR_PRXCOND; - if (!(s->flags & SF_FINST_MASK)) - s->flags |= SF_FINST_D; - return 0; + s->flags |= SF_ERR_SRVCL; - aborted_xfer: + return_error: txn->rsp.err_state = txn->rsp.msg_state; txn->rsp.msg_state = HTTP_MSG_ERROR; /* don't send any error message as we're in the body */ http_reply_and_close(s, txn->status, NULL); res->analysers &= AN_RES_FLT_END; s->req.analysers &= AN_REQ_FLT_END; /* we're in data phase, we want to abort both directions */ - - _HA_ATOMIC_ADD(&sess->fe->fe_counters.cli_aborts, 1); - _HA_ATOMIC_ADD(&s->be->be_counters.cli_aborts, 1); - if (objt_server(s->target)) - _HA_ATOMIC_ADD(&objt_server(s->target)->counters.cli_aborts, 1); - - if (!(s->flags & SF_ERR_MASK)) - s->flags |= SF_ERR_CLICL; if (!(s->flags & SF_FINST_MASK)) s->flags |= SF_FINST_D; return 0; diff --git a/src/proto_htx.c b/src/proto_htx.c index f9e35e5c89..f5e2e73d2a 100644 --- a/src/proto_htx.c +++ b/src/proto_htx.c @@ -1170,6 +1170,7 @@ int htx_request_forward_body(struct stream *s, struct channel *req, int an_bit) struct http_txn *txn = s->txn; struct http_msg *msg = &txn->req; struct htx *htx; + short status = 0; int ret; DPRINTF(stderr,"[%u] %s: stream=%p b=%p, exp(r,w)=%u,%u bf=%08x bh=%lu analysers=%02x\n", @@ -1282,7 +1283,7 @@ int htx_request_forward_body(struct stream *s, struct channel *req, int an_bit) if (req->flags & CF_SHUTW) { /* request errors are most likely due to the * server aborting the transfer. */ - goto aborted_xfer; + goto return_srv_abort; } goto return_bad_req; } @@ -1311,28 +1312,13 @@ int htx_request_forward_body(struct stream *s, struct channel *req, int an_bit) missing_data_or_waiting: /* stop waiting for data if the input is closed before the end */ - if (msg->msg_state < HTTP_MSG_DONE && req->flags & CF_SHUTR) { - if (!(s->flags & SF_ERR_MASK)) - s->flags |= SF_ERR_CLICL; - if (!(s->flags & SF_FINST_MASK)) { - if (txn->rsp.msg_state < HTTP_MSG_ERROR) - s->flags |= SF_FINST_H; - else - s->flags |= SF_FINST_D; - } - - _HA_ATOMIC_ADD(&sess->fe->fe_counters.cli_aborts, 1); - _HA_ATOMIC_ADD(&s->be->be_counters.cli_aborts, 1); - if (objt_server(s->target)) - _HA_ATOMIC_ADD(&objt_server(s->target)->counters.cli_aborts, 1); - - goto return_bad_req_stats_ok; - } + if (msg->msg_state < HTTP_MSG_DONE && req->flags & CF_SHUTR) + goto return_cli_abort; waiting: /* waiting for the last bits to leave the buffer */ if (req->flags & CF_SHUTW) - goto aborted_xfer; + goto return_srv_abort; if (htx->flags & HTX_FL_PARSING_ERROR) goto return_bad_req; @@ -1361,60 +1347,48 @@ int htx_request_forward_body(struct stream *s, struct channel *req, int an_bit) return 0; - return_bad_req: /* let's centralize all bad requests */ + return_cli_abort: + _HA_ATOMIC_ADD(&sess->fe->fe_counters.cli_aborts, 1); + _HA_ATOMIC_ADD(&s->be->be_counters.cli_aborts, 1); + if (objt_server(s->target)) + _HA_ATOMIC_ADD(&objt_server(s->target)->counters.cli_aborts, 1); + if (!(s->flags & SF_ERR_MASK)) + s->flags |= SF_ERR_CLICL; + status = 400; + goto return_error; + + return_srv_abort: + _HA_ATOMIC_ADD(&sess->fe->fe_counters.srv_aborts, 1); + _HA_ATOMIC_ADD(&s->be->be_counters.srv_aborts, 1); + if (objt_server(s->target)) + _HA_ATOMIC_ADD(&objt_server(s->target)->counters.srv_aborts, 1); + if (!(s->flags & SF_ERR_MASK)) + s->flags |= SF_ERR_SRVCL; + status = 502; + goto return_error; + + return_bad_req: _HA_ATOMIC_ADD(&sess->fe->fe_counters.failed_req, 1); if (sess->listener->counters) _HA_ATOMIC_ADD(&sess->listener->counters->failed_req, 1); - - return_bad_req_stats_ok: - txn->req.err_state = txn->req.msg_state; - txn->req.msg_state = HTTP_MSG_ERROR; - if (txn->status > 0) { - /* Note: we don't send any error if some data were already sent */ - htx_reply_and_close(s, txn->status, NULL); - } else { - txn->status = 400; - htx_reply_and_close(s, txn->status, htx_error_message(s)); - } - req->analysers &= AN_REQ_FLT_END; - s->res.analysers &= AN_RES_FLT_END; /* we're in data phase, we want to abort both directions */ - if (!(s->flags & SF_ERR_MASK)) - s->flags |= SF_ERR_PRXCOND; - if (!(s->flags & SF_FINST_MASK)) { - if (txn->rsp.msg_state < HTTP_MSG_ERROR) - s->flags |= SF_FINST_H; - else - s->flags |= SF_FINST_D; - } - return 0; + s->flags |= SF_ERR_CLICL; + status = 400; - aborted_xfer: + return_error: txn->req.err_state = txn->req.msg_state; txn->req.msg_state = HTTP_MSG_ERROR; if (txn->status > 0) { /* Note: we don't send any error if some data were already sent */ htx_reply_and_close(s, txn->status, NULL); } else { - txn->status = 502; + txn->status = status; htx_reply_and_close(s, txn->status, htx_error_message(s)); } req->analysers &= AN_REQ_FLT_END; s->res.analysers &= AN_RES_FLT_END; /* we're in data phase, we want to abort both directions */ - - _HA_ATOMIC_ADD(&sess->fe->fe_counters.srv_aborts, 1); - _HA_ATOMIC_ADD(&s->be->be_counters.srv_aborts, 1); - if (objt_server(s->target)) - _HA_ATOMIC_ADD(&objt_server(s->target)->counters.srv_aborts, 1); - - if (!(s->flags & SF_ERR_MASK)) - s->flags |= SF_ERR_SRVCL; - if (!(s->flags & SF_FINST_MASK)) { - if (txn->rsp.msg_state < HTTP_MSG_ERROR) - s->flags |= SF_FINST_H; - else - s->flags |= SF_FINST_D; - } + if (!(s->flags & SF_FINST_MASK)) + s->flags |= ((txn->rsp.msg_state < HTTP_MSG_ERROR) ? SF_FINST_H : SF_FINST_D); return 0; } @@ -2234,7 +2208,7 @@ int htx_response_forward_body(struct stream *s, struct channel *res, int an_bit) if (res->flags & CF_SHUTW) { /* response errors are most likely due to the * client aborting the transfer. */ - goto aborted_xfer; + goto return_cli_abort; } goto return_bad_res; } @@ -2244,7 +2218,7 @@ int htx_response_forward_body(struct stream *s, struct channel *res, int an_bit) missing_data_or_waiting: if (res->flags & CF_SHUTW) - goto aborted_xfer; + goto return_cli_abort; if (htx->flags & HTX_FL_PARSING_ERROR) goto return_bad_res; @@ -2256,17 +2230,10 @@ int htx_response_forward_body(struct stream *s, struct channel *res, int an_bit) */ if (msg->msg_state < HTTP_MSG_DONE && res->flags & CF_SHUTR) { if ((s->req.flags & (CF_SHUTR|CF_SHUTW)) == (CF_SHUTR|CF_SHUTW)) - goto aborted_xfer; + goto return_cli_abort; /* If we have some pending data, we continue the processing */ - if (htx_is_empty(htx)) { - if (!(s->flags & SF_ERR_MASK)) - s->flags |= SF_ERR_SRVCL; - _HA_ATOMIC_ADD(&sess->fe->fe_counters.srv_aborts, 1); - _HA_ATOMIC_ADD(&s->be->be_counters.srv_aborts, 1); - if (objt_server(s->target)) - _HA_ATOMIC_ADD(&objt_server(s->target)->counters.srv_aborts, 1); - goto return_bad_res_stats_ok; - } + if (htx_is_empty(htx)) + goto return_srv_abort; } /* When TE: chunked is used, we need to get there again to parse @@ -2292,42 +2259,40 @@ int htx_response_forward_body(struct stream *s, struct channel *res, int an_bit) /* the stream handler will take care of timeouts and errors */ return 0; - return_bad_res: /* let's centralize all bad responses */ - _HA_ATOMIC_ADD(&s->be->be_counters.failed_resp, 1); + return_srv_abort: + _HA_ATOMIC_ADD(&sess->fe->fe_counters.srv_aborts, 1); + _HA_ATOMIC_ADD(&s->be->be_counters.srv_aborts, 1); if (objt_server(s->target)) - _HA_ATOMIC_ADD(&objt_server(s->target)->counters.failed_resp, 1); + _HA_ATOMIC_ADD(&objt_server(s->target)->counters.srv_aborts, 1); + if (!(s->flags & SF_ERR_MASK)) + s->flags |= SF_ERR_SRVCL; + goto return_error; - return_bad_res_stats_ok: - txn->rsp.err_state = txn->rsp.msg_state; - txn->rsp.msg_state = HTTP_MSG_ERROR; - /* don't send any error message as we're in the body */ - htx_reply_and_close(s, txn->status, NULL); - res->analysers &= AN_RES_FLT_END; - s->req.analysers &= AN_REQ_FLT_END; /* we're in data phase, we want to abort both directions */ + return_cli_abort: + _HA_ATOMIC_ADD(&sess->fe->fe_counters.cli_aborts, 1); + _HA_ATOMIC_ADD(&s->be->be_counters.cli_aborts, 1); if (objt_server(s->target)) - health_adjust(__objt_server(s->target), HANA_STATUS_HTTP_HDRRSP); + _HA_ATOMIC_ADD(&objt_server(s->target)->counters.cli_aborts, 1); + if (!(s->flags & SF_ERR_MASK)) + s->flags |= SF_ERR_CLICL; + goto return_error; + return_bad_res: + _HA_ATOMIC_ADD(&s->be->be_counters.failed_resp, 1); + if (objt_server(s->target)) { + _HA_ATOMIC_ADD(&objt_server(s->target)->counters.failed_resp, 1); + health_adjust(__objt_server(s->target), HANA_STATUS_HTTP_RSP); + } if (!(s->flags & SF_ERR_MASK)) - s->flags |= SF_ERR_PRXCOND; - if (!(s->flags & SF_FINST_MASK)) - s->flags |= SF_FINST_D; - return 0; + s->flags |= SF_ERR_SRVCL; - aborted_xfer: + return_error: txn->rsp.err_state = txn->rsp.msg_state; txn->rsp.msg_state = HTTP_MSG_ERROR; /* don't send any error message as we're in the body */ htx_reply_and_close(s, txn->status, NULL); res->analysers &= AN_RES_FLT_END; s->req.analysers &= AN_REQ_FLT_END; /* we're in data phase, we want to abort both directions */ - - _HA_ATOMIC_ADD(&sess->fe->fe_counters.cli_aborts, 1); - _HA_ATOMIC_ADD(&s->be->be_counters.cli_aborts, 1); - if (objt_server(s->target)) - _HA_ATOMIC_ADD(&objt_server(s->target)->counters.cli_aborts, 1); - - if (!(s->flags & SF_ERR_MASK)) - s->flags |= SF_ERR_CLICL; if (!(s->flags & SF_FINST_MASK)) s->flags |= SF_FINST_D; return 0; -- 2.39.5