From 44cb36d04adb737be1aee32908232003deeb67dd Mon Sep 17 00:00:00 2001 From: Tomas Mraz Date: Mon, 24 Jul 2023 18:05:47 +0200 Subject: Resolve some of the TODO(QUIC) items For some of the items we add FUTURE/SERVER/TESTING/MULTIPATH designation to indicate these do not need to be resolved in QUIC MVP release. Reviewed-by: Paul Dale Reviewed-by: Hugo Landau (Merged from https://github.com/openssl/openssl/pull/21539) --- ssl/quic/cc_newreno.c | 2 +- ssl/quic/quic_ackm.c | 5 +++-- ssl/quic/quic_channel.c | 16 ++++++++-------- ssl/quic/quic_impl.c | 10 ++++------ ssl/quic/quic_local.h | 2 +- ssl/quic/quic_reactor.c | 16 ++++++++-------- ssl/quic/quic_rstream.c | 1 - ssl/quic/quic_rx_depack.c | 12 ++++++------ ssl/quic/quic_stream_map.c | 2 +- ssl/quic/quic_trace.c | 4 ++-- ssl/quic/quic_txp.c | 4 ++-- ssl/quic/quic_wire_pkt.c | 2 +- 12 files changed, 37 insertions(+), 39 deletions(-) (limited to 'ssl/quic') diff --git a/ssl/quic/cc_newreno.c b/ssl/quic/cc_newreno.c index de4c10c6a2..1fe37c276e 100644 --- a/ssl/quic/cc_newreno.c +++ b/ssl/quic/cc_newreno.c @@ -36,7 +36,7 @@ typedef struct ossl_cc_newreno_st { #define MIN_MAX_INIT_WND_SIZE 14720 /* RFC 9002 s. 7.2 */ -/* TODO(QUIC): Pacing support. */ +/* TODO(QUIC FUTURE): Pacing support. */ static void newreno_set_max_dgram_size(OSSL_CC_NEWRENO *nr, size_t max_dgram_size); diff --git a/ssl/quic/quic_ackm.c b/ssl/quic/quic_ackm.c index 9e22bf2fc0..7c567eae77 100644 --- a/ssl/quic/quic_ackm.c +++ b/ssl/quic/quic_ackm.c @@ -927,7 +927,7 @@ static int ackm_set_loss_detection_timer(OSSL_ACKM *ackm) static int ackm_in_persistent_congestion(OSSL_ACKM *ackm, const OSSL_ACKM_TX_PKT *lpkt) { - /* TODO(QUIC): Persistent congestion not currently implemented. */ + /* TODO(QUIC FUTURE): Persistent congestion not currently implemented. */ return 0; } @@ -1284,7 +1284,8 @@ static void ackm_queue_probe_anti_deadlock_initial(OSSL_ACKM *ackm) static void ackm_queue_probe(OSSL_ACKM *ackm, int pkt_space) { /* - * TODO(QUIC): We are allowed to send either one or two probe packets here. + * TODO(QUIC FUTURE): We are allowed to send either one or two probe + * packets here. * Determine a strategy for when we should send two probe packets. */ ++ackm->pending_probe.pto[pkt_space]; diff --git a/ssl/quic/quic_channel.c b/ssl/quic/quic_channel.c index 54ccf5d316..af3a1d051a 100644 --- a/ssl/quic/quic_channel.c +++ b/ssl/quic/quic_channel.c @@ -21,8 +21,8 @@ * not suitable for network use. In particular, it does not implement address * validation, anti-amplification or retry logic. * - * TODO(QUIC): Implement address validation and anti-amplification - * TODO(QUIC): Implement retry logic + * TODO(QUIC SERVER): Implement address validation and anti-amplification + * TODO(QUIC SERVER): Implement retry logic */ #define INIT_DCID_LEN 8 @@ -1380,7 +1380,7 @@ static int ch_on_transport_params(const unsigned char *params, case QUIC_TPARAM_PREFERRED_ADDR: { - /* TODO(QUIC): Handle preferred address. */ + /* TODO(QUIC FUTURE): Handle preferred address. */ QUIC_PREFERRED_ADDR pfa; /* @@ -1684,7 +1684,7 @@ static void ch_tick(QUIC_TICK_RESULT *res, void *arg, uint32_t flags) * again because packets that were not previously processable and * were deferred might now be processable. * - * TODO(QUIC): Consider handling this in the yield_secret callback. + * TODO(QUIC FUTURE): Consider handling this in the yield_secret callback. */ } while (ch->have_new_rx_secret); } @@ -1985,7 +1985,7 @@ static void ch_rx_handle_packet(QUIC_CHANNEL *ch) return; /* - * TODO(QUIC): Theoretically this should probably be in the QRX. + * TODO(QUIC FUTURE): Theoretically this should probably be in the QRX. * However because validation is dependent on context (namely the * client's initial DCID) we can't do this cleanly. In the future we * should probably add a callback to the QRX to let it call us (via @@ -2011,8 +2011,8 @@ static void ch_rx_handle_packet(QUIC_CHANNEL *ch) return; /* - * TODO(QUIC): Implement 0-RTT on the server side. We currently do - * not need to implement this as a client can only do 0-RTT if we + * TODO(QUIC 0RTT): Implement 0-RTT on the server side. We currently + * do not need to implement this as a client can only do 0-RTT if we * have given it permission to in a previous session. */ break; @@ -2124,7 +2124,7 @@ static void ch_default_packet_handler(QUIC_URXE *e, void *arg) case QUIC_VERSION_NONE: default: /* Unknown version or proactive version negotiation request, bail. */ - /* TODO(QUIC): Handle version negotiation on server side */ + /* TODO(QUIC SERVER): Handle version negotiation on server side */ goto undesirable; } diff --git a/ssl/quic/quic_impl.c b/ssl/quic/quic_impl.c index 89475f3889..ca00fcd476 100644 --- a/ssl/quic/quic_impl.c +++ b/ssl/quic/quic_impl.c @@ -375,7 +375,7 @@ SSL *ossl_quic_new(SSL_CTX *ctx) = (ssl_base->method == OSSL_QUIC_client_thread_method()); #endif - qc->as_server = 0; /* TODO(QUIC): server support */ + qc->as_server = 0; /* TODO(QUIC SERVER): add server support */ qc->as_server_state = qc->as_server; qc->default_stream_mode = SSL_DEFAULT_STREAM_MODE_AUTO_BIDI; @@ -547,7 +547,7 @@ int ossl_quic_clear(SSL *s) if (!expect_quic(s, &ctx)) return 0; - /* TODO(QUIC): Currently a no-op. */ + /* TODO(QUIC FUTURE): Currently a no-op. */ return 1; } @@ -1172,7 +1172,6 @@ int ossl_quic_conn_shutdown(SSL *s, uint64_t flags, return -1; if (ctx.is_stream) - /* TODO(QUIC): Semantics currently undefined for QSSOs */ return -1; quic_lock(ctx.qc); @@ -1412,7 +1411,6 @@ static int quic_do_handshake(QCTX *ctx) } if (qc->as_server != qc->as_server_state) { - /* TODO(QUIC): Must match the method used to create the QCSO */ QUIC_RAISE_NON_NORMAL_ERROR(ctx, ERR_R_PASSED_INVALID_ARGUMENT, NULL); return -1; /* Non-protocol error */ } @@ -1800,8 +1798,8 @@ static void quic_post_write(QUIC_XSO *xso, int did_append, int do_tick) /* * Try and send. * - * TODO(QUIC): It is probably inefficient to try and do this immediately, - * plus we should eventually consider Nagle's algorithm. + * TODO(QUIC FUTURE): It is probably inefficient to try and do this + * immediately, plus we should eventually consider Nagle's algorithm. */ if (do_tick) ossl_quic_reactor_tick(ossl_quic_channel_get_reactor(xso->conn->ch), 0); diff --git a/ssl/quic/quic_local.h b/ssl/quic/quic_local.h index eb6d3e4e6f..d00a63d556 100644 --- a/ssl/quic/quic_local.h +++ b/ssl/quic/quic_local.h @@ -293,7 +293,7 @@ const SSL_METHOD *func_name(void) \ ossl_quic_free, \ ossl_quic_reset, \ ossl_quic_init, \ - ossl_quic_clear, \ + NULL /* clear */, \ ossl_quic_deinit, \ q_accept, \ q_connect, \ diff --git a/ssl/quic/quic_reactor.c b/ssl/quic/quic_reactor.c index ef447b78c3..f89337b38e 100644 --- a/ssl/quic/quic_reactor.c +++ b/ssl/quic/quic_reactor.c @@ -342,14 +342,14 @@ int ossl_quic_reactor_block_until_pred(QUIC_REACTOR *rtor, * things again. If poll_two_fds returns 0, this is some other * non-timeout failure and we should stop here. * - * TODO(QUIC): In the future we could avoid unnecessary syscalls by - * not retrying network I/O that isn't ready based on the result of - * the poll call. However this might be difficult because it - * requires we do the call to poll(2) or equivalent syscall - * ourselves, whereas in the general case the application does the - * polling and just calls SSL_handle_events(). Implementing this - * optimisation in the future will probably therefore require API - * changes. + * TODO(QUIC FUTURE): In the future we could avoid unnecessary + * syscalls by not retrying network I/O that isn't ready based + * on the result of the poll call. However this might be difficult + * because it requires we do the call to poll(2) or equivalent + * syscall ourselves, whereas in the general case the application + * does the polling and just calls SSL_handle_events(). + * Implementing this optimisation in the future will probably + * therefore require API changes. */ return 0; } diff --git a/ssl/quic/quic_rstream.c b/ssl/quic/quic_rstream.c index 0b3c870661..c51bc2014c 100644 --- a/ssl/quic/quic_rstream.c +++ b/ssl/quic/quic_rstream.c @@ -280,7 +280,6 @@ int ossl_quic_rstream_move_to_rbuf(QUIC_RSTREAM *qrs) int ossl_quic_rstream_resize_rbuf(QUIC_RSTREAM *qrs, size_t rbuf_size) { - /* TODO(QUIC): Do we need to distinguish different error conditions ? */ if (ossl_sframe_list_is_head_locked(&qrs->fl)) return 0; diff --git a/ssl/quic/quic_rx_depack.c b/ssl/quic/quic_rx_depack.c index 9e10a0a032..a20aac61bf 100644 --- a/ssl/quic/quic_rx_depack.c +++ b/ssl/quic/quic_rx_depack.c @@ -332,7 +332,7 @@ static int depack_do_frame_new_token(PACKET *pkt, QUIC_CHANNEL *ch, return 0; } - /* TODO(QUIC): ADD CODE to send |token| to the session manager */ + /* TODO(QUIC FUTURE): ADD CODE to send |token| to the session manager */ return 1; } @@ -881,7 +881,7 @@ static int depack_do_frame_retire_conn_id(PACKET *pkt, * currently non-conformant and for internal testing use; simply handle it * as a no-op in this case. * - * TODO(QUIC): Revise and implement correctly for server support. + * TODO(QUIC SERVER): Revise and implement correctly for server support. */ if (!ch->is_server) { ossl_quic_channel_raise_protocol_error(ch, @@ -968,7 +968,7 @@ static int depack_do_frame_path_response(PACKET *pkt, return 0; } - /* TODO(QUIC): ADD CODE to send |frame_data| to the ch manager */ + /* TODO(QUIC MULTIPATH): ADD CODE to send |frame_data| to the ch manager */ return 1; } @@ -1378,8 +1378,8 @@ int ossl_quic_handle_frames(QUIC_CHANNEL *ch, OSSL_QRX_PKT *qpacket) /* Initialize |ackm_data| (and reinitialize |ok|)*/ memset(&ackm_data, 0, sizeof(ackm_data)); /* - * TODO(QUIC): ASSUMPTION: All packets that aren't special case have a - * packet number + * ASSUMPTION: All packets that aren't special case have a + * packet number. */ ackm_data.pkt_num = qpacket->pn; ackm_data.time = qpacket->time; @@ -1413,7 +1413,7 @@ int ossl_quic_handle_frames(QUIC_CHANNEL *ch, OSSL_QRX_PKT *qpacket) ok = 1; end: /* - * TODO(QUIC): ASSUMPTION: If this function is called at all, |qpacket| is + * ASSUMPTION: If this function is called at all, |qpacket| is * a legitimate packet, even if its contents aren't. * Therefore, we call ossl_ackm_on_rx_packet() unconditionally, as long as * |ackm_data| has at least been initialized. diff --git a/ssl/quic/quic_stream_map.c b/ssl/quic/quic_stream_map.c index b23429a378..5d4354a2df 100644 --- a/ssl/quic/quic_stream_map.c +++ b/ssl/quic/quic_stream_map.c @@ -290,7 +290,7 @@ static ossl_unused int qsm_send_part_permits_gc(const QUIC_STREAM *qs) static int qsm_ready_for_gc(QUIC_STREAM_MAP *qsm, QUIC_STREAM *qs) { - int recv_stream_fully_drained = 0; /* TODO(QUIC): Optimisation */ + int recv_stream_fully_drained = 0; /* TODO(QUIC FUTURE): Optimisation */ /* * If sstream has no FIN, we auto-reset it at marked-for-deletion time, so diff --git a/ssl/quic/quic_trace.c b/ssl/quic/quic_trace.c index 5a57e675f9..d881140294 100644 --- a/ssl/quic/quic_trace.c +++ b/ssl/quic/quic_trace.c @@ -578,8 +578,8 @@ int ossl_quic_trace(int write_p, int version, int content_type, return 0; /* Decode the packet header */ /* - * TODO(QUIC): We need to query the short connection id len here, - * e.g. via some API SSL_get_short_conn_id_len() + * TODO(QUIC SERVER): We need to query the short connection id len + * here, e.g. via some API SSL_get_short_conn_id_len() */ if (ossl_quic_wire_decode_pkt_hdr(&pkt, 0, 0, 1, &hdr, NULL) != 1) return 0; diff --git a/ssl/quic/quic_txp.c b/ssl/quic/quic_txp.c index 4768cd856b..1107a0778d 100644 --- a/ssl/quic/quic_txp.c +++ b/ssl/quic/quic_txp.c @@ -1276,7 +1276,7 @@ static int txp_should_try_staging(OSSL_QUIC_TX_PACKETISER *txp, * This is not a major concern for clients, since if a client has a 1-RTT EL * provisioned the server is guaranteed to also have a 1-RTT EL provisioned. * - * TODO(QUIC): Revisit this when server support is added. + * TODO(QUIC SERVER): Revisit this when server support is added. */ if (*conn_close_enc_level > enc_level && *conn_close_enc_level != QUIC_ENC_LEVEL_1RTT) @@ -1402,7 +1402,7 @@ static int sstream_is_pending(QUIC_SSTREAM *sstream) /* Determine how many bytes we should use for the encoded PN. */ static size_t txp_determine_pn_len(OSSL_QUIC_TX_PACKETISER *txp) { - return 4; /* TODO(QUIC) */ + return 4; /* TODO(QUIC FUTURE) */ } /* Determine plaintext packet payload length from payload length. */ diff --git a/ssl/quic/quic_wire_pkt.c b/ssl/quic/quic_wire_pkt.c index a96843aa9f..5185d7821b 100644 --- a/ssl/quic/quic_wire_pkt.c +++ b/ssl/quic/quic_wire_pkt.c @@ -854,7 +854,7 @@ int ossl_quic_calculate_retry_integrity_tag(OSSL_LIB_CTX *libctx, return 0; /* Create and initialise cipher context. */ - /* TODO(QUIC): Cipher fetch caching. */ + /* TODO(QUIC FUTURE): Cipher fetch caching. */ if ((cipher = EVP_CIPHER_fetch(libctx, "AES-128-GCM", propq)) == NULL) goto err; -- cgit v1.2.3