From: Hugo Landau Date: Tue, 6 Jun 2023 15:25:10 +0000 (+0100) Subject: QUIC QSM/STREAM: Refactor to use RFC stream states X-Git-Tag: openssl-3.2.0-alpha1~460 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=2f018d14f06d54c9528ac41d40e95d7638371e50;p=thirdparty%2Fopenssl.git QUIC QSM/STREAM: Refactor to use RFC stream states Reviewed-by: Tomas Mraz Reviewed-by: Matt Caswell Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/21135) --- diff --git a/include/internal/quic_stream_map.h b/include/internal/quic_stream_map.h index 7d2356c22d7..f3b9f4811a4 100644 --- a/include/internal/quic_stream_map.h +++ b/include/internal/quic_stream_map.h @@ -34,6 +34,48 @@ struct quic_stream_list_node_st { QUIC_STREAM_LIST_NODE *prev, *next; }; +/* + * QUIC Send Stream States + * ----------------------- + * + * These correspond to the states defined in RFC 9000 s. 3.1, with the + * exception of the NONE state which represents the absence of a send stream + * part. + * + * Invariants in each state are noted in comments below. In particular, once all + * data has been acknowledged received, or we have reset the stream, we don't + * need to keep the QUIC_SSTREAM and data buffers around. Of course, we also + * don't have a QUIC_SSTREAM on a receive-only stream. + */ +#define QUIC_SSTREAM_STATE_NONE 0 /* --- sstream == NULL */ +#define QUIC_SSTREAM_STATE_READY 1 /* \ */ +#define QUIC_SSTREAM_STATE_SEND 2 /* |-- sstream != NULL */ +#define QUIC_SSTREAM_STATE_DATA_SENT 3 /* / */ +#define QUIC_SSTREAM_STATE_DATA_RECVD 4 /* \ */ +#define QUIC_SSTREAM_STATE_RESET_SENT 5 /* |-- sstream == NULL */ +#define QUIC_SSTREAM_STATE_RESET_RECVD 6 /* / */ + +/* + * QUIC Receive Stream States + * -------------------------- + * + * These correspond to the states defined in RFC 9000 s. 3.2, with the exception + * of the NONE state which represents the absence of a receive stream part. + * + * Invariants in each state are noted in comments below. In particular, once all + * data has been read by the application, we don't need to keep the QUIC_RSTREAM + * and data buffers around. If the receive part is instead reset before it is + * finished, we also don't need to keep the QUIC_RSTREAM around. Finally, we + * don't need a QUIC_RSTREAM on a send-only stream. + */ +#define QUIC_RSTREAM_STATE_NONE 0 /* --- rstream == NULL */ +#define QUIC_RSTREAM_STATE_RECV 1 /* \ */ +#define QUIC_RSTREAM_STATE_SIZE_KNOWN 2 /* |-- rstream != NULL */ +#define QUIC_RSTREAM_STATE_DATA_RECVD 3 /* / */ +#define QUIC_RSTREAM_STATE_DATA_READ 4 /* \ */ +#define QUIC_RSTREAM_STATE_RESET_RECVD 5 /* |-- rstream == NULL */ +#define QUIC_RSTREAM_STATE_RESET_READ 6 /* / */ + struct quic_stream_st { QUIC_STREAM_LIST_NODE active_node; /* for use by QUIC_STREAM_MAP */ QUIC_STREAM_LIST_NODE accept_node; /* accept queue of remotely-created streams */ @@ -76,13 +118,46 @@ struct quic_stream_st { /* Temporary value used by TXP. */ uint64_t txp_txfc_new_credit_consumed; + /* + * Send stream part and receive stream part buffer management objects. + * + * DO NOT test these pointers (sstream, rstream) for NULL. Determine the + * state of the send or receive stream part first using the appropriate + * function; then the invariant of that state guarantees that sstream or + * rstream either is or is not NULL respectively, therefore there is no + * valid use case for testing these pointers for NULL. In particular, a + * stream with a send part can still have sstream as NULL, and a stream with + * a receive part can still have rstream as NULL. QUIC_SSTREAM and + * QUIC_RSTREAM are stream buffer resource management objects which exist + * only when they need to for buffer management purposes. The existence or + * non-existence of a QUIC_SSTREAM or QUIC_RSTREAM object does not + * correspond with whether a stream's respective send or receive part + * logically exists or not. + */ QUIC_SSTREAM *sstream; /* NULL if RX-only */ QUIC_RSTREAM *rstream; /* NULL if TX only */ + + /* Stream-level flow control managers. */ QUIC_TXFC txfc; /* NULL if RX-only */ QUIC_RXFC rxfc; /* NULL if TX-only */ - unsigned int type : 8; /* QUIC_STREAM_INITIATOR_*, QUIC_STREAM_DIR_* */ + + unsigned int type : 8; /* QUIC_STREAM_INITIATOR_*, QUIC_STREAM_DIR_* */ + + unsigned int send_state : 8; /* QUIC_SSTREAM_STATE_* */ + unsigned int recv_state : 8; /* QUIC_RSTREAM_STATE_* */ + + /* 1 iff this QUIC_STREAM is on the active queue (invariant). */ unsigned int active : 1; + /* + * This is a cpoy of the QUIC connection as_server value, indicating + * whether we are locally operating as a server or not. Having this + * significantly simplifies stream type determination relative to our + * perspective. It never changes after a QUIC_STREAM is created and is the + * same for all QUIC_STREAMS under a QUIC_STREAM_MAP. + */ + unsigned int as_server : 1; + /* * Has STOP_SENDING been requested (by us)? Note that this is not the same * as want_stop_sending below, as a STOP_SENDING frame may already have been @@ -94,12 +169,8 @@ struct quic_stream_st { * Has RESET_STREAM been requested (by us)? Works identically to * STOP_SENDING for transmission purposes. */ - unsigned int reset_stream : 1; - /* Has our peer sent a STOP_SENDING frame? */ unsigned int peer_stop_sending : 1; - /* Has our peer sent a RESET_STREAM frame? */ - unsigned int peer_reset_stream : 1; /* Temporary flags used by TXP. */ unsigned int txp_sent_fc : 1; @@ -115,7 +186,6 @@ struct quic_stream_st { /* Flags set when frames *we* sent were acknowledged. */ unsigned int acked_stop_sending : 1; - unsigned int acked_reset_stream : 1; /* A FIN has been retired from the rstream buffer. */ unsigned int recv_fin_retired : 1; @@ -233,7 +303,137 @@ struct quic_stream_st { unsigned int ready_for_gc : 1; }; -/* +#define QUIC_STREAM_INITIATOR_CLIENT 0 +#define QUIC_STREAM_INITIATOR_SERVER 1 +#define QUIC_STREAM_INITIATOR_MASK 1 + +#define QUIC_STREAM_DIR_BIDI 0 +#define QUIC_STREAM_DIR_UNI 2 +#define QUIC_STREAM_DIR_MASK 2 + +void ossl_quic_stream_check(const QUIC_STREAM *s); + +/* + * Returns 1 if the QUIC_STREAM was initiated by the endpoint with the server + * role. + */ +static ossl_inline ossl_unused int ossl_quic_stream_is_server_init(const QUIC_STREAM *s) +{ + return (s->type & QUIC_STREAM_INITIATOR_MASK) == QUIC_STREAM_INITIATOR_SERVER; +} + +/* + * Returns 1 if the QUIC_STREAM is bidirectional and 0 if it is unidirectional. + */ +static ossl_inline ossl_unused int ossl_quic_stream_is_bidi(const QUIC_STREAM *s) +{ + return (s->type & QUIC_STREAM_DIR_MASK) == QUIC_STREAM_DIR_BIDI; +} + +/* Returns 1 if the QUIC_STREAM was locally initiated. */ +static ossl_inline ossl_unused int ossl_quic_stream_is_local_init(const QUIC_STREAM *s) +{ + return ossl_quic_stream_is_server_init(s) == s->as_server; +} + +/* + * Returns 1 if the QUIC_STREAM has a sending part, based on its stream type. + * + * Do NOT use (s->sstream != NULL) to test this; use this function. Note that + * even if this function returns 1, s->sstream might be NULL if the QUIC_SSTREAM + * has been deemed no longer needed, for example due to a RESET_STREAM. + */ +static ossl_inline ossl_unused int ossl_quic_stream_has_send(const QUIC_STREAM *s) +{ + return s->send_state != QUIC_SSTREAM_STATE_NONE; +} + +/* + * Returns 1 if the QUIC_STREAM has a receiving part, based on its stream type. + * + * Do NOT use (s->rstream != NULL) to test this; use this function. Note that + * even if this function returns 1, s->rstream might be NULL if the QUIC_RSTREAM + * has been deemed no longer needed, for example if the receive stream is + * completely finished with. + */ +static ossl_inline ossl_unused int ossl_quic_stream_has_recv(const QUIC_STREAM *s) +{ + return s->recv_state != QUIC_RSTREAM_STATE_NONE; +} + +/* + * Returns 1 if the QUIC_STREAM has a QUIC_SSTREAM send buffer associated with + * it. If this returns 1, s->sstream is guaranteed to be non-NULL. The converse + * is not necessarily true; erasure of a send stream buffer which is no longer + * required is an optimisation which the QSM may, but is not obliged, to + * perform. + * + * This call should be used where it is desired to do something with the send + * stream buffer but there is no more specific send state restriction which is + * applicable. + * + * Note: This does NOT indicate whether it is suitable to allow an application + * to append to the buffer. DATA_SENT indicates all data (including FIN) has + * been *sent*; the absence of DATA_SENT does not mean a FIN has not been queued + * (meaning no more application data can be appended). This is enforced by + * QUIC_SSTREAM. + */ +static ossl_inline ossl_unused int ossl_quic_stream_has_send_buffer(const QUIC_STREAM *s) +{ + switch (s->send_state) { + case QUIC_SSTREAM_STATE_READY: + case QUIC_SSTREAM_STATE_SEND: + case QUIC_SSTREAM_STATE_DATA_SENT: + return 1; + default: + return 0; + } +} + +/* + * Returns 1 if the QUIC_STREAM has a sending part which is in one of the reset + * states. + */ +static ossl_inline ossl_unused int ossl_quic_stream_send_is_reset(const QUIC_STREAM *s) +{ + return s->send_state == QUIC_SSTREAM_STATE_RESET_SENT + || s->send_state == QUIC_SSTREAM_STATE_RESET_RECVD; +} + +/* + * Returns 1 if the QUIC_STREAM has a QUIC_RSTREAM receive buffer associated + * with it. If this returns 1, s->rstream is guaranteed to be non-NULL. The + * converse is not necessarily true; erasure of a receive stream buffer which is + * no longer required is an optimisation which the QSM may, but is not obliged, + * to perform. + * + * This call should be used where it is desired to do something with the receive + * stream buffer but there is no more specific receive state restriction which is + * applicable. + */ +static ossl_inline ossl_unused int ossl_quic_stream_has_recv_buffer(const QUIC_STREAM *s) +{ + switch (s->recv_state) { + case QUIC_RSTREAM_STATE_RECV: + case QUIC_RSTREAM_STATE_SIZE_KNOWN: + case QUIC_RSTREAM_STATE_DATA_RECVD: + return 1; + default: + return 0; + } +} + +/* + * Returns 1 if the QUIC_STREAM has a receiving part which is in one of the + * reset states. + */ +static ossl_inline ossl_unused int ossl_quic_stream_recv_is_reset(const QUIC_STREAM *s) +{ + return s->recv_state == QUIC_RSTREAM_STATE_RESET_RECVD + || s->recv_state == QUIC_RSTREAM_STATE_RESET_READ; +} + +/* * QUIC Stream Map * =============== * @@ -284,24 +484,6 @@ int ossl_quic_stream_map_init(QUIC_STREAM_MAP *qsm, */ void ossl_quic_stream_map_cleanup(QUIC_STREAM_MAP *qsm); -#define QUIC_STREAM_INITIATOR_CLIENT 0 -#define QUIC_STREAM_INITIATOR_SERVER 1 -#define QUIC_STREAM_INITIATOR_MASK 1 - -#define QUIC_STREAM_DIR_BIDI 0 -#define QUIC_STREAM_DIR_UNI 2 -#define QUIC_STREAM_DIR_MASK 2 - -static ossl_inline ossl_unused int ossl_quic_stream_is_server_init(QUIC_STREAM *s) -{ - return (s->type & QUIC_STREAM_INITIATOR_MASK) == QUIC_STREAM_INITIATOR_SERVER; -} - -static ossl_inline ossl_unused int ossl_quic_stream_is_bidi(QUIC_STREAM *s) -{ - return (s->type & QUIC_STREAM_DIR_MASK) == QUIC_STREAM_DIR_BIDI; -} - /* * Allocate a new stream. type is a combination of one QUIC_STREAM_INITIATOR_* * value and one QUIC_STREAM_DIR_* value. Note that clients can e.g. allocate @@ -354,20 +536,128 @@ void ossl_quic_stream_map_update_state(QUIC_STREAM_MAP *qsm, QUIC_STREAM *s); */ void ossl_quic_stream_map_set_rr_stepping(QUIC_STREAM_MAP *qsm, size_t stepping); + +/* + * Stream Send Part + * ================ + */ + /* - * Resets the sending part of a stream. + * Ensures that the sending part has transitioned out of the READY state (i.e., + * to SEND, or a subsequent state). This function is named as it is because, + * while on paper the distinction between READY and SEND is whether we have + * started transmitting application data, in practice the meaningful distinction + * between the two states is whether we have allocated a stream ID to the stream + * or not. QUIC permits us to defer stream ID allocation until first STREAM (or + * STREAM_DATA_BLOCKED) frame transmission for locally-initiated streams. * - * Returns 1 if the sending part of a stream was not already reset. - * Returns 0 otherwise, which need not be considered an error. + * Our implementation does not currently do this and we allocate stream IDs up + * front, however we may revisit this in the future. Calling this ensures + * represents a demand for a stream ID by the caller and ensures one has been + * allocated to the stream, and causes us to transition to SEND if we are still + * in the READY state. + * + * Returns 0 if there is no send part (caller error) and 1 otherwise. + */ +int ossl_quic_stream_map_ensure_send_part_id(QUIC_STREAM_MAP *qsm, + QUIC_STREAM *qs); + +/* + * Transitions from READY or SEND to the DATA_SENT state. Note that this is NOT + * the same as the point in time at which the final size of the stream becomes + * known (i.e., the time at which ossl_quic_sstream_fin()) is called as it + * occurs when we have SENT all data on a given stream send part, not merely + * buffered it. + * + * Returns 1 if the state transition was successfully taken. Returns 0 if there + * is no send part (caller error) or if the state transition cannot be taken + * because the send part is not in the SEND state. + */ +int ossl_quic_stream_map_notify_all_data_sent(QUIC_STREAM_MAP *qsm, + QUIC_STREAM *qs); + +/* + * Transitions from the DATA_SENT to DATA_RECVD state; should be called + * when all transmitted stream data is ACKed by the peer. + * + * Returns 1 if the state transition was successfully taken, or if the send part + * was already in the DATA_RECVD state. Returns 0 if there is no send part + * (caller error) or the state transition cannot be taken because the send part + * is not in the DATA_SENT or DATA_RECVD states. Because + * ossl_quic_stream_map_fin_send_part() should always be called prior to this + * function, the send state must already be in DATA_SENT in order for this + * function to succeed. + */ +int ossl_quic_stream_map_notify_totally_acked(QUIC_STREAM_MAP *qsm, + QUIC_STREAM *qs); + +/* + * Resets the sending part of a stream. This is a transition from the READY, + * SEND or DATA_SENT send stream states to the RESET_SENT state. + * + * This function returns 1 if the transition is taken (i.e., if the send stream + * part was in one of the states above), or if it is already in the RESET_SENT + * state (idempotent operation), or if it has reached the RESET_RECVD state. + * + * It returns 0 if in the DATA_RECVD state, as a send stream cannot be reset + * in this state. It also returns 0 if there is no send part (caller error). */ int ossl_quic_stream_map_reset_stream_send_part(QUIC_STREAM_MAP *qsm, QUIC_STREAM *qs, uint64_t aec); /* - * Marks the receiving part of a stream for STOP_SENDING. + * Transitions from the RESET_SENT to the RESET_RECVD state. This should be + * called when a sent RESET_STREAM frame has been acknowledged by the peer. + * + * This function returns 1 if the transition is taken (i.e., if the send stream + * part was in one of the states above) or if it is already in the RESET_RECVD + * state (idempotent operation). + * + * It returns 0 if not in the RESET_SENT state, as this function should only be + * called after we have already sent a RESET_STREAM frame and entered the + * RESET_SENT state. It also returns 0 if there is no send part (caller error). + */ +int ossl_quic_stream_map_notify_reset_stream_acked(QUIC_STREAM_MAP *qsm, + QUIC_STREAM *qs); + + +/* + * Stream Receive Part + * =================== + */ + +/* + * Transitions from the RECV to SIZE_KNOWN receive stream states. This should be + * called once a STREAM frame is received for the stream with the FIN bit set. * - * Returns 1 if the receiving part of a stream was not already marked for + * Returns 1 if the transition was taken. + */ +int ossl_quic_stream_map_notify_size_known_recv_part(QUIC_STREAM_MAP *qsm, + QUIC_STREAM *qs); + +/* SIZE_KNOWN -> DATA_RECVD */ +int ossl_quic_stream_map_notify_totally_received(QUIC_STREAM_MAP *qsm, + QUIC_STREAM *qs); + +/* DATA_RECVD -> DATA_READ */ +int ossl_quic_stream_map_notify_totally_read(QUIC_STREAM_MAP *qsm, + QUIC_STREAM *qs); + +/* RECV/SIZE_KNOWN/DATA_RECVD -> RESET_RECVD */ +int ossl_quic_stream_map_notify_reset_recv_part(QUIC_STREAM_MAP *qsm, + QUIC_STREAM *qs, + uint64_t app_error_code); + +/* RESET_RECVD -> RESET_READ */ +int ossl_quic_stream_map_notify_app_read_reset_recv_part(QUIC_STREAM_MAP *qsm, + QUIC_STREAM *qs); + +/* + * Marks the receiving part of a stream for STOP_SENDING. This is orthogonal to + * receive stream state as it does not affect it directly. + * + * Returns 1 if the receiving part of a stream was not already marked for * STOP_SENDING. * Returns 0 otherwise, which need not be considered an error. */ @@ -375,6 +665,11 @@ int ossl_quic_stream_map_stop_sending_recv_part(QUIC_STREAM_MAP *qsm, QUIC_STREAM *qs, uint64_t aec); +/* + * Accept Queue Management + * ======================= + */ + /* * Adds a stream to the accept queue. */ diff --git a/ssl/quic/quic_channel.c b/ssl/quic/quic_channel.c index 3edb059ad4e..7598fe589d9 100644 --- a/ssl/quic/quic_channel.c +++ b/ssl/quic/quic_channel.c @@ -2817,6 +2817,7 @@ QUIC_STREAM *ossl_quic_channel_new_stream_local(QUIC_CHANNEL *ch, int is_uni) if ((qs = ossl_quic_stream_map_alloc(&ch->qsm, stream_id, type)) == NULL) return NULL; + /* Locally-initiated stream, so we always want a send buffer. */ if (!ch_init_new_stream(ch, qs, /*can_send=*/1, /*can_recv=*/!is_uni)) goto err; diff --git a/ssl/quic/quic_impl.c b/ssl/quic/quic_impl.c index 694e15b3893..f9506923906 100644 --- a/ssl/quic/quic_impl.c +++ b/ssl/quic/quic_impl.c @@ -415,13 +415,15 @@ void ossl_quic_free(SSL *s) --ctx.qc->num_xso; /* If a stream's send part has not been finished, auto-reset it. */ - if (ctx.xso->stream->sstream != NULL + if (( ctx.xso->stream->send_state == QUIC_SSTREAM_STATE_READY + || ctx.xso->stream->send_state == QUIC_SSTREAM_STATE_SEND) && !ossl_quic_sstream_get_final_size(ctx.xso->stream->sstream, NULL)) ossl_quic_stream_map_reset_stream_send_part(ossl_quic_channel_get_qsm(ctx.qc->ch), ctx.xso->stream, 0); /* Do STOP_SENDING for the receive part, if applicable. */ - if (ctx.xso->stream->rstream != NULL) + if ( ctx.xso->stream->recv_state == QUIC_RSTREAM_STATE_RECV + || ctx.xso->stream->recv_state == QUIC_RSTREAM_STATE_SIZE_KNOWN) ossl_quic_stream_map_stop_sending_recv_part(ossl_quic_channel_get_qsm(ctx.qc->ch), ctx.xso->stream, 0); @@ -1960,7 +1962,8 @@ int ossl_quic_write(SSL *s, const void *buf, size_t len, size_t *written) goto out; } - if (ctx.xso->stream == NULL || ctx.xso->stream->sstream == NULL) { + if (ctx.xso->stream == NULL + || !ossl_quic_stream_has_send_buffer(ctx.xso->stream)) { ret = QUIC_RAISE_NON_NORMAL_ERROR(&ctx, ERR_R_INTERNAL_ERROR, NULL); goto out; } @@ -2004,7 +2007,7 @@ static int quic_read_actual(QCTX *ctx, if (stream->recv_fin_retired) return QUIC_RAISE_NORMAL_ERROR(ctx, SSL_ERROR_ZERO_RETURN); - if (stream->rstream == NULL) + if (!ossl_quic_stream_has_recv_buffer(stream)) return QUIC_RAISE_NON_NORMAL_ERROR(ctx, ERR_R_INTERNAL_ERROR, NULL); if (peek) { @@ -2198,7 +2201,8 @@ static size_t ossl_quic_pending_int(const SSL *s, int check_channel) if (!expect_quic_with_stream_lock(s, /*remote_init=*/-1, &ctx)) return 0; - if (ctx.xso->stream == NULL || ctx.xso->stream->rstream == NULL) + if (ctx.xso->stream == NULL + || !ossl_quic_stream_has_recv_buffer(ctx.xso->stream)) /* Cannot raise errors here because we are const, just fail. */ goto out; @@ -2239,13 +2243,15 @@ int ossl_quic_conn_stream_conclude(SSL *s) qs = ctx.xso->stream; - if (qs == NULL || qs->sstream == NULL) { + if (qs == NULL + || !ossl_quic_channel_is_active(ctx.qc->ch) + || !ossl_quic_stream_has_send(qs) + || ossl_quic_stream_send_is_reset(qs)) { quic_unlock(ctx.qc); return 0; } - if (!ossl_quic_channel_is_active(ctx.qc->ch) - || ossl_quic_sstream_get_final_size(qs->sstream, NULL)) { + if (ossl_quic_sstream_get_final_size(qs->sstream, NULL)) { quic_unlock(ctx.qc); return 1; } @@ -2646,6 +2652,7 @@ int ossl_quic_stream_reset(SSL *ssl, QUIC_STREAM_MAP *qsm; QUIC_STREAM *qs; uint64_t error_code; + int ok; if (!expect_quic_with_stream_lock(ssl, /*remote_init=*/0, &ctx)) return 0; @@ -2654,10 +2661,10 @@ int ossl_quic_stream_reset(SSL *ssl, qs = ctx.xso->stream; error_code = (args != NULL ? args->quic_error_code : 0); - ossl_quic_stream_map_reset_stream_send_part(qsm, qs, error_code); + ok = ossl_quic_stream_map_reset_stream_send_part(qsm, qs, error_code); quic_unlock(ctx.qc); - return 1; + return ok; } /* @@ -2693,7 +2700,7 @@ static void quic_classify_stream(QUIC_CONNECTION *qc, /* Application has read a FIN. */ *state = SSL_STREAM_STATE_FINISHED; } else if ((!is_write && qs->stop_sending) - || (is_write && qs->reset_stream)) { + || (is_write && ossl_quic_stream_send_is_reset(qs))) { /* * Stream has been reset locally. FIN takes precedence over this for the * read case as the application need not care if the stream is reset @@ -2703,7 +2710,7 @@ static void quic_classify_stream(QUIC_CONNECTION *qc, *app_error_code = !is_write ? qs->stop_sending_aec : qs->reset_stream_aec; - } else if ((!is_write && qs->peer_reset_stream) + } else if ((!is_write && ossl_quic_stream_recv_is_reset(qs)) || (is_write && qs->peer_stop_sending)) { /* * Stream has been reset remotely. */ diff --git a/ssl/quic/quic_rx_depack.c b/ssl/quic/quic_rx_depack.c index 12aae998d95..a29690897af 100644 --- a/ssl/quic/quic_rx_depack.c +++ b/ssl/quic/quic_rx_depack.c @@ -157,7 +157,7 @@ static int depack_do_frame_reset_stream(PACKET *pkt, if (stream == NULL) return 1; /* old deleted stream, not a protocol violation, ignore */ - if (stream->rstream == NULL) { + if (!ossl_quic_stream_has_recv(stream)) { ossl_quic_channel_raise_protocol_error(ch, QUIC_ERR_STREAM_STATE_ERROR, OSSL_QUIC_FRAME_TYPE_RESET_STREAM, @@ -166,8 +166,14 @@ static int depack_do_frame_reset_stream(PACKET *pkt, return 0; } - stream->peer_reset_stream = 1; - stream->peer_reset_stream_aec = frame_data.app_error_code; + /* + * Depending on the receive part state this is handled either as a reset + * transition or a no-op (e.g. if a reset has already been received before, + * or the application already retired a FIN). Best effort - there are no + * protoocol error conditions we need to check for here. + */ + ossl_quic_stream_map_notify_reset_recv_part(&ch->qsm, stream, + frame_data.app_error_code); ossl_quic_stream_map_update_state(&ch->qsm, stream); return 1; @@ -199,7 +205,7 @@ static int depack_do_frame_stop_sending(PACKET *pkt, if (stream == NULL) return 1; /* old deleted stream, not a protocol violation, ignore */ - if (stream->sstream == NULL) { + if (!ossl_quic_stream_has_send(stream)) { ossl_quic_channel_raise_protocol_error(ch, QUIC_ERR_STREAM_STATE_ERROR, OSSL_QUIC_FRAME_TYPE_STOP_SENDING, @@ -452,7 +458,7 @@ static int depack_do_frame_stream(PACKET *pkt, QUIC_CHANNEL *ch, */ return 1; - if (stream->rstream == NULL) { + if (!ossl_quic_stream_has_recv(stream)) { ossl_quic_channel_raise_protocol_error(ch, QUIC_ERR_STREAM_STATE_ERROR, frame_type, @@ -576,7 +582,7 @@ static int depack_do_frame_max_stream_data(PACKET *pkt, if (stream == NULL) return 1; /* old deleted stream, not a protocol violation, ignore */ - if (stream->sstream == NULL) { + if (!ossl_quic_stream_has_send(stream)) { ossl_quic_channel_raise_protocol_error(ch, QUIC_ERR_STREAM_STATE_ERROR, OSSL_QUIC_FRAME_TYPE_MAX_STREAM_DATA, diff --git a/ssl/quic/quic_stream_map.c b/ssl/quic/quic_stream_map.c index be2505d55ec..b093a33d985 100644 --- a/ssl/quic/quic_stream_map.c +++ b/ssl/quic/quic_stream_map.c @@ -149,8 +149,18 @@ QUIC_STREAM *ossl_quic_stream_map_alloc(QUIC_STREAM_MAP *qsm, if (s == NULL) return NULL; - s->id = stream_id; - s->type = type; + s->id = stream_id; + s->type = type; + s->as_server = qsm->is_server; + s->send_state = (ossl_quic_stream_is_local_init(s) + || ossl_quic_stream_is_bidi(s)) + ? QUIC_SSTREAM_STATE_READY + : QUIC_SSTREAM_STATE_NONE; + s->recv_state = (!ossl_quic_stream_is_local_init(s) + || ossl_quic_stream_is_bidi(s)) + ? QUIC_RSTREAM_STATE_RECV + : QUIC_RSTREAM_STATE_NONE; + lh_QUIC_STREAM_insert(qsm->map, s); return s; } @@ -228,8 +238,18 @@ static int stream_has_data_to_send(QUIC_STREAM *s) size_t num_iov; uint64_t fc_credit, fc_swm, fc_limit; - if (s->sstream == NULL) - return 0; + switch (s->send_state) { + case QUIC_SSTREAM_STATE_READY: + case QUIC_SSTREAM_STATE_SEND: + case QUIC_SSTREAM_STATE_DATA_SENT: + /* + * We can still have data to send in DATA_SENT due to retransmissions, + * etc. + */ + break; + default: + return 0; /* Nothing to send. */ + } /* * We cannot determine if we have data to send simply by checking if @@ -250,6 +270,18 @@ static int stream_has_data_to_send(QUIC_STREAM *s) return (shdr.is_fin && shdr.len == 0) || shdr.offset < fc_limit; } +static ossl_unused int qsm_send_part_permits_gc(const QUIC_STREAM *qs) +{ + switch (qs->send_state) { + case QUIC_SSTREAM_STATE_NONE: + case QUIC_SSTREAM_STATE_DATA_RECVD: + case QUIC_SSTREAM_STATE_RESET_RECVD: + return 1; + default: + return 0; + } +} + static int qsm_ready_for_gc(QUIC_STREAM_MAP *qsm, QUIC_STREAM *qs) { int recv_stream_fully_drained = 0; /* TODO(QUIC): Optimisation */ @@ -259,20 +291,20 @@ static int qsm_ready_for_gc(QUIC_STREAM_MAP *qsm, QUIC_STREAM *qs) * we don't need to worry about that here. */ assert(!qs->deleted - || qs->sstream == NULL - || qs->reset_stream + || !ossl_quic_stream_has_send(qs) + || ossl_quic_stream_send_is_reset(qs) || ossl_quic_sstream_get_final_size(qs->sstream, NULL)); return qs->deleted - && (qs->rstream == NULL + && (!ossl_quic_stream_has_recv(qs) || recv_stream_fully_drained || qs->acked_stop_sending) - && (qs->sstream == NULL - || (!qs->reset_stream + && (!ossl_quic_stream_has_send(qs) + || (!ossl_quic_stream_send_is_reset(qs) && ossl_quic_sstream_is_totally_acked(qs->sstream)) - || (qs->reset_stream - && qs->acked_reset_stream)); + || (ossl_quic_stream_send_is_reset(qs) + && qs->send_state == QUIC_SSTREAM_STATE_RESET_RECVD)); } void ossl_quic_stream_map_update_state(QUIC_STREAM_MAP *qsm, QUIC_STREAM *s) @@ -299,7 +331,8 @@ void ossl_quic_stream_map_update_state(QUIC_STREAM_MAP *qsm, QUIC_STREAM *s) should_be_active = allowed_by_stream_limit && !s->ready_for_gc - && ((!s->peer_reset_stream && s->rstream != NULL + && ((ossl_quic_stream_has_recv(s) + && !ossl_quic_stream_recv_is_reset(s) && (s->want_max_stream_data || ossl_quic_rxfc_has_cwm_changed(&s->rxfc, 0))) || s->want_stop_sending @@ -312,19 +345,251 @@ void ossl_quic_stream_map_update_state(QUIC_STREAM_MAP *qsm, QUIC_STREAM *s) stream_map_mark_inactive(qsm, s); } +/* + * Stream Send Part State Management + * ================================= + */ + +int ossl_quic_stream_map_ensure_send_part_id(QUIC_STREAM_MAP *qsm, + QUIC_STREAM *qs) +{ + switch (qs->send_state) { + case QUIC_SSTREAM_STATE_NONE: + /* Stream without send part - caller error. */ + return 0; + + case QUIC_SSTREAM_STATE_READY: + /* + * We always allocate a stream ID upfront, so we don't need to do it + * here. + */ + qs->send_state = QUIC_SSTREAM_STATE_SEND; + return 1; + + default: + /* Nothing to do. */ + return 1; + } +} + +int ossl_quic_stream_map_notify_all_data_sent(QUIC_STREAM_MAP *qsm, + QUIC_STREAM *qs) +{ + switch (qs->send_state) { + default: + /* Wrong state - caller error. */ + case QUIC_SSTREAM_STATE_NONE: + /* Stream without send part - caller error. */ + return 0; + + case QUIC_SSTREAM_STATE_SEND: + if (!ossl_quic_sstream_get_final_size(qs->sstream, &qs->send_final_size)) + return 0; + + qs->send_state = QUIC_SSTREAM_STATE_DATA_SENT; + return 1; + } +} + +int ossl_quic_stream_map_notify_totally_acked(QUIC_STREAM_MAP *qsm, + QUIC_STREAM *qs) +{ + switch (qs->send_state) { + default: + /* Wrong state - caller error. */ + case QUIC_SSTREAM_STATE_NONE: + /* Stream without send part - caller error. */ + return 0; + + case QUIC_SSTREAM_STATE_DATA_SENT: + qs->send_state = QUIC_SSTREAM_STATE_DATA_RECVD; + /* We no longer need a QUIC_SSTREAM in this state. */ + //ossl_quic_sstream_free(qs->sstream); + //qs->sstream = NULL; + return 1; + } +} + int ossl_quic_stream_map_reset_stream_send_part(QUIC_STREAM_MAP *qsm, QUIC_STREAM *qs, uint64_t aec) { - if (qs->reset_stream) + switch (qs->send_state) { + default: + case QUIC_SSTREAM_STATE_NONE: + /* + * RESET_STREAM pertains to sending part only, so we cannot reset a + * receive-only stream. + */ + case QUIC_SSTREAM_STATE_DATA_RECVD: + /* + * RFC 9000 s. 3.3: A sender MUST NOT [...] send RESET_STREAM from a + * terminal state. If the stream has already finished normally and the + * peer has acknowledged this, we cannot reset it. + */ return 0; - qs->reset_stream = 1; - qs->reset_stream_aec = aec; - qs->want_reset_stream = 1; + case QUIC_SSTREAM_STATE_READY: + case QUIC_SSTREAM_STATE_SEND: + case QUIC_SSTREAM_STATE_DATA_SENT: + qs->reset_stream_aec = aec; + qs->send_state = QUIC_SSTREAM_STATE_RESET_SENT; + qs->want_reset_stream = 1; - ossl_quic_stream_map_update_state(qsm, qs); - return 1; + /* TODO free */ + ossl_quic_stream_map_update_state(qsm, qs); + return 1; + + case QUIC_SSTREAM_STATE_RESET_SENT: + case QUIC_SSTREAM_STATE_RESET_RECVD: + /* + * Idempotent - no-op. In any case, do not send RESET_STREAM again - as + * mentioned, we must not send it from a terminal state. + */ + return 1; + } +} + +/* + * Transitions from the RESET_SENT to the RESET_RECVD state. This should be + * called when a sent RESET_STREAM frame has been acknowledged by the peer. + * + * This function returns 1 if the transition is taken (i.e., if the send stream + * part was in one of the states above) or if it is already in the RESET_RECVD + * state (idempotent operation). + * + * It returns 0 if not in the RESET_SENT state, as this function should only be + * called after we have already sent a RESET_STREAM frame and entered the + * RESET_SENT state. It also returns 0 if there is no send part (caller error). + */ +int ossl_quic_stream_map_notify_reset_stream_acked(QUIC_STREAM_MAP *qsm, + QUIC_STREAM *qs) +{ + switch (qs->send_state) { + default: + /* Wrong state - caller error. */ + case QUIC_SSTREAM_STATE_NONE: + /* Stream without send part - caller error. */ + return 0; + + case QUIC_SSTREAM_STATE_RESET_SENT: + qs->send_state = QUIC_SSTREAM_STATE_RESET_RECVD; + return 1; + + case QUIC_SSTREAM_STATE_RESET_RECVD: + /* Already in the correct state. */ + return 1; + } +} + +/* Stream Receive Part State Management + * ==================================== + */ + +int ossl_quic_stream_map_notify_size_known_recv_part(QUIC_STREAM_MAP *qsm, + QUIC_STREAM *qs) +{ + switch (qs->recv_state) { + default: + /* Wrong state - caller error. */ + case QUIC_RSTREAM_STATE_NONE: + /* Stream without receive part - caller error. */ + return 0; + + case QUIC_RSTREAM_STATE_RECV: + qs->recv_state = QUIC_RSTREAM_STATE_SIZE_KNOWN; + return 1; + } +} + +int ossl_quic_stream_map_notify_totally_received(QUIC_STREAM_MAP *qsm, + QUIC_STREAM *qs) +{ + switch (qs->recv_state) { + default: + /* Wrong state - caller error. */ + case QUIC_RSTREAM_STATE_NONE: + /* Stream without receive part - caller error. */ + return 0; + + case QUIC_RSTREAM_STATE_SIZE_KNOWN: + qs->recv_state = QUIC_RSTREAM_STATE_DATA_RECVD; + return 1; + } +} + +int ossl_quic_stream_map_notify_totally_read(QUIC_STREAM_MAP *qsm, + QUIC_STREAM *qs) +{ + switch (qs->recv_state) { + default: + /* Wrong state - caller error. */ + case QUIC_RSTREAM_STATE_NONE: + /* Stream without receive part - caller error. */ + return 0; + + case QUIC_RSTREAM_STATE_DATA_RECVD: + qs->recv_state = QUIC_RSTREAM_STATE_DATA_READ; + + /* QUIC_RSTREAM is no longer needed */ + //ossl_quic_rstream_free(qs->rstream); + //qs->rstream = NULL; + return 1; + } +} + +int ossl_quic_stream_map_notify_reset_recv_part(QUIC_STREAM_MAP *qsm, + QUIC_STREAM *qs, + uint64_t app_error_code) +{ + switch (qs->recv_state) { + default: + case QUIC_RSTREAM_STATE_NONE: + /* Stream without receive part - caller error. */ + return 0; + + case QUIC_RSTREAM_STATE_RECV: + case QUIC_RSTREAM_STATE_SIZE_KNOWN: + case QUIC_RSTREAM_STATE_DATA_RECVD: + qs->recv_state = QUIC_RSTREAM_STATE_RESET_RECVD; + qs->peer_reset_stream_aec = app_error_code; + + /* RFC 9000 s. 3.3: No point sending STOP_SENDING if already reset. */ + qs->want_stop_sending = 0; + + /* QUIC_RSTREAM is no longer needed */ + //ossl_quic_rstream_free(qs->rstream); + //qs->rstream = NULL; + + ossl_quic_stream_map_update_state(qsm, qs); + return 1; + + case QUIC_RSTREAM_STATE_DATA_READ: + /* + * If we already retired the FIN to the application this is moot + * - just ignore. + */ + case QUIC_RSTREAM_STATE_RESET_RECVD: + case QUIC_RSTREAM_STATE_RESET_READ: + /* Could be a reordered/retransmitted frame - just ignore. */ + return 1; + } +} + +int ossl_quic_stream_map_notify_app_read_reset_recv_part(QUIC_STREAM_MAP *qsm, + QUIC_STREAM *qs) +{ + switch (qs->recv_state) { + default: + /* Wrong state - caller error. */ + case QUIC_RSTREAM_STATE_NONE: + /* Stream without receive part - caller error. */ + return 0; + + case QUIC_RSTREAM_STATE_RESET_RECVD: + qs->recv_state = QUIC_RSTREAM_STATE_RESET_READ; + return 1; + } } int ossl_quic_stream_map_stop_sending_recv_part(QUIC_STREAM_MAP *qsm, diff --git a/ssl/quic/quic_tserver.c b/ssl/quic/quic_tserver.c index 0bfebbdb3f0..70bda7e9e9e 100644 --- a/ssl/quic/quic_tserver.c +++ b/ssl/quic/quic_tserver.c @@ -237,7 +237,7 @@ int ossl_quic_tserver_read(QUIC_TSERVER *srv, return 1; } - if (qs->recv_fin_retired || qs->rstream == NULL) + if (qs->recv_fin_retired || !ossl_quic_stream_has_recv_buffer(qs)) return 0; if (!ossl_quic_rstream_read(qs->rstream, buf, buf_len, @@ -279,7 +279,7 @@ int ossl_quic_tserver_has_read_ended(QUIC_TSERVER *srv, uint64_t stream_id) qs = ossl_quic_stream_map_get_by_id(ossl_quic_channel_get_qsm(srv->ch), stream_id); - if (qs == NULL || qs->rstream == NULL) + if (qs == NULL || !ossl_quic_stream_has_recv_buffer(qs)) return 0; if (qs->recv_fin_retired) @@ -323,7 +323,7 @@ int ossl_quic_tserver_write(QUIC_TSERVER *srv, qs = ossl_quic_stream_map_get_by_id(ossl_quic_channel_get_qsm(srv->ch), stream_id); - if (qs == NULL || qs->sstream == NULL) + if (qs == NULL || !ossl_quic_stream_has_send_buffer(qs)) return 0; if (!ossl_quic_sstream_append(qs->sstream, @@ -351,7 +351,7 @@ int ossl_quic_tserver_conclude(QUIC_TSERVER *srv, uint64_t stream_id) qs = ossl_quic_stream_map_get_by_id(ossl_quic_channel_get_qsm(srv->ch), stream_id); - if (qs == NULL || qs->sstream == NULL) + if (qs == NULL || !ossl_quic_stream_has_send_buffer(qs)) return 0; if (!ossl_quic_sstream_get_final_size(qs->sstream, NULL)) { @@ -412,10 +412,10 @@ int ossl_quic_tserver_stream_has_peer_reset_stream(QUIC_TSERVER *srv, if (qs == NULL) return 0; - if (qs->peer_reset_stream && app_error_code != NULL) + if (ossl_quic_stream_recv_is_reset(qs) && app_error_code != NULL) *app_error_code = qs->peer_reset_stream_aec; - return qs->peer_reset_stream; + return ossl_quic_stream_recv_is_reset(qs); } int ossl_quic_tserver_set_new_local_cid(QUIC_TSERVER *srv, diff --git a/ssl/quic/quic_txp.c b/ssl/quic/quic_txp.c index f471237e2a4..6f868cfadd7 100644 --- a/ssl/quic/quic_txp.c +++ b/ssl/quic/quic_txp.c @@ -1209,7 +1209,11 @@ static void on_confirm_notify(uint64_t frame_type, uint64_t stream_id, if (s == NULL) return; - s->acked_reset_stream = 1; + /* + * We must already be in RESET_SENT or RESET_RECVD if we are + * here, so we don't need to check state here. + */ + ossl_quic_stream_map_notify_reset_stream_acked(txp->args.qsm, s); ossl_quic_stream_map_update_state(txp->args.qsm, s); } break; @@ -1783,7 +1787,6 @@ static int txp_generate_stream_related(OSSL_QUIC_TX_PACKETISER *txp, QUIC_STREAM **tmp_head) { QUIC_STREAM_ITER it; - void *rstream; WPACKET *wpkt; uint64_t cwm; QUIC_STREAM *stream, *snext; @@ -1802,8 +1805,6 @@ static int txp_generate_stream_related(OSSL_QUIC_TX_PACKETISER *txp, stream->txp_blocked = 0; stream->txp_txfc_new_credit_consumed = 0; - rstream = stream->rstream; - /* Stream Abort Frames (STOP_SENDING, RESET_STREAM) */ if (stream->want_stop_sending) { OSSL_QUIC_FRAME_STOP_SENDING f; @@ -1853,7 +1854,7 @@ static int txp_generate_stream_related(OSSL_QUIC_TX_PACKETISER *txp, } /* Stream Flow Control Frames (MAX_STREAM_DATA) */ - if (rstream != NULL + if (ossl_quic_stream_has_recv_buffer(stream) && (stream->want_max_stream_data || ossl_quic_rxfc_has_cwm_changed(&stream->rxfc, 0))) { @@ -1879,7 +1880,7 @@ static int txp_generate_stream_related(OSSL_QUIC_TX_PACKETISER *txp, } /* Stream Data Frames (STREAM) */ - if (stream->sstream != NULL) { + if (ossl_quic_stream_has_send_buffer(stream)) { int packet_full = 0, stream_drained = 0; if (!txp_generate_stream_frames(txp, h, pn_space, tpkt, diff --git a/test/quic_multistream_test.c b/test/quic_multistream_test.c index 3085ffff0da..ed17ebd33d7 100644 --- a/test/quic_multistream_test.c +++ b/test/quic_multistream_test.c @@ -128,6 +128,7 @@ struct script_op { #define OPK_EXPECT_ERR_REASON 38 #define OPK_EXPECT_ERR_LIB 39 #define OPK_SLEEP 40 +#define OPK_S_READ_FAIL 41 #define EXPECT_CONN_CLOSE_APP (1U << 0) #define EXPECT_CONN_CLOSE_REMOTE (1U << 1) @@ -211,6 +212,8 @@ struct script_op { {OPK_S_WRITE_FAIL, NULL, 0, NULL, #stream_name}, #define OP_C_READ_FAIL(stream_name) \ {OPK_C_READ_FAIL, NULL, 0, NULL, #stream_name}, +#define OP_S_READ_FAIL(stream_name) \ + {OPK_S_READ_FAIL, NULL, 0, NULL, #stream_name}, #define OP_C_STREAM_RESET(stream_name, aec) \ {OPK_C_STREAM_RESET, NULL, 0, NULL, #stream_name, (aec)}, #define OP_S_ACCEPT_STREAM_WAIT(stream_name) \ @@ -1315,6 +1318,21 @@ static int run_script_worker(struct helper *h, const struct script_op *script, } break; + case OPK_S_READ_FAIL: + { + size_t bytes_read = 0; + unsigned char buf[1]; + + if (!TEST_uint64_t_ne(s_stream_id, UINT64_MAX)) + goto out; + + if (!TEST_false(ossl_quic_tserver_read(h->s, s_stream_id, + buf, sizeof(buf), + &bytes_read))) + goto out; + } + break; + case OPK_C_STREAM_RESET: { SSL_STREAM_RESET_ARGS args = {0}; @@ -1636,7 +1654,8 @@ static const struct script_op script_5[] = { OP_C_STREAM_RESET (a, 42) OP_S_BIND_STREAM_ID (a, C_BIDI_ID(0)) - OP_S_READ_EXPECT (a, "apple", 5) + /* Reset disrupts read of already sent data */ + OP_S_READ_FAIL (a) OP_CHECK (check_stream_reset, C_BIDI_ID(0)) OP_END diff --git a/test/quic_txp_test.c b/test/quic_txp_test.c index 8da1b05128c..013f542170c 100644 --- a/test/quic_txp_test.c +++ b/test/quic_txp_test.c @@ -1055,7 +1055,6 @@ static const struct script_op script_13[] = { OP_EXPECT_FRAME(OSSL_QUIC_FRAME_TYPE_RESET_STREAM) OP_CHECK(check_stream_13) OP_NEXT_FRAME() - OP_EXPECT_FRAME(OSSL_QUIC_FRAME_TYPE_STREAM) OP_EXPECT_NO_FRAME() OP_RX_PKT_NONE() OP_TXP_GENERATE_NONE(TX_PACKETISER_ARCHETYPE_NORMAL) @@ -1431,7 +1430,9 @@ static int run_script(const struct script_op *script) || !TEST_true(ossl_quic_rxfc_init(&s->rxfc, &h.conn_rxfc, 1 * 1024 * 1024, 16 * 1024 * 1024, - fake_now, NULL))) { + fake_now, NULL)) + || !TEST_ptr(s->rstream = ossl_quic_rstream_new(&s->rxfc, + NULL, 1024))) { ossl_quic_sstream_free(s->sstream); ossl_quic_stream_map_release(h.args.qsm, s); goto err;