From f2b02cfd940eec2810590ef15b37631defd11b6b Mon Sep 17 00:00:00 2001 From: Christopher Faulet Date: Fri, 13 Jan 2023 11:02:28 +0100 Subject: [PATCH] MAJOR: http-ana: Review error handling during HTTP payload forwarding The error handling in the HTTP payload forwarding is far to be ideal because both sides (request and response) are tested each time. It is espcially ugly on the request side. To report a server error instead of a client error, there are some workarounds to delay the error handling. The reason is that the request analyzer is evaluated before the response one. In addition, errors are tested before the data analysis. It means it is possible to truncate data because errors may be handled to early. So the error handling at this stages was totally reviewed. Aborts are now handled after the data analysis. We also stop to finish the response on request error or the opposite. As a side effect, the HTTP_MSG_ERROR state is now useless. As another side effect, the termination flags are now set by the HTTP analysers and not process_stream(). --- .../http-messaging/http_request_buffer.vtc | 2 +- src/http_ana.c | 117 +++--------------- 2 files changed, 20 insertions(+), 99 deletions(-) diff --git a/reg-tests/http-messaging/http_request_buffer.vtc b/reg-tests/http-messaging/http_request_buffer.vtc index 15ec54045..302db4ab4 100644 --- a/reg-tests/http-messaging/http_request_buffer.vtc +++ b/reg-tests/http-messaging/http_request_buffer.vtc @@ -36,7 +36,7 @@ syslog S -level info { barrier b1 sync recv - expect ~ "[^:\\[ ]*\\[[0-9]*\\]: .* .* fe2 be1/ [0-9]*/-1/-1/-1/[0-9]* -1 .* - - CR-- .* .* \"POST /2 HTTP/1\\.1\"" + expect ~ "[^:\\[ ]*\\[[0-9]*\\]: .* .* fe2 be1/ [0-9]*/-1/-1/-1/[0-9]* 400 .* - - CR-- .* .* \"POST /2 HTTP/1\\.1\"" } -start haproxy h1 -conf { diff --git a/src/http_ana.c b/src/http_ana.c index 12dd48768..694956261 100644 --- a/src/http_ana.c +++ b/src/http_ana.c @@ -984,35 +984,6 @@ int http_request_forward_body(struct stream *s, struct channel *req, int an_bit) if (htx->flags & HTX_FL_PROCESSING_ERROR) goto return_int_err; - if ((req->flags & (CF_READ_ERROR|CF_READ_TIMEOUT|CF_WRITE_ERROR|CF_WRITE_TIMEOUT)) || - ((req->flags & CF_SHUTW) && (req->to_forward || co_data(req)))) { - /* Output closed while we were sending data. We must abort and - * wake the other side up. - * - * If we have finished to send the request and the response is - * still in progress, don't catch write error on the request - * side if it is in fact a read error on the server side. - */ - if (msg->msg_state == HTTP_MSG_DONE && (s->res.flags & CF_READ_ERROR) && s->res.analysers) - return 0; - - /* Don't abort yet if we had L7 retries activated and it - * was a write error, we may recover. - */ - if (!(req->flags & (CF_READ_ERROR | CF_READ_TIMEOUT)) && - (txn->flags & TX_L7_RETRY)) { - DBG_TRACE_DEVEL("leaving on L7 retry", - STRM_EV_STRM_ANA|STRM_EV_HTTP_ANA|STRM_EV_HTTP_ERR, s, txn); - return 0; - } - msg->msg_state = HTTP_MSG_ERROR; - http_end_request(s); - http_end_response(s); - DBG_TRACE_DEVEL("leaving on error", - STRM_EV_STRM_ANA|STRM_EV_HTTP_ANA|STRM_EV_HTTP_ERR, s, txn); - return 1; - } - /* Note that we don't have to send 100-continue back because we don't * need the data to complete our job, and it's up to the server to * decide whether to return 100, 417 or anything else in return of @@ -1100,17 +1071,14 @@ int http_request_forward_body(struct stream *s, struct channel *req, int an_bit) if (!(txn->flags & TX_CON_WANT_TUN)) channel_dont_close(req); + if ((req->flags & CF_SHUTW) && co_data(req)) { + /* request errors are most likely due to the server aborting the + * transfer. */ + goto return_srv_abort; + } + http_end_request(s); if (!(req->analysers & an_bit)) { - http_end_response(s); - if (unlikely(msg->msg_state == HTTP_MSG_ERROR)) { - if (req->flags & CF_SHUTW) { - /* request errors are most likely due to the - * server aborting the transfer. */ - goto return_srv_abort; - } - goto return_bad_req; - } DBG_TRACE_LEAVE(STRM_EV_STRM_ANA|STRM_EV_HTTP_ANA, s, txn); return 1; } @@ -1180,7 +1148,7 @@ int http_request_forward_body(struct stream *s, struct channel *req, int an_bit) if (objt_server(s->target)) _HA_ATOMIC_INC(&__objt_server(s->target)->counters.cli_aborts); if (!(s->flags & SF_ERR_MASK)) - s->flags |= SF_ERR_CLICL; + s->flags |= ((req->flags & CF_READ_TIMEOUT) ? SF_ERR_CLITO : SF_ERR_CLICL); status = 400; goto return_prx_cond; @@ -1192,7 +1160,7 @@ int http_request_forward_body(struct stream *s, struct channel *req, int an_bit) if (objt_server(s->target)) _HA_ATOMIC_INC(&__objt_server(s->target)->counters.srv_aborts); if (!(s->flags & SF_ERR_MASK)) - s->flags |= SF_ERR_SRVCL; + s->flags |= ((req->flags & CF_WRITE_TIMEOUT) ? SF_ERR_SRVTO : SF_ERR_SRVCL); status = 502; goto return_prx_cond; @@ -1223,10 +1191,7 @@ int http_request_forward_body(struct stream *s, struct channel *req, int an_bit) txn->status = status; http_reply_and_close(s, txn->status, http_error_message(s)); } - if (!(s->flags & SF_ERR_MASK)) - s->flags |= SF_ERR_PRXCOND; - if (!(s->flags & SF_FINST_MASK)) - s->flags |= ((txn->rsp.msg_state < HTTP_MSG_ERROR) ? SF_FINST_H : SF_FINST_D); + http_set_term_flags(s); DBG_TRACE_DEVEL("leaving on error ", STRM_EV_STRM_ANA|STRM_EV_HTTP_ANA|STRM_EV_HTTP_ERR, s, txn); return 0; @@ -2126,19 +2091,6 @@ int http_response_forward_body(struct stream *s, struct channel *res, int an_bit if (htx->flags & HTX_FL_PROCESSING_ERROR) goto return_int_err; - 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)))) { - /* Output closed while we were sending data. We must abort and - * wake the other side up. - */ - msg->msg_state = HTTP_MSG_ERROR; - http_end_response(s); - http_end_request(s); - DBG_TRACE_DEVEL("leaving on error", - STRM_EV_STRM_ANA|STRM_EV_HTTP_ANA|STRM_EV_HTTP_ERR, s, txn); - return 1; - } - if (msg->msg_state == HTTP_MSG_BODY) msg->msg_state = HTTP_MSG_DATA; @@ -2229,17 +2181,14 @@ int http_response_forward_body(struct stream *s, struct channel *res, int an_bit channel_dont_close(res); + if ((res->flags & CF_SHUTW) && co_data(res)) { + /* response errors are most likely due to the client aborting + * the transfer. */ + goto return_cli_abort; + } + http_end_response(s); if (!(res->analysers & an_bit)) { - http_end_request(s); - if (unlikely(msg->msg_state == HTTP_MSG_ERROR)) { - if (res->flags & CF_SHUTW) { - /* response errors are most likely due to the - * client aborting the transfer. */ - goto return_cli_abort; - } - goto return_bad_res; - } DBG_TRACE_LEAVE(STRM_EV_STRM_ANA|STRM_EV_HTTP_ANA, s, txn); return 1; } @@ -2298,7 +2247,7 @@ int http_response_forward_body(struct stream *s, struct channel *res, int an_bit _HA_ATOMIC_INC(&__objt_server(s->target)->counters.srv_aborts); stream_inc_http_fail_ctr(s); if (!(s->flags & SF_ERR_MASK)) - s->flags |= SF_ERR_SRVCL; + s->flags |= ((res->flags & CF_READ_TIMEOUT) ? SF_ERR_SRVTO : SF_ERR_SRVCL); goto return_error; return_cli_abort: @@ -2309,7 +2258,7 @@ int http_response_forward_body(struct stream *s, struct channel *res, int an_bit if (objt_server(s->target)) _HA_ATOMIC_INC(&__objt_server(s->target)->counters.cli_aborts); if (!(s->flags & SF_ERR_MASK)) - s->flags |= SF_ERR_CLICL; + s->flags |= ((res->flags & CF_WRITE_TIMEOUT) ? SF_ERR_CLITO : SF_ERR_CLICL); goto return_error; return_int_err: @@ -2337,8 +2286,8 @@ int http_response_forward_body(struct stream *s, struct channel *res, int an_bit return_error: /* don't send any error message as we're in the body */ http_reply_and_close(s, txn->status, NULL); - if (!(s->flags & SF_FINST_MASK)) - s->flags |= SF_FINST_D; + http_set_term_flags(s); + stream_inc_http_fail_ctr(s); DBG_TRACE_DEVEL("leaving on error", STRM_EV_STRM_ANA|STRM_EV_HTTP_ANA|STRM_EV_HTTP_ERR, s, txn); return 0; @@ -4336,13 +4285,6 @@ static void http_end_request(struct stream *s) DBG_TRACE_ENTER(STRM_EV_HTTP_ANA, s, txn); - if (unlikely(txn->req.msg_state == HTTP_MSG_ERROR || - txn->rsp.msg_state == HTTP_MSG_ERROR)) { - channel_abort(chn); - channel_htx_truncate(chn, htxbuf(&chn->buf)); - goto end; - } - if (unlikely(txn->req.msg_state < HTTP_MSG_DONE)) { DBG_TRACE_DEVEL("waiting end of the request", STRM_EV_HTTP_ANA, s, txn); return; @@ -4422,10 +4364,6 @@ static void http_end_request(struct stream *s) txn->req.msg_state = HTTP_MSG_CLOSED; goto http_msg_closed; } - else if (chn->flags & CF_SHUTW) { - txn->req.msg_state = HTTP_MSG_ERROR; - goto end; - } DBG_TRACE_LEAVE(STRM_EV_HTTP_ANA, s, txn); return; } @@ -4472,13 +4410,6 @@ static void http_end_response(struct stream *s) DBG_TRACE_ENTER(STRM_EV_HTTP_ANA, s, txn); - if (unlikely(txn->req.msg_state == HTTP_MSG_ERROR || - txn->rsp.msg_state == HTTP_MSG_ERROR)) { - channel_htx_truncate(&s->req, htxbuf(&s->req.buf)); - channel_abort(&s->req); - goto end; - } - if (unlikely(txn->rsp.msg_state < HTTP_MSG_DONE)) { DBG_TRACE_DEVEL("waiting end of the response", STRM_EV_HTTP_ANA, s, txn); return; @@ -4532,16 +4463,6 @@ static void http_end_response(struct stream *s) txn->rsp.msg_state = HTTP_MSG_CLOSED; goto http_msg_closed; } - else if (chn->flags & CF_SHUTW) { - txn->rsp.msg_state = HTTP_MSG_ERROR; - _HA_ATOMIC_INC(&strm_sess(s)->fe->fe_counters.cli_aborts); - _HA_ATOMIC_INC(&s->be->be_counters.cli_aborts); - if (strm_sess(s)->listener && strm_sess(s)->listener->counters) - _HA_ATOMIC_INC(&strm_sess(s)->listener->counters->cli_aborts); - if (objt_server(s->target)) - _HA_ATOMIC_INC(&__objt_server(s->target)->counters.cli_aborts); - goto end; - } DBG_TRACE_LEAVE(STRM_EV_HTTP_ANA, s, txn); return; }