From: Amaury Denoyelle Date: Thu, 4 Jan 2024 10:33:33 +0000 (+0100) Subject: BUG/MEDIUM: h3: fix incorrect snd_buf return value X-Git-Tag: v3.0-dev1~16 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=14673fe54d91af3ce28ee2d94ca77f626eb1a7ea;p=thirdparty%2Fhaproxy.git BUG/MEDIUM: h3: fix incorrect snd_buf return value h3_resp_data_send() is used to transcode HTX data into H3 data frames. If QCS Tx buffer is not aligned when first invoked, two separate frames may be built, first until buffer end, then with remaining space in front. If buffer space is not enough for at least the H3 frame header, -1 is returned with the flag QC_SF_BLK_MROOM set to await for more room. An issue arises if this occurs for the second frame : -1 is returned even though HTX data were properly transcoded and removed on the first step. This causes snd_buf callback to return an incorrect value to the stream layer, which in the end will corrupt the channel output buffer. To fix this, stop considering that not enough remaining space is an error case. Instead, return 0 if this is encountered for the first frame or the HTX removed block size for the second one. As QC_SF_BLK_MROOM is set, this will correctly interrupt H3 encoding. Label err is thus only properly limited to fatal error which should cause a connection closure. A new BUG_ON() has been added which should prevent similar issues in the future. This issue was detected using the following client : $ ngtcp2-client --no-quic-dump --no-http-dump --exit-on-all-streams-close \ 127.0.0.1 20443 -n2 "http://127.0.0.1:20443/?s=50k" This triggers the following CHECK_IF statement. Note that it may be necessary to disable fast forwarding to enforce snd_buf usage. Thread 1 "haproxy" received signal SIGILL, Illegal instruction. 0x00005555558bc48a in co_data (c=0x5555561ed428) at include/haproxy/channel.h:130 130 CHECK_IF_HOT(c->output > c_data(c)); [ ## gdb ## ] bt #0 0x00005555558bc48a in co_data (c=0x5555561ed428) at include/haproxy/channel.h:130 #1 0x00005555558c1d69 in sc_conn_send (sc=0x5555561f92d0) at src/stconn.c:1637 #2 0x00005555558c2683 in sc_conn_io_cb (t=0x5555561f7f10, ctx=0x5555561f92d0, state=32832) at src/stconn.c:1824 #3 0x000055555590c48f in run_tasks_from_lists (budgets=0x7fffffffdaa0) at src/task.c:596 #4 0x000055555590cf88 in process_runnable_tasks () at src/task.c:876 #5 0x00005555558aae3b in run_poll_loop () at src/haproxy.c:3049 #6 0x00005555558ab57e in run_thread_poll_loop (data=0x555555d9fa00 ) at src/haproxy.c:3251 #7 0x00005555558ad053 in main (argc=6, argv=0x7fffffffddd8) at src/haproxy.c:3948 In case CHECK_IF are not activated, it may cause crash or incorrect transfers. This was introduced by the following commit commit 2144d2418651c1f76b91cc1f6e745feecdefcb00 BUG/MINOR: h3: close connection on sending alloc errors This must be backported wherever the above patch is. --- diff --git a/src/h3.c b/src/h3.c index f0ac695082..5b9d85d622 100644 --- a/src/h3.c +++ b/src/h3.c @@ -1773,7 +1773,8 @@ static int h3_resp_trailers_send(struct qcs *qcs, struct htx *htx) * performed. * * Returns the total bytes of encoded HTTP/3 payload. This corresponds to the - * total bytes of HTX block removed. + * total bytes of HTX block removed. A negative error code is returned in case + * of a fatal error which should caused a connection closure. */ static int h3_resp_data_send(struct qcs *qcs, struct htx *htx, struct buffer *buf, size_t count) @@ -1856,7 +1857,7 @@ static int h3_resp_data_send(struct qcs *qcs, struct htx *htx, if (b_size(&outbuf) <= hsize) { TRACE_STATE("not enough room for data frame", H3_EV_TX_FRAME|H3_EV_TX_DATA, qcs->qcc->conn, qcs); qcs->flags |= QC_SF_BLK_MROOM; - goto err; + goto end; } if (b_size(&outbuf) < hsize + fsize) @@ -1886,6 +1887,7 @@ static int h3_resp_data_send(struct qcs *qcs, struct htx *htx, return total; err: + BUG_ON(total); /* Must return HTX removed size if at least on frame encoded. */ TRACE_DEVEL("leaving on error", H3_EV_TX_FRAME|H3_EV_TX_DATA, qcs->qcc->conn, qcs); return -1; }