Skip to content

Commit

Permalink
quic: fix hs_data_empty cache invalidation bug
Browse files Browse the repository at this point in the history
Fixes an issue where QUIC handshakes can stall if fd_quic has
nothing to send temporarily.  This occurs specifically when the
client sends an Initial packet that doesn't contain a complete
ClientHello.  This caused 'hs_data_empty' flag to be set which then
suppresses any more TLS handshake progress.  There was no logic to
reset 'hs_data_empty' when new data arrives.

With this fix, we always check for handshake data for as long as
a quic_tls_hs object exists.  The 'hs_data_empty' bit is removed.
  • Loading branch information
riptl authored and mmcgee-jump committed Jan 8, 2025
1 parent d36df1e commit 4dbff0a
Show file tree
Hide file tree
Showing 6 changed files with 7 additions and 20 deletions.
19 changes: 6 additions & 13 deletions src/waltz/quic/fd_quic.c
Original file line number Diff line number Diff line change
Expand Up @@ -887,7 +887,7 @@ fd_quic_tx_enc_level( fd_quic_conn_t * conn, int acks ) {

/* TODO consider this optimization... but we want to ack all handshakes, even if there is stream_data */
case FD_QUIC_CONN_STATE_ACTIVE:
if( FD_LIKELY( conn->hs_data_empty ) ) {
if( FD_LIKELY( !conn->tls_hs ) ) {
/* optimization for case where we have stream data to send */

/* find stream data to send */
Expand All @@ -908,9 +908,8 @@ fd_quic_tx_enc_level( fd_quic_conn_t * conn, int acks ) {
}

/* Check for handshake data to send */
/* hs_data_empty is used to optimize the test */
uint peer_enc_level = conn->peer_enc_level;
if( FD_UNLIKELY( !conn->hs_data_empty ) ) {
if( FD_UNLIKELY( conn->tls_hs ) ) {
fd_quic_tls_hs_data_t * hs_data = NULL;

for( uint i = peer_enc_level; i < 4 && i < enc_level; ++i ) {
Expand All @@ -930,9 +929,6 @@ fd_quic_tx_enc_level( fd_quic_conn_t * conn, int acks ) {
}
}
}

/* no hs_data was found, so set hs_data_empty */
conn->hs_data_empty = 1;
}

/* if we have acks to send or handshake data, then use that enc_level */
Expand Down Expand Up @@ -1610,6 +1606,7 @@ fd_quic_handle_v1_initial( fd_quic_t * quic,
FD_DEBUG( FD_LOG_WARNING( ( "failed to allocate QUIC conn" ) ) );
return FD_QUIC_PARSE_FAIL;
}
FD_DEBUG( FD_LOG_DEBUG(( "new connection allocated" )) );

/* set the value for the caller */
*p_conn = conn;
Expand Down Expand Up @@ -1769,8 +1766,6 @@ fd_quic_handle_v1_initial( fd_quic_t * quic,
conn->exp_pkt_number[pn_space] = fd_ulong_max( conn->exp_pkt_number[pn_space], next_pkt_number );
} while(0);

FD_DEBUG( FD_LOG_DEBUG(( "new connection success" )) );

/* insert into service queue */
fd_quic_svc_schedule( state, conn, FD_QUIC_SVC_INSTANT );

Expand Down Expand Up @@ -1988,7 +1983,6 @@ fd_quic_handle_v1_retry(
/* have to rewind the handshake data */
uint enc_level = fd_quic_enc_level_initial_id;
conn->hs_sent_bytes[enc_level] = 0;
conn->hs_data_empty = 0;

/* send the INITIAL */
conn->upd_pkt_number = FD_QUIC_PKT_NUM_PENDING;
Expand Down Expand Up @@ -3534,7 +3528,7 @@ fd_quic_gen_frames( fd_quic_conn_t * conn,
payload_ptr += fd_quic_gen_max_streams_frame( conn, payload_ptr, payload_end, pkt_meta, pkt_number, now );
payload_ptr += fd_quic_gen_ping_frame ( conn, payload_ptr, payload_end, pkt_number );
}
if( FD_LIKELY( conn->hs_data_empty ) ) {
if( FD_LIKELY( !conn->tls_hs ) ) {
payload_ptr = fd_quic_gen_stream_frames( conn, payload_ptr, payload_end, pkt_meta, pkt_number, now );
}
}
Expand Down Expand Up @@ -3957,8 +3951,9 @@ fd_quic_conn_tx( fd_quic_t * quic,
break;
}

/* choose enc_level to tx at */
/* Refresh enc_level in case we can coalesce another packet */
enc_level = fd_quic_tx_enc_level( conn, 0 /* acks */ );
FD_DEBUG( if( enc_level!=~0u) FD_LOG_DEBUG(( "Attempting to append enc_level=%u packet", enc_level )); )
}

/* unused pkt_meta? deallocate */
Expand Down Expand Up @@ -4348,7 +4343,6 @@ fd_quic_conn_create( fd_quic_t * quic,
conn->handshake_complete = 0;
conn->handshake_done_send = 0;
conn->handshake_done_ackd = 0;
conn->hs_data_empty = 0;
conn->tls_hs = NULL; /* created later */

/* initialize stream_id members */
Expand Down Expand Up @@ -4753,7 +4747,6 @@ fd_quic_reclaim_pkt_meta( fd_quic_conn_t * conn,
if( flags & FD_QUIC_PKT_META_FLAGS_HS_DONE ) {
conn->handshake_done_ackd = 1;
conn->handshake_done_send = 0;
conn->hs_data_empty = 1;
fd_quic_state_t * state = fd_quic_get_state( conn->quic );
fd_quic_tls_hs_delete( conn->tls_hs );
fd_quic_tls_hs_pool_ele_release( state->hs_pool, conn->tls_hs );
Expand Down
1 change: 0 additions & 1 deletion src/waltz/quic/fd_quic_conn.h
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,6 @@ struct fd_quic_conn {
uint handshake_complete : 1; /* have we completed a successful handshake? */
uint handshake_done_send : 1; /* do we need to send handshake-done to peer? */
uint handshake_done_ackd : 1; /* was handshake_done ack'ed? */
uint hs_data_empty : 1; /* has all hs_data been consumed? */
fd_quic_tls_hs_t * tls_hs;

/* amount of handshake data already sent from head of queue */
Expand Down
1 change: 0 additions & 1 deletion src/waltz/quic/tests/fd_quic_sandbox.c
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,6 @@ fd_quic_sandbox_new_conn_established( fd_quic_sandbox_t * sandbox,

/* Mock a completed handshake */
conn->handshake_complete = 1;
conn->hs_data_empty = 1;
conn->peer_enc_level = fd_quic_enc_level_appdata_id;
conn->keys_avail = 1U<<fd_quic_enc_level_appdata_id;

Expand Down
1 change: 0 additions & 1 deletion src/waltz/quic/tests/test_quic_tls_hs.c
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ static uchar const test_tp[] =
typedef struct my_quic_tls my_quic_tls_t;
struct my_quic_tls {
int is_server;
int is_flush;
int is_hs_complete;

int state;
Expand Down
4 changes: 1 addition & 3 deletions src/waltz/quic/tls/fd_quic_tls.c
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,6 @@ fd_quic_tls_hs_new( fd_quic_tls_hs_t * self,
// set properties on self
self->quic_tls = quic_tls;
self->is_server = is_server;
self->is_flush = 0;
self->context = context;

/* initialize handshake data */
Expand Down Expand Up @@ -266,7 +265,7 @@ fd_quic_tls_sendmsg( void const * handshake,
void const * data,
ulong data_sz,
uint enc_level,
int flush ) {
int flush FD_PARAM_UNUSED ) {

uint buf_sz = FD_QUIC_TLS_HS_DATA_SZ;
if( data_sz > buf_sz ) {
Expand All @@ -276,7 +275,6 @@ fd_quic_tls_sendmsg( void const * handshake,
/* Safe because the fd_tls_estate_{srv,cli}_t object is the first
element of fd_quic_tls_hs_t */
fd_quic_tls_hs_t * hs = (fd_quic_tls_hs_t *)handshake;
hs->is_flush |= flush;

/* add handshake data to handshake for retrieval by user */

Expand Down
1 change: 0 additions & 1 deletion src/waltz/quic/tls/fd_quic_tls.h
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,6 @@ struct fd_quic_tls_hs {
fd_quic_tls_t * quic_tls;

int is_server;
int is_flush;
int is_hs_complete;

/* user defined context supplied in callbacks */
Expand Down

0 comments on commit 4dbff0a

Please sign in to comment.