Skip to content

Commit

Permalink
BUG/MINOR: mux-quic: handle closure of uni-stream
Browse files Browse the repository at this point in the history
This commit is a direct follow-up to the previous one. As already
described, a previous fix was merged to prevent streamdesc attach
operation on already completed QCS instances scheduled for purging. This
was implemented by skipping app proto decoding.

However, this has a bad side-effect for remote uni-directional stream.
If receiving a FIN stream frame on such a stream, it will considered as
complete because streamdesc are never attached to a uni stream. Due to
the mentionned new fix, this prevent analysis of this last frame for
every uni stream.

To fix this, do not skip anymore app proto decoding for completed QCS.
Update instead qcs_attach_sc() to transform it as a noop function if QCS
is already fully closed before streamdesc instantiation. However,
success return value is still used to prevent an invalid decoding error
report.

The impact of this bug should be minor. Indeed, HTTP3 and QPACK uni
streams are never closed by the client as this is invalid due to the
spec. The only issue was that this prevented QUIC MUX to close the
connection with error H3_ERR_CLOSED_CRITICAL_STREAM.

This must be backported along the previous patch, at least to 3.1, and
eventually to 2.8 if mentionned patches are merged there.
  • Loading branch information
a-denoyelle committed Jan 3, 2025
1 parent af00be8 commit 801e39e
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 4 deletions.
4 changes: 2 additions & 2 deletions src/h3.c
Original file line number Diff line number Diff line change
Expand Up @@ -938,8 +938,8 @@ static ssize_t h3_headers_to_htx(struct qcs *qcs, const struct buffer *buf,
if (htx)
htx_to_buf(htx, &htx_buf);

/* buffer is transferred to the stream connector and set to NULL
* except on stream creation error.
/* Buffer is transferred to the stream connector and set to NULL except
* on stream creation error or QCS already fully closed.
*/
if (b_size(&htx_buf)) {
b_free(&htx_buf);
Expand Down
15 changes: 13 additions & 2 deletions src/mux_quic.c
Original file line number Diff line number Diff line change
Expand Up @@ -899,7 +899,12 @@ void qcs_send_metadata(struct qcs *qcs)
* the first received request data. <fin> must be set if the whole request is
* already received.
*
* Returns 0 on success else a negative error code.
* Note that if <qcs> is already fully closed, no streamdesc is instantiated.
* This is useful if a RESET_STREAM was already emitted in response to a
* STOP_SENDING.
*
* Returns 0 on success else a negative error code. If stream is already fully
* closed and nothing is performed, it is considered as a success case.
*/
int qcs_attach_sc(struct qcs *qcs, struct buffer *buf, char fin)
{
Expand All @@ -908,6 +913,11 @@ int qcs_attach_sc(struct qcs *qcs, struct buffer *buf, char fin)

TRACE_ENTER(QMUX_EV_STRM_RECV, qcc->conn, qcs);

if (qcs->st == QC_SS_CLO) {
TRACE_STATE("skip attach on already closed stream", QMUX_EV_STRM_RECV, qcc->conn, qcs);
goto out;
}

/* TODO duplicated from mux_h2 */
sess->t_idle = ns_to_ms(now_ns - sess->accept_ts) - sess->t_handshake;

Expand Down Expand Up @@ -958,6 +968,7 @@ int qcs_attach_sc(struct qcs *qcs, struct buffer *buf, char fin)
se_fl_set_error(qcs->sd);
}

out:
TRACE_LEAVE(QMUX_EV_STRM_RECV, qcc->conn, qcs);
return 0;
}
Expand Down Expand Up @@ -1152,7 +1163,7 @@ static int qcc_decode_qcs(struct qcc *qcc, struct qcs *qcs)
if (qcs_is_close_remote(qcs))
fin = 1;

if (!(qcs->flags & QC_SF_READ_ABORTED) && !qcs_is_completed(qcs)) {
if (!(qcs->flags & QC_SF_READ_ABORTED)) {
ret = qcc->app_ops->rcv_buf(qcs, &b, fin);

if (qcc->glitches != prev_glitches)
Expand Down

0 comments on commit 801e39e

Please sign in to comment.