From ef2b15998cc7148ec78c0ef6b4f511732bdf608f Mon Sep 17 00:00:00 2001 From: Christopher Faulet Date: Wed, 2 Aug 2023 09:48:55 +0200 Subject: [PATCH] BUG/MINOR: htx/mux-h1: Properly handle bodyless responses when splicing is used There is a mechanisme in the H1 and H2 multiplexer to skip the payload when a response is returned to the client when it must not contain any payload (response to a HEAD request or a 204/304 response). However, this does not work when the splicing is used. The H2 multiplexer does not support the splicing, so there is no issue. But with the mux-h1, when data are sent using the kernel splicing, the mux on the server side is not aware the client side should skip the payload. And once the data are put in a pipe, there is no way to stop the sending. It is a defect of the current design. This will be easier to deal with this case when the mux-to-mux forwarding will be implemented. But for now, to fix the issue, we should add an HTX flag on the start-line to pass the info from the client side to the server side and be able to disable the splicing in necessary. The associated reg-test was improved to be sure it does not fail when the splicing is configured. This patch should be backported as far as 2.4.. --- include/haproxy/htx-t.h | 1 + .../http-messaging/http_bodyless_response.vtc | 45 ++++++++++++++++++- src/h1_htx.c | 11 +++-- src/mux_h1.c | 7 +-- src/mux_h2.c | 4 +- 5 files changed, 57 insertions(+), 11 deletions(-) diff --git a/include/haproxy/htx-t.h b/include/haproxy/htx-t.h index 1c970eb1b8..687e403c7b 100644 --- a/include/haproxy/htx-t.h +++ b/include/haproxy/htx-t.h @@ -140,6 +140,7 @@ #define HTX_SL_F_HAS_AUTHORITY 0x00000400 /* The request authority is explicitly specified */ #define HTX_SL_F_NORMALIZED_URI 0x00000800 /* The received URI is normalized (an implicit absolute-uri form) */ #define HTX_SL_F_CONN_UPG 0x00001000 /* The message contains "connection: upgrade" header */ +#define HTX_SL_F_BODYLESS_RESP 0x00002000 /* The response to this message is bodyloess (only for reqyest) */ /* This function is used to report flags in debugging tools. Please reflect * below any single-bit flag addition above in the same order via the diff --git a/reg-tests/http-messaging/http_bodyless_response.vtc b/reg-tests/http-messaging/http_bodyless_response.vtc index 9e0ce1c1b3..5e95203661 100644 --- a/reg-tests/http-messaging/http_bodyless_response.vtc +++ b/reg-tests/http-messaging/http_bodyless_response.vtc @@ -12,7 +12,7 @@ server s1 { rxreq txresp \ -status 200 \ - -bodylen 20000 + -bodylen 50000 rxreq txresp \ @@ -21,13 +21,14 @@ server s1 { chunkedlen 15 chunkedlen 1024 chunkedlen 4048 + chunkedlen 50000 chunkedlen 0 rxreq txresp \ -status 200 \ -body "last response" -} -repeat 2 -start +} -repeat 3 -start haproxy h1 -conf { global @@ -60,6 +61,14 @@ haproxy h1 -conf { listen fe2 bind "fd@${fe2}" server s1 ${h1_int_addr}:${h1_int_port} proto h2 + + listen fe3 + bind "fd@${fe3}" + # Rewrite the method to be sure to get the response payload + # on the server side + http-request set-method GET + option splice-response + server s1 ${s1_addr}:${s1_port} } -start client c1 -connect ${h1_fe1_sock} { @@ -125,3 +134,35 @@ client c2 -connect ${h1_fe2_sock} { expect resp.status == 200 expect resp.body == "last response" } -run + +client c3 -connect ${h1_fe3_sock} { + txreq \ + -req "HEAD" \ + -url "/req1" + rxresp + expect resp.status == 200 + expect resp.body == "" + + txreq \ + -req "HEAD" \ + -url "/req2" + rxresp + expect resp.status == 200 + expect resp.body == "" + + txreq \ + -req "HEAD" \ + -url "/req3" + rxresp + expect resp.status == 200 + expect resp.body == "" + + # The last one have a body and validate the connection was not closed + # unexpectedly and no payload was received for previous requests + txreq \ + -req "GET" \ + -url "/req4" + rxresp + expect resp.status == 200 + expect resp.body == "last response" +} -run diff --git a/src/h1_htx.c b/src/h1_htx.c index d913fe17bc..617169fdbf 100644 --- a/src/h1_htx.c +++ b/src/h1_htx.c @@ -152,7 +152,7 @@ static int h1_postparse_req_hdrs(struct h1m *h1m, union h1_sl *h1sl, struct htx { struct htx_sl *sl; struct ist meth, uri, vsn; - unsigned int flags; + unsigned int flags = 0; /* is always defined for a request */ meth = h1sl->rq.m; @@ -175,9 +175,11 @@ static int h1_postparse_req_hdrs(struct h1m *h1m, union h1_sl *h1sl, struct htx h1m->flags &= ~(H1_MF_CLEN|H1_MF_CHNK); h1m->curr_len = h1m->body_len = 0; } + else if (h1sl->rq.meth == HTTP_METH_HEAD) + flags |= HTX_SL_F_BODYLESS_RESP; - flags = h1m_htx_sl_flags(h1m); + flags |= h1m_htx_sl_flags(h1m); if ((flags & (HTX_SL_F_CONN_UPG|HTX_SL_F_BODYLESS)) == HTX_SL_F_CONN_UPG) { int i; @@ -235,7 +237,7 @@ static int h1_postparse_res_hdrs(struct h1m *h1m, union h1_sl *h1sl, struct htx { struct htx_sl *sl; struct ist vsn, status, reason; - unsigned int flags; + unsigned int flags = 0; uint16_t code = 0; if (h1sl) { @@ -293,13 +295,14 @@ static int h1_postparse_res_hdrs(struct h1m *h1m, union h1_sl *h1sl, struct htx h1m->flags &= ~(H1_MF_CLEN|H1_MF_CHNK); h1m->flags |= H1_MF_XFER_LEN; h1m->curr_len = h1m->body_len = 0; + flags |= HTX_SL_F_BODYLESS_RESP; } else if (h1m->flags & (H1_MF_CLEN|H1_MF_CHNK)) { /* Responses with a known body length. */ h1m->flags |= H1_MF_XFER_LEN; } - flags = h1m_htx_sl_flags(h1m); + flags |= h1m_htx_sl_flags(h1m); sl = htx_add_stline(htx, HTX_BLK_RES_SL, flags, vsn, status, reason); if (!sl || !htx_add_all_headers(htx, hdrs)) goto error; diff --git a/src/mux_h1.c b/src/mux_h1.c index 40f94cad5b..667eff50be 100644 --- a/src/mux_h1.c +++ b/src/mux_h1.c @@ -1906,7 +1906,8 @@ static size_t h1_process_demux(struct h1c *h1c, struct buffer *buf, size_t count } /* Here h1s_sc(h1s) is always defined */ - if (h1m->state == H1_MSG_DATA || (h1m->state == H1_MSG_TUNNEL)) { + if ((!(h1m->flags & H1_MF_RESP) || !(h1s->flags & H1S_F_BODYLESS_RESP)) && + (h1m->state == H1_MSG_DATA || h1m->state == H1_MSG_TUNNEL)) { TRACE_STATE("notify the mux can use splicing", H1_EV_RX_DATA|H1_EV_RX_BODY, h1c->conn, h1s); se_fl_set(h1s->sd, SE_FL_MAY_SPLICE); } @@ -2028,7 +2029,7 @@ static size_t h1_make_reqline(struct h1s *h1s, struct h1m *h1m, struct htx *htx, h1m->flags |= H1_MF_XFER_LEN; if (sl->flags & HTX_SL_F_BODYLESS) h1m->flags |= H1_MF_CLEN; - if (h1s->meth == HTTP_METH_HEAD) + if ((sl->flags & HTX_SL_F_BODYLESS_RESP) || h1s->meth == HTTP_METH_HEAD) h1s->flags |= H1S_F_BODYLESS_RESP; if (h1s->flags & H1S_F_RX_BLK) { @@ -2105,7 +2106,7 @@ static size_t h1_make_stline(struct h1s *h1s, struct h1m *h1m, struct htx *htx, h1m->flags |= H1_MF_XFER_LEN; if (h1s->status < 200) h1s->flags |= H1S_F_HAVE_O_CONN; - else if (h1s->status == 204 || h1s->status == 304) + else if ((sl->flags & HTX_SL_F_BODYLESS_RESP) || h1s->status == 204 || h1s->status == 304) h1s->flags |= H1S_F_BODYLESS_RESP; h1m->state = H1_MSG_HDR_NAME; diff --git a/src/mux_h2.c b/src/mux_h2.c index 2e21088ae1..4120e00405 100644 --- a/src/mux_h2.c +++ b/src/mux_h2.c @@ -5195,7 +5195,7 @@ static size_t h2s_snd_fhdrs(struct h2s *h2s, struct htx *htx) BUG_ON(sl); /* Only one start-line expected */ sl = htx_get_blk_ptr(htx, blk); h2s->status = sl->info.res.status; - if (h2s->status == 204 || h2s->status == 304) + if ((sl->flags & HTX_SL_F_BODYLESS_RESP) || h2s->status == 204 || h2s->status == 304) h2s->flags |= H2_SF_BODYLESS_RESP; if (h2s->status < 100 || h2s->status > 999) { TRACE_ERROR("will not encode an invalid status code", H2_EV_TX_FRAME|H2_EV_TX_HDR|H2_EV_H2S_ERR, h2c->conn, h2s); @@ -5507,7 +5507,7 @@ static size_t h2s_snd_bhdrs(struct h2s *h2s, struct htx *htx) sl = htx_get_blk_ptr(htx, blk); meth = htx_sl_req_meth(sl); uri = htx_sl_req_uri(sl); - if (sl->info.req.meth == HTTP_METH_HEAD) + if ((sl->flags & HTX_SL_F_BODYLESS_RESP) || sl->info.req.meth == HTTP_METH_HEAD) h2s->flags |= H2_SF_BODYLESS_RESP; if (unlikely(uri.len == 0)) { TRACE_ERROR("no URI in HTX request", H2_EV_TX_FRAME|H2_EV_TX_HDR|H2_EV_H2S_ERR, h2c->conn, h2s); -- 2.39.5