diff --git a/CHANGELOG b/CHANGELOG index c78b87cfc..6d4e586c7 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,21 @@ +2019-10-08 + - 2.4.4 + - [API] Add lsquic_alpn2ver() to aid parsing Alt-Svc header. + - [BUGFIX] NULL dereference when H3 frame header would be split. + - [BUGFIX] Do not close fixed-size H3 frame prematurely. + - [BUGFIX] Allow PING frames in IETF mini conn. + - [BUGFIX] Mini conns: don't send any packets after receiving + CONNECTION_CLOSE. + - [BUGFIX] Client migration: reserve slot for DCID from transport params. + - [BUGFIX] Allow max_early_data_size=0 -- early_data might not be there. + - [BUGFIX] Use an invalid stream number to reset BPT cache (zero is now a + valid stream number). + - [SPEC] Use FINAL_SIZE_ERROR when FIN mismatch is detected. + - [OPTIMIZATION] Closed connection only gets one chance to send packets. + - [OPTIMIZATION] Flush headers stream before packetizing stream data. + - [OPTIMIZATION] process QPACK encoder STREAM frames immediately. + - Update ls-qpack to v0.10.1. + 2019-09-30 - 2.4.3 - Add GQUIC versions to the list of h3 ALPNs for Alt-Svc header. diff --git a/CMakeLists.txt b/CMakeLists.txt index 0fc9b12eb..ce30364aa 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -20,6 +20,8 @@ ENDIF() MESSAGE(STATUS "Build type: ${CMAKE_BUILD_TYPE}") +OPTION(LSQUIC_FIU "Use Fault Injection in Userspace (FIU)" OFF) + IF (NOT MSVC) @@ -35,6 +37,11 @@ IF(CMAKE_COMPILER_IS_GNUCC AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 4.9.3) SET(MY_CMAKE_FLAGS "${MY_CMAKE_FLAGS} -Wno-missing-field-initializers") ENDIF() +IF(LSQUIC_FIU) + SET(MY_CMAKE_FLAGS "${MY_CMAKE_FLAGS} -DFIU_ENABLE=1") + SET(LIBS ${LIBS} fiu) +ENDIF() + IF(CMAKE_BUILD_TYPE STREQUAL "Debug") SET(MY_CMAKE_FLAGS "${MY_CMAKE_FLAGS} -O0 -g3") SET(MY_CMAKE_FLAGS "${MY_CMAKE_FLAGS} -Werror") @@ -42,11 +49,8 @@ IF(CMAKE_BUILD_TYPE STREQUAL "Debug") NOT "$ENV{TRAVIS}" MATCHES "^true$") SET(MY_CMAKE_FLAGS "${MY_CMAKE_FLAGS} -fsanitize=address") ENDIF() - # Uncomment to enable fault injection testing via libfiu: - #SET (MY_CMAKE_FLAGS "${MY_CMAKE_FLAGS} -DFIU_ENABLE=1") # Uncomment to enable cleartext protocol mode (no crypto): #SET (MY_CMAKE_FLAGS "${MY_CMAKE_FLAGS} -DLSQUIC_ENABLE_HANDSHAKE_DISABLE=1") - #SET(LIBS ${LIBS} fiu) ELSE() SET(MY_CMAKE_FLAGS "${MY_CMAKE_FLAGS} -O3 -g0") # Comment out the following line to compile out debug messages: @@ -187,7 +191,7 @@ ENDIF() IF (CMAKE_SYSTEM_NAME STREQUAL Windows) FIND_LIBRARY(EVENT_LIB event) ELSE() - FIND_LIBRARY(EVENT_LIB libevent.a) + FIND_LIBRARY(EVENT_LIB libevent.a libevent.so) ENDIF() IF(EVENT_LIB) MESSAGE(STATUS "Found event: ${EVENT_LIB}") diff --git a/include/lsquic.h b/include/lsquic.h index 5537ff197..cbbd2e029 100644 --- a/include/lsquic.h +++ b/include/lsquic.h @@ -25,7 +25,7 @@ extern "C" { #define LSQUIC_MAJOR_VERSION 2 #define LSQUIC_MINOR_VERSION 4 -#define LSQUIC_PATCH_VERSION 3 +#define LSQUIC_PATCH_VERSION 4 /** * Engine flags: @@ -1505,6 +1505,10 @@ lsquic_conn_crypto_cipher (const lsquic_conn_t *c); enum lsquic_version lsquic_str2ver (const char *str, size_t len); +/** Translate ALPN (e.g. "h3", "h3-23", "h3-Q046") to LSQUIC enum */ +enum lsquic_version +lsquic_alpn2ver (const char *alpn, size_t len); + /** * This function closes all mini connections and marks all full connection * as going away. In server mode, this also causes the engine to stop diff --git a/src/liblsquic/gen-verstrs.pl b/src/liblsquic/gen-verstrs.pl index 7cfbd40e7..6fac68b05 100755 --- a/src/liblsquic/gen-verstrs.pl +++ b/src/liblsquic/gen-verstrs.pl @@ -37,6 +37,7 @@ */ #include +#include #include "lsquic.h" @@ -127,4 +128,37 @@ C_CODE +print OUT <<'C_CODE'; + +enum lsquic_version +lsquic_alpn2ver (const char *alpn, size_t len) +{ + static const struct el { + size_t len; + char alpn[10]; + enum lsquic_version version; + } map[] = { +C_CODE + +for ($i = 0; $i < @versions; ++$i) { + print OUT " {sizeof(\"h3-Q0$versions[$i]\")-1,\"h3-Q0$versions[$i]\", $enums[$i]},\n"; +} + +for ($i = 0; $i < @draft_versions; ++$i) { + print OUT " {sizeof(\"h3-$draft_versions[$i]\")-1,\"h3-$draft_versions[$i]\", LSQVER_ID$draft_versions[$i]},\n"; +} + +print OUT <<'C_CODE'; + }; + const struct el *el; + + if (alpn) + for (el = map; el < map + sizeof(map) / sizeof(map[0]); ++el) + if (el->len == len && 0 == strncmp(el->alpn, alpn, len)) + return el->version; + + return -1; +} +C_CODE + close OUT; diff --git a/src/liblsquic/lsquic_enc_sess_ietf.c b/src/liblsquic/lsquic_enc_sess_ietf.c index 3e806f300..413654749 100644 --- a/src/liblsquic/lsquic_enc_sess_ietf.c +++ b/src/liblsquic/lsquic_enc_sess_ietf.c @@ -1154,8 +1154,13 @@ iquic_new_session_cb (SSL *ssl, SSL_SESSION *session) assert(enc_sess->esi_enpub->enp_stream_if->on_zero_rtt_info); max_early_data_size = SSL_SESSION_get_max_early_data_size(session); - if (0xFFFFFFFFu != max_early_data_size) + if (max_early_data_size && 0xFFFFFFFFu != max_early_data_size) { + /* XXX We do not catch the case when early_data extension is present + * and max_early_data_size is set to zero, which is an invalid value. + * This is because there is no way to check this using existing + * BoringSSL APIs. + */ /* See [draft-ietf-quic-tls-23], Section 4.5 */ LSQ_INFO("max_early_data_size=0x%X, protocol violation", max_early_data_size); diff --git a/src/liblsquic/lsquic_engine.c b/src/liblsquic/lsquic_engine.c index c1e515ec6..19364a9c4 100644 --- a/src/liblsquic/lsquic_engine.c +++ b/src/liblsquic/lsquic_engine.c @@ -1922,7 +1922,10 @@ coi_reheap (struct conns_out_iter *iter, lsquic_engine_t *engine) { TAILQ_REMOVE(&iter->coi_active_list, conn, cn_next_out); conn->cn_flags &= ~LSCONN_COI_ACTIVE; - lsquic_mh_insert(iter->coi_heap, conn, conn->cn_last_sent); + if ((conn->cn_flags & CONN_REF_FLAGS) != LSCONN_HAS_OUTGOING) + lsquic_mh_insert(iter->coi_heap, conn, conn->cn_last_sent); + else /* Closed connection gets one shot at sending packets */ + (void) engine_decref_conn(engine, conn, LSCONN_HAS_OUTGOING); } while ((conn = TAILQ_FIRST(&iter->coi_inactive_list))) { diff --git a/src/liblsquic/lsquic_full_conn_ietf.c b/src/liblsquic/lsquic_full_conn_ietf.c index 4e91f9793..42de51763 100644 --- a/src/liblsquic/lsquic_full_conn_ietf.c +++ b/src/liblsquic/lsquic_full_conn_ietf.c @@ -4507,6 +4507,12 @@ process_stream_frame (struct ietf_full_conn *conn, return 0; } + /* Don't wait for the regular on_read dispatch in order to save an + * unnecessary blocked/unblocked sequence. + */ + if ((conn->ifc_flags & IFC_HTTP) && conn->ifc_qdh.qdh_enc_sm_in == stream) + lsquic_stream_dispatch_read_events(conn->ifc_qdh.qdh_enc_sm_in); + return parsed_len; } @@ -4809,6 +4815,30 @@ retire_dcids_prior_to (struct ietf_full_conn *conn, unsigned retire_prior_to) } +/* We need to be able to allocate a DCE slot to begin migration or to retire + * the DCID in transport parameters. + */ +static int +must_reserve_one_dce_slot (struct ietf_full_conn *conn) +{ + struct lsquic_conn *const lconn = &conn->ifc_conn; + const struct transport_params *params; + + if (conn->ifc_flags & IFC_SERVER) + return 0; + + if (lsquic_send_ctl_1rtt_acked(&conn->ifc_send_ctl)) + return 0; + + params = lconn->cn_esf.i->esfi_get_peer_transport_params( + lconn->cn_enc_session); + if (params) /* Just in case */ + return !!(params->tp_flags & (TRAPA_PREFADDR_IPv4|TRAPA_PREFADDR_IPv6)); + else + return 0; +} + + static unsigned process_new_connection_id_frame (struct ietf_full_conn *conn, struct lsquic_packet_in *packet_in, const unsigned char *p, size_t len) @@ -4883,6 +4913,16 @@ process_new_connection_id_frame (struct ietf_full_conn *conn, if (dce) { + if (must_reserve_one_dce_slot(conn)) + { + for (el = dce + 1; el < DCES_END(conn) && *el; ++el) + ; + if (el == DCES_END(conn)) + { + action_str = "Ignored (last slot reserved for migration)"; + goto end; + } + } *dce = lsquic_malo_get(conn->ifc_pub.mm->malo.dcid_elem); if (*dce) { @@ -4901,6 +4941,7 @@ process_new_connection_id_frame (struct ietf_full_conn *conn, else action_str = "Ignored (no slots available)"; + end: LSQ_DEBUGC("Got new connection ID from peer: seq=%"PRIu64"; " "cid: %"CID_FMT". %s.", seqno, CID_BITS(&cid), action_str); return parsed_len; diff --git a/src/liblsquic/lsquic_mini_conn.c b/src/liblsquic/lsquic_mini_conn.c index 77ecff8fe..b4ae44a4f 100644 --- a/src/liblsquic/lsquic_mini_conn.c +++ b/src/liblsquic/lsquic_mini_conn.c @@ -342,6 +342,24 @@ process_blocked_frame (struct mini_conn *mc, lsquic_packet_in_t *packet_in, } +static mconn_packno_set_t +drop_packets_out (struct mini_conn *mc) +{ + struct lsquic_packet_out *packet_out; + mconn_packno_set_t in_flight = 0; + + while ((packet_out = TAILQ_FIRST(&mc->mc_packets_out))) + { + TAILQ_REMOVE(&mc->mc_packets_out, packet_out, po_next); + if (packet_out->po_flags & PO_SENT) + in_flight |= MCONN_PACKET_MASK(packet_out->po_packno); + mini_destroy_packet(mc, packet_out); + } + + return in_flight; +} + + static unsigned process_connection_close_frame (struct mini_conn *mc, lsquic_packet_in_t *packet_in, const unsigned char *p, size_t len) @@ -350,6 +368,8 @@ process_connection_close_frame (struct mini_conn *mc, uint16_t reason_len; uint8_t reason_off; int parsed_len; + + (void) drop_packets_out(mc); parsed_len = mc->mc_conn.cn_pf->pf_parse_connect_close_frame(p, len, NULL, &error_code, &reason_len, &reason_off); if (parsed_len < 0) @@ -1700,8 +1720,7 @@ mini_conn_ci_destroy (struct lsquic_conn *lconn) assert(!(lconn->cn_flags & LSCONN_HASHED)); struct mini_conn *mc = (struct mini_conn *) lconn; lsquic_packet_in_t *packet_in; - lsquic_packet_out_t *packet_out; - mconn_packno_set_t still_deferred = 0, in_flight = 0; + mconn_packno_set_t still_deferred = 0, in_flight; enum lsq_log_level log_level; #if LSQUIC_RECORD_INORD_HIST char inord_str[0x100]; @@ -1717,13 +1736,10 @@ mini_conn_ci_destroy (struct lsquic_conn *lconn) still_deferred |= MCONN_PACKET_MASK(packet_in->pi_packno); lsquic_packet_in_put(&mc->mc_enpub->enp_mm, packet_in); } - while ((packet_out = TAILQ_FIRST(&mc->mc_packets_out))) - { - TAILQ_REMOVE(&mc->mc_packets_out, packet_out, po_next); - if (packet_out->po_flags & PO_SENT) - in_flight |= MCONN_PACKET_MASK(packet_out->po_packno); - mini_destroy_packet(mc, packet_out); - } + if (TAILQ_EMPTY(&mc->mc_packets_out)) + in_flight = ~0ull; /* Indicates that packets were dropped before */ + else + in_flight = drop_packets_out(mc); if (mc->mc_conn.cn_enc_session) mc->mc_conn.cn_esf.g->esf_destroy(mc->mc_conn.cn_enc_session); log_level = warning_is_warranted(mc) ? LSQ_LOG_WARN : LSQ_LOG_DEBUG; diff --git a/src/liblsquic/lsquic_mini_conn_ietf.c b/src/liblsquic/lsquic_mini_conn_ietf.c index b7f6e8779..befbc43bc 100644 --- a/src/liblsquic/lsquic_mini_conn_ietf.c +++ b/src/liblsquic/lsquic_mini_conn_ietf.c @@ -798,6 +798,42 @@ imico_process_ack_frame (IMICO_PROC_FRAME_ARGS) } +static unsigned +imico_process_ping_frame (IMICO_PROC_FRAME_ARGS) +{ + LSQ_DEBUG("got a PING frame, do nothing"); + return 1; +} + + +static unsigned +imico_process_connection_close_frame (IMICO_PROC_FRAME_ARGS) +{ + struct lsquic_packet_out *packet_out; + uint64_t error_code; + uint16_t reason_len; + uint8_t reason_off; + int parsed_len, app_error; + + while ((packet_out = TAILQ_FIRST(&conn->imc_packets_out))) + { + TAILQ_REMOVE(&conn->imc_packets_out, packet_out, po_next); + imico_destroy_packet(conn, packet_out); + } + conn->imc_flags |= IMC_CLOSE_RECVD; + parsed_len = conn->imc_conn.cn_pf->pf_parse_connect_close_frame(p, len, + &app_error, &error_code, &reason_len, &reason_off); + if (parsed_len < 0) + return 0; + EV_LOG_CONNECTION_CLOSE_FRAME_IN(LSQUIC_LOG_CONN_ID, error_code, + (int) reason_len, (const char *) p + reason_off); + LSQ_INFO("Received CONNECTION_CLOSE frame (%s-level code: %"PRIu64"; " + "reason: %.*s)", app_error ? "application" : "transport", + error_code, (int) reason_len, (const char *) p + reason_off); + return 0; /* This shuts down the connection */ +} + + static unsigned imico_process_invalid_frame (IMICO_PROC_FRAME_ARGS) { @@ -814,15 +850,15 @@ static unsigned (*const imico_process_frames[N_QUIC_FRAMES]) [QUIC_FRAME_STREAM] = imico_process_stream_frame, [QUIC_FRAME_CRYPTO] = imico_process_crypto_frame, [QUIC_FRAME_ACK] = imico_process_ack_frame, + [QUIC_FRAME_PING] = imico_process_ping_frame, + [QUIC_FRAME_CONNECTION_CLOSE] = imico_process_connection_close_frame, /* XXX: Some of them are invalid, while others are unexpected. We treat * them the same: handshake cannot proceed. */ [QUIC_FRAME_RST_STREAM] = imico_process_invalid_frame, - [QUIC_FRAME_CONNECTION_CLOSE] = imico_process_invalid_frame, [QUIC_FRAME_MAX_DATA] = imico_process_invalid_frame, [QUIC_FRAME_MAX_STREAM_DATA] = imico_process_invalid_frame, [QUIC_FRAME_MAX_STREAMS] = imico_process_invalid_frame, - [QUIC_FRAME_PING] = imico_process_invalid_frame, [QUIC_FRAME_BLOCKED] = imico_process_invalid_frame, [QUIC_FRAME_STREAM_BLOCKED] = imico_process_invalid_frame, [QUIC_FRAME_STREAMS_BLOCKED] = imico_process_invalid_frame, @@ -1422,7 +1458,8 @@ ietf_mini_conn_ci_tick (struct lsquic_conn *lconn, lsquic_time_t now) if (conn->imc_flags & IMC_ERROR) { - imico_generate_conn_close(conn); + if (!(conn->imc_flags & IMC_CLOSE_RECVD)) + imico_generate_conn_close(conn); tick |= TICK_CLOSE; } else if (conn->imc_flags & IMC_HSK_OK) diff --git a/src/liblsquic/lsquic_mini_conn_ietf.h b/src/liblsquic/lsquic_mini_conn_ietf.h index c1c60823a..79300f7b2 100644 --- a/src/liblsquic/lsquic_mini_conn_ietf.h +++ b/src/liblsquic/lsquic_mini_conn_ietf.h @@ -54,6 +54,7 @@ struct ietf_mini_conn IMC_BAD_TRANS_PARAMS = 1 << 16, IMC_ADDR_VALIDATED = 1 << 17, IMC_HSK_PACKET_SENT = 1 << 18, + IMC_CLOSE_RECVD = 1 << 19, } imc_flags; struct mini_crypto_stream imc_streams[N_ENC_LEVS]; void *imc_stream_ps[N_ENC_LEVS]; diff --git a/src/liblsquic/lsquic_send_ctl.c b/src/liblsquic/lsquic_send_ctl.c index f4701dd5c..c4bdaabe2 100644 --- a/src/liblsquic/lsquic_send_ctl.c +++ b/src/liblsquic/lsquic_send_ctl.c @@ -345,6 +345,7 @@ lsquic_send_ctl_init (lsquic_send_ctl_t *ctl, struct lsquic_alarmset *alset, sizeof(ctl->sc_buffered_packets[0]); ++i) TAILQ_INIT(&ctl->sc_buffered_packets[i].bpq_packets); ctl->sc_max_packno_bits = PACKNO_BITS_2; /* Safe value before verneg */ + ctl->sc_cached_bpt.stream_id = UINT64_MAX; } @@ -2347,16 +2348,6 @@ lsquic_send_ctl_get_packet_for_stream (lsquic_send_ctl_t *ctl, } - -int -lsquic_send_ctl_buffered_and_same_prio_as_headers (struct lsquic_send_ctl *ctl, - const struct lsquic_stream *stream) -{ - return !lsquic_send_ctl_schedule_stream_packets_immediately(ctl) - && BPT_HIGHEST_PRIO == send_ctl_lookup_bpt(ctl, stream); -} - - #ifdef NDEBUG static #elif __GNUC__ diff --git a/src/liblsquic/lsquic_send_ctl.h b/src/liblsquic/lsquic_send_ctl.h index 8d27f75ad..e386cf6c8 100644 --- a/src/liblsquic/lsquic_send_ctl.h +++ b/src/liblsquic/lsquic_send_ctl.h @@ -282,7 +282,7 @@ lsquic_send_ctl_schedule_buffered (lsquic_send_ctl_t *, enum buf_packet_type); || TAILQ_FIRST(&(ctl)->sc_buffered_packets[BPT_OTHER_PRIO].bpq_packets )) #define lsquic_send_ctl_invalidate_bpt_cache(ctl) do { \ - (ctl)->sc_cached_bpt.stream_id = 0; \ + (ctl)->sc_cached_bpt.stream_id = UINT64_MAX; \ } while (0) #ifndef NDEBUG @@ -328,11 +328,6 @@ lsquic_send_ctl_pacer_blocked (struct lsquic_send_ctl *); int lsquic_send_ctl_sched_is_blocked (struct lsquic_send_ctl *); -int - -lsquic_send_ctl_buffered_and_same_prio_as_headers (struct lsquic_send_ctl *, - const struct lsquic_stream *); - void lsquic_send_ctl_verneg_done (struct lsquic_send_ctl *); diff --git a/src/liblsquic/lsquic_stream.c b/src/liblsquic/lsquic_stream.c index 253eae030..e7b16a193 100644 --- a/src/liblsquic/lsquic_stream.c +++ b/src/liblsquic/lsquic_stream.c @@ -870,6 +870,7 @@ lsquic_stream_frame_in (lsquic_stream_t *stream, stream_frame_t *frame) uint64_t max_off; int got_next_offset, rv, free_frame; enum ins_frame ins_frame; + struct lsquic_conn *lconn; assert(frame->packet_in); @@ -885,6 +886,18 @@ lsquic_stream_frame_in (lsquic_stream_t *stream, stream_frame_t *frame) return -1; } + if (frame->data_frame.df_fin && (stream->sm_bflags & SMBF_IETF) + && (stream->stream_flags & STREAM_FIN_RECVD) + && stream->sm_fin_off != DF_END(frame)) + { + lconn = stream->conn_pub->lconn; + lconn->cn_if->ci_abort_error(lconn, 0, TEC_FINAL_SIZE_ERROR, + "new final size %"PRIu64" from STREAM frame (id: %"PRIu64") does " + "not match previous final size %"PRIu64, DF_END(frame), + stream->id, stream->sm_fin_off); + return -1; + } + got_next_offset = frame->data_frame.df_offset == stream->read_offset; insert_frame: ins_frame = stream->data_in->di_if->di_insert_frame(stream->data_in, frame, stream->read_offset); @@ -981,6 +994,19 @@ int lsquic_stream_rst_in (lsquic_stream_t *stream, uint64_t offset, uint64_t error_code) { + struct lsquic_conn *lconn; + + if ((stream->sm_bflags & SMBF_IETF) + && (stream->stream_flags & STREAM_FIN_RECVD) + && stream->sm_fin_off != offset) + { + lconn = stream->conn_pub->lconn; + lconn->cn_if->ci_abort_error(lconn, 0, TEC_FINAL_SIZE_ERROR, + "final size %"PRIu64" from RESET_STREAM frame (id: %"PRIu64") " + "does not match previous final size %"PRIu64, offset, + stream->id, stream->sm_fin_off); + return -1; + } if (stream->stream_flags & STREAM_RST_RECVD) { @@ -2502,12 +2528,18 @@ stream_activate_hq_frame (struct lsquic_stream *stream, uint64_t off, shf->shf_flags |= flags; shf->shf_frame_type = frame_type; if (shf->shf_flags & SHF_FIXED_SIZE) + { shf->shf_frame_size = size; + LSQ_DEBUG("activated fixed-size HQ frame of type 0x%X at offset " + "%"PRIu64", size %zu", shf->shf_frame_type, shf->shf_off, size); + } else { shf->shf_frame_ptr = NULL; if (size >= (1 << 6)) shf->shf_flags |= SHF_TWO_BYTES; + LSQ_DEBUG("activated variable-size HQ frame of type 0x%X at offset " + "%"PRIu64, shf->shf_frame_type, shf->shf_off); } return shf; @@ -2528,7 +2560,10 @@ frame_hq_gen_read (void *ctx, void *begin_buf, size_t len, int *fin) while (p < end) { shf = find_cur_hq_frame(stream); - if (!shf) + if (shf) + LSQ_DEBUG("found current HQ frame of type 0x%X at offset %"PRIu64, + shf->shf_frame_type, shf->shf_off); + else { rem = frame_std_gen_size(ctx); if (rem) @@ -2537,7 +2572,11 @@ frame_hq_gen_read (void *ctx, void *begin_buf, size_t len, int *fin) rem = (1 << 14) - 1; shf = stream_activate_hq_frame(stream, stream->sm_payload, HQFT_DATA, 0, rem); - /* XXX malloc can fail */ + if (!shf) + { + /* TODO: abort connection? Handle failure somehow */ + break; + } } else break; @@ -2548,7 +2587,10 @@ frame_hq_gen_read (void *ctx, void *begin_buf, size_t len, int *fin) { frame_sz = stream_hq_frame_size(shf); if (frame_sz > (uintptr_t) (end - p)) + { + stream_hq_frame_put(stream, shf); break; + } LSQ_DEBUG("insert %zu-byte HQ frame of type 0x%X at payload " "offset %"PRIu64" (actual offset %"PRIu64")", frame_sz, shf->shf_frame_type, stream->sm_payload, stream->tosend_off); @@ -2705,32 +2747,43 @@ stream_write_to_packet_std (struct frame_gen_ctx *fg_ctx, const size_t size) struct lsquic_send_ctl *const send_ctl = stream->conn_pub->send_ctl; unsigned stream_header_sz, need_at_least; struct lsquic_packet_out *packet_out; + struct lsquic_stream *headers_stream; int len; - if (!(stream->sm_bflags & SMBF_IETF) - && (stream->stream_flags & - (STREAM_HEADERS_SENT|STREAM_HDRS_FLUSHED)) - == STREAM_HEADERS_SENT - && lsquic_send_ctl_buffered_and_same_prio_as_headers(send_ctl, stream)) + if ((stream->stream_flags & (STREAM_HEADERS_SENT|STREAM_HDRS_FLUSHED)) + == STREAM_HEADERS_SENT) { - /* TODO: make this logic work for IETF streams as well XXX */ - struct lsquic_stream *const headers_stream - = lsquic_headers_stream_get_stream(stream->conn_pub->u.gquic.hs); - if (lsquic_stream_has_data_to_flush(headers_stream)) + /* Optimization idea: the QPACK encoder stream needs only be flushed + * if the headers in this stream are dependent on the buffered encoder + * stream bytes. Knowing this would require changes to ls-qpack. For + * this reason, we don't perform this check and just flush it. + */ + if (stream->sm_bflags & SMBF_IETF) + headers_stream = stream->conn_pub->u.ietf.qeh->qeh_enc_sm_out; + else + headers_stream = + lsquic_headers_stream_get_stream(stream->conn_pub->u.gquic.hs); + if (headers_stream && lsquic_stream_has_data_to_flush(headers_stream)) { - LSQ_DEBUG("flushing headers stream before potential write to a " - "buffered packet"); + LSQ_DEBUG("flushing headers stream before packetizing stream data"); (void) lsquic_stream_flush(headers_stream); } - else - /* Some other stream must have flushed it: this means our headers - * are flushed. - */ - stream->stream_flags |= STREAM_HDRS_FLUSHED; + /* If there is nothing to flush, some other stream must have flushed it: + * this means our headers are flushed. Either way, only do this once. + */ + stream->stream_flags |= STREAM_HDRS_FLUSHED; } stream_header_sz = stream->sm_frame_header_sz(stream, size); - need_at_least = stream_header_sz + (size > 0); + need_at_least = stream_header_sz; + if ((stream->sm_bflags & (SMBF_IETF|SMBF_USE_HEADERS)) + == (SMBF_IETF|SMBF_USE_HEADERS)) + { + if (size > 0) + need_at_least += 3; /* Enough room for HTTP/3 frame */ + } + else + need_at_least += size > 0; get_packet: packet_out = lsquic_send_ctl_get_packet_for_stream(send_ctl, need_at_least, stream->conn_pub->path, stream); @@ -2826,13 +2879,14 @@ maybe_close_varsize_hq_frame (struct lsquic_stream *stream) if (shf->shf_flags & SHF_FIXED_SIZE) { - stream_hq_frame_put(stream, shf); + if (shf->shf_off + shf->shf_frame_size <= stream->sm_payload) + stream_hq_frame_put(stream, shf); return; } bits = (shf->shf_flags & SHF_TWO_BYTES) > 0; size = stream->sm_payload + stream->sm_n_buffered - shf->shf_off; - if (size && size <= VINT_MAX_B(bits)) + if (size && size <= VINT_MAX_B(bits) && shf->shf_frame_ptr) { if (0 == stream->sm_n_buffered) LSQ_DEBUG("close HQ frame type 0x%X of size %"PRIu64, @@ -2850,6 +2904,14 @@ maybe_close_varsize_hq_frame (struct lsquic_stream *stream) shf->shf_flags |= SHF_FIXED_SIZE; } } + else if (!shf->shf_frame_ptr) + { + LSQ_WARN("dangling HTTP/3 frame"); + stream->conn_pub->lconn->cn_if->ci_internal_error( + stream->conn_pub->lconn, "dangling HTTP/3 frame on stream %"PRIu64, + stream->id); + stream_hq_frame_put(stream, shf); + } else if (!size) { assert(!shf->shf_frame_ptr); diff --git a/test/test_common.c b/test/test_common.c index 27a56bb85..d9db175bb 100644 --- a/test/test_common.c +++ b/test/test_common.c @@ -1677,7 +1677,13 @@ set_engine_option (struct lsquic_engine_settings *settings, *version_cleared = 1; settings->es_versions = 0; } - const enum lsquic_version ver = lsquic_str2ver(val, strlen(val)); + enum lsquic_version ver = lsquic_str2ver(val, strlen(val)); + if (ver < N_LSQVER) + { + settings->es_versions |= 1 << ver; + return 0; + } + ver = lsquic_alpn2ver(val, strlen(val)); if (ver < N_LSQVER) { settings->es_versions |= 1 << ver; diff --git a/test/unittests/test_h3_framing.c b/test/unittests/test_h3_framing.c index 797bcee0a..795d14360 100644 --- a/test/unittests/test_h3_framing.c +++ b/test/unittests/test_h3_framing.c @@ -644,6 +644,118 @@ main_test_hq_framing (void) } +static void +test_frame_header_split (unsigned n_packets) +{ + struct test_objs tobjs; + struct lsquic_stream *stream; + size_t nw; + int fin, s; + unsigned char *buf_in, *buf_out; + const unsigned wsize = 70; + const size_t buf_in_sz = wsize, buf_out_sz = 0x500000; + + struct lsquic_http_header header = { + .name = { ":method", 7, }, + .value = { "GET", 3, }, + }; + struct lsquic_http_headers headers = { 1, &header, }; + + buf_in = malloc(buf_in_sz); + buf_out = malloc(buf_out_sz); + assert(buf_in && buf_out); + + struct packetization_test_stream_ctx packet_stream_ctx = + { + .buf = buf_in, + .off = 0, + .len = buf_in_sz, + .write_size = wsize, + .flush_after_each_write = 0, + }; + + init_buf(buf_in, buf_in_sz); + + init_test_ctl_settings(&g_ctl_settings); + g_ctl_settings.tcs_schedule_stream_packets_immediately = 1; + + stream_ctor_flags |= SCF_IETF; + init_test_objs(&tobjs, 0x1000, buf_out_sz, 1252); + tobjs.stream_if_ctx = &packet_stream_ctx; + tobjs.ctor_flags |= SCF_HTTP|SCF_IETF; + + g_ctl_settings.tcs_can_send = n_packets; + tobjs.stream_if = &packetization_inside_once_stream_if; + tobjs.ctor_flags |= SCF_DISP_RW_ONCE; + + struct lsquic_packet_out *const packet_out + = lsquic_send_ctl_new_packet_out(&tobjs.send_ctl, 0, PNS_APP, &network_path); + + const size_t pad_size = packet_out->po_n_alloc + - 2 /* STREAM header */ + - 5 /* 3-byte HEADERS frame */ + - 2; + packet_out->po_data_sz = pad_size; + lsquic_send_ctl_scheduled_one(&tobjs.send_ctl, packet_out); + + stream = new_stream(&tobjs, 0, buf_out_sz); + + s = lsquic_stream_send_headers(stream, &headers, 0); + assert(0 == s); + + const ssize_t w = lsquic_stream_write(stream, buf_in, buf_in_sz); + assert(w >= 0 && (size_t) w == buf_in_sz); + lsquic_stream_flush(stream); + + /* Verify written data: */ + nw = read_from_scheduled_packets(&tobjs.send_ctl, 0, buf_out, buf_out_sz, + 0, &fin, 1); + { /* Remove framing and verify contents */ + const unsigned char *src; + unsigned char *dst; + uint64_t sz; + unsigned frame_type; + int s; + + src = buf_out; + dst = buf_out; + while (src < buf_out + nw) + { + frame_type = *src++; + s = vint_read(src, buf_out + buf_out_sz, &sz); + assert(s > 0); + assert(sz > 0); + assert(sz < (1 << 14)); + src += s; + if (src == buf_out + s + 1) + { + /* Ignore headers */ + assert(frame_type == HQFT_HEADERS); + src += sz; + } + else + { + assert(frame_type == HQFT_DATA); + if (src + sz > buf_out + nw) /* Chopped DATA frame (last) */ + sz = buf_out + nw - src; + memmove(dst, src, sz); + dst += sz; + src += sz; + } + } + assert(0 == memcmp(buf_in, buf_out, (uintptr_t) dst - (uintptr_t) buf_out)); + } + + lsquic_stream_destroy(stream); + deinit_test_objs(&tobjs); + free(buf_in); + free(buf_out); + + stream_ctor_flags &= ~SCF_IETF; +} + + + int main (int argc, char **argv) { @@ -667,6 +779,8 @@ main (int argc, char **argv) init_test_ctl_settings(&g_ctl_settings); main_test_hq_framing(); + test_frame_header_split(1); + test_frame_header_split(2); return 0; }