RepoMirrors/haproxy
1
0
Fork 0
mirror of http://git.haproxy.org/git/haproxy.git/ synced 2025-04-25 20:38:03 +00:00
Code Issues Packages Projects Releases Wiki Activity

BUG/MEDIUM: mux-quic: report early error on stream

Browse Source 
On STOP_SENDING reception, an error is notified to the stream layer as
no more data can be responded. However, this is not done if the stream
instance is not allocated (already freed for example).

The issue occurs if STOP_SENDING is received and the stream instance is
instantiated after it. It happens if a STREAM frame is received after it
with H3 HEADERS, which is valid in QUIC protocol due to UDP packet
reordering. In this case, stream layer is never notified about the
underlying error. Instead, reponse buffers are silently purged by the
MUX in qmux_strm_snd_buf().

This is suboptimal as there is no point in exchanging data from the
server if it cannot be eventually transferred back to the client.
However, aside from this consideration, no other issue occured. However,
this is not the case with QUIC mux-to-mux implementation. Now, if
mux-to-mux is used, qmux_strm_snd_buf() is bypassed and response if
transferred via nego_ff/done_ff callbacks. However, these functions did
not checked if QCS is already locally closed. This causes a crash when
qcc_send_stream() is called via done_ff.

To fix this crash, there is several approach, one of them would be to
adjust nego_ff/done_ff QUIC callbacks. However, another method has been
chosen. Now stream layer is flagged on error just after its
instantiation if the stream is already locally closed. This ensures that
mux-to-mux won't try to emit data as se_nego_ff() check if the opposide
SD is not on error before continuing.

Note that an alternative solution could be to not instantiate at all
stream layer if QCS is already locally closed. This is the most optimal
solution as it reduce unnecessary allocations and task processing.
However, it's not easy to implement so the easier bug fix has been
chosen for the moment.

This patch is labelled as MEDIUM as it can change behavior of all QCS
instances, wheter mux-to-mux is used or not, and thus could reveal other
architecture issues.

This should fix latest crash occurence on github issue #2392.

It should be backported up to 2.6, until a necessary period of
observation.
This commit is contained in:
Amaury Denoyelle 2023-12-13 16:28:28 +01:00
parent 682f73b4fa
commit af297f19f6
1 changed files with 13 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
src/mux_quic.c
View File

@ -681,6 +681,19 @@ struct stconn *qcs_attach_sc(struct qcs *qcs, struct buffer *buf, char fin)
se_fl_set(qcs->sd, SE_FL_EOI);
}
/* A QCS can be already locally closed before stream layer
* instantiation. This notably happens if STOP_SENDING was the first
* frame received for this instance. In this case, an error is
* immediately to the stream layer to prevent transmission.
*
* TODO it could be better to not instantiate at all the stream layer.
* However, extra care is required to ensure QCS instance is released.
*/
if (unlikely(qcs_is_close_local(qcs) || (qcs->flags & QC_SF_TO_RESET))) {
TRACE_STATE("report early error", QMUX_EV_STRM_RECV, qcc->conn, qcs);
se_fl_set_error(qcs->sd);
}
return qcs->sd->sc;
}