From 63fac76c2485c7c675fcbd5bc719c969c76ecc01 Mon Sep 17 00:00:00 2001 From: Hugo Landau Date: Mon, 24 Jul 2023 18:11:23 +0100 Subject: [PATCH] QUIC: Automatically drain non-concluded streams, bugfixes Reviewed-by: Tomas Mraz Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/21484) --- ssl/quic/quic_impl.c | 7 ++++++- ssl/quic/quic_stream_map.c | 25 ++++++++++++++++++------- ssl/quic/quic_txp.c | 5 +++-- test/helpers/quictestlib.c | 11 ++++++++++- test/quicapitest.c | 9 +++++++-- 5 files changed, 44 insertions(+), 13 deletions(-) diff --git a/ssl/quic/quic_impl.c b/ssl/quic/quic_impl.c index 3264fb9c94..848b1b4f52 100644 --- a/ssl/quic/quic_impl.c +++ b/ssl/quic/quic_impl.c @@ -1169,7 +1169,7 @@ int ossl_quic_conn_shutdown(SSL *s, uint64_t flags, int stream_flush = ((flags & SSL_SHUTDOWN_FLAG_NO_STREAM_FLUSH) == 0); if (!expect_quic(s, &ctx)) - return 0; + return -1; if (ctx.is_stream) /* TODO(QUIC): Semantics currently undefined for QSSOs */ @@ -1177,6 +1177,11 @@ int ossl_quic_conn_shutdown(SSL *s, uint64_t flags, quic_lock(ctx.qc); + if (ossl_quic_channel_is_terminated(ctx.qc->ch)) { + quic_unlock(ctx.qc); + return 1; + } + /* Phase 1: Stream Flushing */ if (stream_flush) { qc_shutdown_flush_init(ctx.qc); diff --git a/ssl/quic/quic_stream_map.c b/ssl/quic/quic_stream_map.c index e50da2b101..b23429a378 100644 --- a/ssl/quic/quic_stream_map.c +++ b/ssl/quic/quic_stream_map.c @@ -16,6 +16,8 @@ */ DEFINE_LHASH_OF_EX(QUIC_STREAM); +static void shutdown_flush_done(QUIC_STREAM_MAP *qsm, QUIC_STREAM *qs); + /* Circular list management. */ static void list_insert_tail(QUIC_STREAM_LIST_NODE *l, QUIC_STREAM_LIST_NODE *n) @@ -327,6 +329,10 @@ void ossl_quic_stream_map_update_state(QUIC_STREAM_MAP *qsm, QUIC_STREAM *s) if (s->send_state == QUIC_SSTREAM_STATE_DATA_SENT && ossl_quic_sstream_is_totally_acked(s->sstream)) ossl_quic_stream_map_notify_totally_acked(qsm, s); + else if (s->shutdown_flush + && s->send_state == QUIC_SSTREAM_STATE_SEND + && ossl_quic_sstream_is_totally_acked(s->sstream)) + shutdown_flush_done(qsm, s); if (!s->ready_for_gc) { s->ready_for_gc = qsm_ready_for_gc(qsm, s); @@ -765,14 +771,19 @@ static int eligible_for_shutdown_flush(QUIC_STREAM *qs) { /* * We only care about servicing the send part of a stream (if any) during - * shutdown flush. A stream needs to have a final size and that final size - * needs to be a result of normal conclusion of a stream via - * SSL_stream_conclude (as opposed to due to reset). If the application did - * not conclude a stream before deleting it we assume it does not care about - * data being flushed during connection termination. + * shutdown flush. We make sure we flush a stream if it is either + * non-terminated or was terminated normally such as via + * SSL_stream_conclude. A stream which was terminated via a reset is not + * flushed, and we will have thrown away the send buffer in that case + * anyway. */ - return ossl_quic_stream_has_send_buffer(qs) - && ossl_quic_stream_send_get_final_size(qs, NULL); + switch (qs->send_state) { + case QUIC_SSTREAM_STATE_SEND: + case QUIC_SSTREAM_STATE_DATA_SENT: + return !ossl_quic_sstream_is_totally_acked(qs->sstream); + default: + return 0; + } } static void begin_shutdown_flush_each(QUIC_STREAM *qs, void *arg) diff --git a/ssl/quic/quic_txp.c b/ssl/quic/quic_txp.c index 55755eddf8..f244488a4c 100644 --- a/ssl/quic/quic_txp.c +++ b/ssl/quic/quic_txp.c @@ -2969,8 +2969,9 @@ OSSL_TIME ossl_quic_tx_packetiser_get_deadline(OSSL_QUIC_TX_PACKETISER *txp) } /* When will CC let us send more? */ - deadline = ossl_time_min(deadline, - txp->args.cc_method->get_wakeup_deadline(txp->args.cc_data)); + if (txp->args.cc_method->get_tx_allowance(txp->args.cc_data) == 0) + deadline = ossl_time_min(deadline, + txp->args.cc_method->get_wakeup_deadline(txp->args.cc_data)); return deadline; } diff --git a/test/helpers/quictestlib.c b/test/helpers/quictestlib.c index 0d99d4556c..d20afb4585 100644 --- a/test/helpers/quictestlib.c +++ b/test/helpers/quictestlib.c @@ -337,8 +337,17 @@ int qtest_create_quic_connection(QUIC_TSERVER *qtserv, SSL *clientssl) int qtest_shutdown(QUIC_TSERVER *qtserv, SSL *clientssl) { /* Busy loop in non-blocking mode. It should be quick because its local */ - while (SSL_shutdown(clientssl) != 1) + for (;;) { + int rc = SSL_shutdown(clientssl); + + if (rc == 1) + break; + + if (rc < 0) + return 0; + ossl_quic_tserver_tick(qtserv); + } return 1; } diff --git a/test/quicapitest.c b/test/quicapitest.c index 780224f07c..9c79572208 100644 --- a/test/quicapitest.c +++ b/test/quicapitest.c @@ -133,8 +133,13 @@ static int test_quic_write_read(int idx) goto end; } - if (!TEST_true(qtest_shutdown(qtserv, clientquic))) - goto end; + if (idx < 1) + /* + * Blocking SSL_shutdown cannot be tested here due to requirement to + * tick TSERVER during drainage. + */ + if (!TEST_true(qtest_shutdown(qtserv, clientquic))) + goto end; ret = 1; -- 2.39.5