From: Tomas Mraz Date: Fri, 18 Aug 2023 15:08:18 +0000 (+0200) Subject: ossl_quic_tx_packetiser_generate(): Always report if packets were sent X-Git-Tag: openssl-3.2.0-alpha1~176 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=64fd69911e04cec45f65b396a4e91d6caa4fdc9a;p=thirdparty%2Fopenssl.git ossl_quic_tx_packetiser_generate(): Always report if packets were sent Even in case of later failure we need to flush the previous packets. Reviewed-by: Hugo Landau Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/21700) --- diff --git a/include/internal/quic_txp.h b/include/internal/quic_txp.h index 5ea464ddd87..09d552ef043 100644 --- a/include/internal/quic_txp.h +++ b/include/internal/quic_txp.h @@ -81,20 +81,17 @@ void ossl_quic_tx_packetiser_record_received_closing_bytes( * generate any frames, and generating a datagram which coalesces packets for * any ELs which do. * - * Returns TX_PACKETISER_RES_FAILURE on failure (e.g. allocation error), - * TX_PACKETISER_RES_NO_PKT if no packets were sent (e.g. because nothing wants - * to send anything), and TX_PACKETISER_RES_SENT_PKT if packets were sent. + * Returns 0 on failure (e.g. allocation error or other errors), 1 otherwise. * - * *status is filled with status information about the generated packet, if any. + * *status is filled with status information about the generated packet. + * It is always filled even in case of failure. In particular, packets can be + * sent even if failure is later returned. * See QUIC_TXP_STATUS for details. */ -#define TX_PACKETISER_RES_FAILURE 0 -#define TX_PACKETISER_RES_NO_PKT 1 -#define TX_PACKETISER_RES_SENT_PKT 2 - typedef struct quic_txp_status_st { int sent_ack_eliciting; /* Was an ACK-eliciting packet sent? */ int sent_handshake; /* Was a Handshake packet sent? */ + size_t sent_pkt; /* Number of packets sent (0 if nothing was sent) */ } QUIC_TXP_STATUS; int ossl_quic_tx_packetiser_generate(OSSL_QUIC_TX_PACKETISER *txp, diff --git a/ssl/quic/quic_channel.c b/ssl/quic/quic_channel.c index 2163990ae78..9fca47bb315 100644 --- a/ssl/quic/quic_channel.c +++ b/ssl/quic/quic_channel.c @@ -2372,6 +2372,7 @@ undesirable: static int ch_tx(QUIC_CHANNEL *ch) { QUIC_TXP_STATUS status; + int res; /* * RFC 9000 s. 10.2.2: Draining Connection State: @@ -2412,8 +2413,8 @@ static int ch_tx(QUIC_CHANNEL *ch) * Best effort. In particular if TXP fails for some reason we should still * flush any queued packets which we already generated. */ - switch (ossl_quic_tx_packetiser_generate(ch->txp, &status)) { - case TX_PACKETISER_RES_SENT_PKT: + res = ossl_quic_tx_packetiser_generate(ch->txp, &status); + if (status.sent_pkt > 0) { ch->have_sent_any_pkt = 1; /* Packet was sent */ /* @@ -2437,13 +2438,12 @@ static int ch_tx(QUIC_CHANNEL *ch) ch->rxku_pending_confirm = 0; ch_update_ping_deadline(ch); - break; - - case TX_PACKETISER_RES_NO_PKT: - break; /* No packet was sent */ + } - default: + if (!res) { /* + * Internal failure (e.g. allocation, assertion). + * * One case where TXP can fail is if we reach a TX PN of 2**62 - 1. As * per RFC 9000 s. 12.3, if this happens we MUST close the connection * without sending a CONNECTION_CLOSE frame. This is actually handled as @@ -2456,7 +2456,6 @@ static int ch_tx(QUIC_CHANNEL *ch) */ ossl_quic_channel_raise_protocol_error(ch, QUIC_ERR_INTERNAL_ERROR, 0, "internal error (txp generate)"); - break; /* Internal failure (e.g. allocation, assertion) */ } /* Flush packets to network. */ diff --git a/ssl/quic/quic_txp.c b/ssl/quic/quic_txp.c index f0c6d96fff9..54b691c9bab 100644 --- a/ssl/quic/quic_txp.c +++ b/ssl/quic/quic_txp.c @@ -702,7 +702,7 @@ int ossl_quic_tx_packetiser_generate(OSSL_QUIC_TX_PACKETISER *txp, * sent to the FIFM. * */ - int res = TX_PACKETISER_RES_FAILURE, rc; + int res = 0, rc; uint32_t archetype, enc_level; uint32_t conn_close_enc_level = QUIC_ENC_LEVEL_NUM; struct txp_pkt pkt[QUIC_ENC_LEVEL_NUM]; @@ -715,6 +715,8 @@ int ossl_quic_tx_packetiser_generate(OSSL_QUIC_TX_PACKETISER *txp, ++enc_level) pkt[enc_level].h_valid = 0; + memset(status, 0, sizeof(*status)); + /* * Should not be needed, but a sanity check in case anyone else has been * using the QTX. @@ -798,8 +800,6 @@ int ossl_quic_tx_packetiser_generate(OSSL_QUIC_TX_PACKETISER *txp, } /* 4. Commit */ - memset(status, 0, sizeof(*status)); - for (enc_level = QUIC_ENC_LEVEL_INITIAL; enc_level < QUIC_ENC_LEVEL_NUM; ++enc_level) { @@ -833,7 +833,7 @@ int ossl_quic_tx_packetiser_generate(OSSL_QUIC_TX_PACKETISER *txp, && pkt[QUIC_ENC_LEVEL_HANDSHAKE].h.bytes_appended > 0); /* Flush & Cleanup */ - res = pkts_done > 0 ? TX_PACKETISER_RES_SENT_PKT : TX_PACKETISER_RES_NO_PKT; + res = 1; out: ossl_qtx_finish_dgram(txp->args.qtx); @@ -842,6 +842,8 @@ out: ++enc_level) txp_pkt_cleanup(&pkt[enc_level], txp); + status->sent_pkt = pkts_done; + return res; } diff --git a/test/quic_txp_test.c b/test/quic_txp_test.c index e6acd5af802..7c3e84b41a5 100644 --- a/test/quic_txp_test.c +++ b/test/quic_txp_test.c @@ -1244,16 +1244,16 @@ static int run_script(int script_idx, const struct script_op *script) for (op = script, opn = 0; op->opcode != OPK_END; ++op, ++opn) { switch (op->opcode) { case OPK_TXP_GENERATE: - if (!TEST_int_eq(ossl_quic_tx_packetiser_generate(h.txp, &status), - TX_PACKETISER_RES_SENT_PKT)) + if (!TEST_true(ossl_quic_tx_packetiser_generate(h.txp, &status)) + && !TEST_size_t_gt(status.sent_pkt, 0)) goto err; ossl_qtx_finish_dgram(h.args.qtx); ossl_qtx_flush_net(h.args.qtx); break; case OPK_TXP_GENERATE_NONE: - if (!TEST_int_eq(ossl_quic_tx_packetiser_generate(h.txp, &status), - TX_PACKETISER_RES_NO_PKT)) + if (!TEST_true(ossl_quic_tx_packetiser_generate(h.txp, &status)) + && !TEST_size_t_eq(status.sent_pkt, 0)) goto err; break;