From 4580fab74733f49913873817c1ef06d60096566d Mon Sep 17 00:00:00 2001 From: Dmitri Tikhonov Date: Wed, 18 Nov 2020 09:05:15 -0500 Subject: [PATCH] Release 2.24.4 - [BUGFIX] Check whether ECN counts are set in ACK struct before using them. - [BUGFIX] Calculate TLP timer correctly when only one packet is in flight. - [BUGFIX] Min RTO delay is 200 milliseconds, not 1 second. - [BUGFIX] Memory leak in QPACK decoder handler: discard hset when necessary. - Allow retired and drained CIDs to be reused after a timeout. --- CHANGELOG | 11 ++++++ docs/conf.py | 2 +- include/lsquic.h | 2 +- src/liblsquic/lsquic_conn_public.h | 1 + src/liblsquic/lsquic_engine.c | 50 ++++++++++++++++----------- src/liblsquic/lsquic_engine_public.h | 3 +- src/liblsquic/lsquic_full_conn.c | 3 +- src/liblsquic/lsquic_full_conn_ietf.c | 48 +++++++++++++++---------- src/liblsquic/lsquic_purga.h | 4 +-- src/liblsquic/lsquic_qdec_hdl.c | 31 +++++++++++++---- src/liblsquic/lsquic_send_ctl.c | 6 ++-- src/liblsquic/lsquic_stream.c | 2 -- 12 files changed, 108 insertions(+), 55 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index d985ce5db..b0dabcf72 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,14 @@ +2020-11-18 + - 2.24.4 + - [BUGFIX] Check whether ECN counts are set in ACK struct before using + them. + - [BUGFIX] Calculate TLP timer correctly when only one packet is in + flight. + - [BUGFIX] Min RTO delay is 200 milliseconds, not 1 second. + - [BUGFIX] Memory leak in QPACK decoder handler: discard hset when + necessary. + - Allow retired and drained CIDs to be reused after a timeout. + 2020-11-11 - 2.24.3 - [BUGFIX] Get rough RTT estimate on receipt of Handshake packet. diff --git a/docs/conf.py b/docs/conf.py index 8df0ab0e5..214a7fdfe 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -26,7 +26,7 @@ # The short X.Y version version = u'2.24' # The full version, including alpha/beta/rc tags -release = u'2.24.3' +release = u'2.24.4' # -- General configuration --------------------------------------------------- diff --git a/include/lsquic.h b/include/lsquic.h index de6e1c166..8fb4b2d56 100644 --- a/include/lsquic.h +++ b/include/lsquic.h @@ -25,7 +25,7 @@ extern "C" { #define LSQUIC_MAJOR_VERSION 2 #define LSQUIC_MINOR_VERSION 24 -#define LSQUIC_PATCH_VERSION 3 +#define LSQUIC_PATCH_VERSION 4 /** * Engine flags: diff --git a/src/liblsquic/lsquic_conn_public.h b/src/liblsquic/lsquic_conn_public.h index d2e660df2..6302b75e5 100644 --- a/src/liblsquic/lsquic_conn_public.h +++ b/src/liblsquic/lsquic_conn_public.h @@ -66,6 +66,7 @@ struct lsquic_conn_public { unsigned bytes_out; /* Used for no-progress timeout */ lsquic_time_t last_tick, last_prog; + unsigned max_peer_ack_usec; }; #endif diff --git a/src/liblsquic/lsquic_engine.c b/src/liblsquic/lsquic_engine.c index 751b14305..0b50a4db8 100644 --- a/src/liblsquic/lsquic_engine.c +++ b/src/liblsquic/lsquic_engine.c @@ -1225,6 +1225,13 @@ find_conn (lsquic_engine_t *engine, lsquic_packet_in_t *packet_in, } +static const char *const puty2str[] = { + [PUTY_CONN_DELETED] = "deleted", + [PUTY_CONN_DRAIN] = "being drained", + [PUTY_CID_RETIRED] = "retired", +}; + + static lsquic_conn_t * find_or_create_conn (lsquic_engine_t *engine, lsquic_packet_in_t *packet_in, struct packin_parse_state *ppstate, const struct sockaddr *sa_local, @@ -1270,34 +1277,33 @@ find_or_create_conn (lsquic_engine_t *engine, lsquic_packet_in_t *packet_in, switch (puel->puel_type) { case PUTY_CID_RETIRED: - LSQ_DEBUGC("CID %"CID_FMT" was retired, ignore packet", - CID_BITS(&packet_in->pi_conn_id)); - return NULL; case PUTY_CONN_DRAIN: - LSQ_DEBUG("drain till: %"PRIu64"; now: %"PRIu64, - puel->puel_time, packet_in->pi_received); if (puel->puel_time > packet_in->pi_received) { - LSQ_DEBUGC("CID %"CID_FMT" is in drain state, ignore packet", - CID_BITS(&packet_in->pi_conn_id)); + LSQ_DEBUGC("CID %"CID_FMT" is %s for another %"PRIu64 + "usec, ignore packet", CID_BITS(&packet_in->pi_conn_id), + puty2str[puel->puel_type], + puel->puel_time - packet_in->pi_received); return NULL; } - LSQ_DEBUGC("CID %"CID_FMT" goes from drain state to deleted", - CID_BITS(&packet_in->pi_conn_id)); - puel->puel_type = PUTY_CONN_DELETED; - puel->puel_count = 0; - puel->puel_time = 0; - /* fall-through */ + LSQ_DEBUGC("CID %"CID_FMT" is no longer %s", + CID_BITS(&packet_in->pi_conn_id), puty2str[puel->puel_type]); + break; case PUTY_CONN_DELETED: LSQ_DEBUGC("Connection with CID %"CID_FMT" was deleted", CID_BITS(&packet_in->pi_conn_id)); if (puel->puel_time < packet_in->pi_received) { - puel->puel_time = packet_in->pi_received - /* Exponential back-off */ - + 1000000ull * (1 << MIN(puel->puel_count, 4)); - ++puel->puel_count; - goto maybe_send_prst; + if (puel->puel_count < 4) + { + puel->puel_time = packet_in->pi_received + /* Exponential back-off */ + + 1000000ull * (1 << MIN(puel->puel_count, 4)); + ++puel->puel_count; + goto maybe_send_prst; + } + else + break; } return NULL; default: @@ -3295,10 +3301,12 @@ lsquic_engine_add_cid (struct lsquic_engine_public *enpub, void lsquic_engine_retire_cid (struct lsquic_engine_public *enpub, - struct lsquic_conn *conn, unsigned cce_idx, lsquic_time_t now) + struct lsquic_conn *conn, unsigned cce_idx, lsquic_time_t now, + lsquic_time_t drain_time) { struct lsquic_engine *const engine = (struct lsquic_engine *) enpub; struct conn_cid_elem *const cce = &conn->cn_cces[cce_idx]; + struct purga_el *puel; void *peer_ctx; assert(cce_idx < conn->cn_n_cces); @@ -3309,8 +3317,10 @@ lsquic_engine_retire_cid (struct lsquic_engine_public *enpub, if (engine->purga) { peer_ctx = lsquic_conn_get_peer_ctx(conn, NULL); - lsquic_purga_add(engine->purga, &cce->cce_cid, peer_ctx, + puel = lsquic_purga_add(engine->purga, &cce->cce_cid, peer_ctx, PUTY_CID_RETIRED, now); + if (puel) + puel->puel_time = now + drain_time; } conn->cn_cces_mask &= ~(1u << cce_idx); LSQ_DEBUGC("retire CID %"CID_FMT, CID_BITS(&cce->cce_cid)); diff --git a/src/liblsquic/lsquic_engine_public.h b/src/liblsquic/lsquic_engine_public.h index 30fdfff29..179ca1b1a 100644 --- a/src/liblsquic/lsquic_engine_public.h +++ b/src/liblsquic/lsquic_engine_public.h @@ -107,7 +107,8 @@ lsquic_engine_add_conn_to_attq (struct lsquic_engine_public *enpub, void lsquic_engine_retire_cid (struct lsquic_engine_public *, - struct lsquic_conn *, unsigned cce_idx, lsquic_time_t now); + struct lsquic_conn *, unsigned cce_idx, lsquic_time_t now, + lsquic_time_t drain_time); int lsquic_engine_add_cid (struct lsquic_engine_public *, diff --git a/src/liblsquic/lsquic_full_conn.c b/src/liblsquic/lsquic_full_conn.c index bcb6790cb..948ca2367 100644 --- a/src/liblsquic/lsquic_full_conn.c +++ b/src/liblsquic/lsquic_full_conn.c @@ -619,6 +619,7 @@ new_conn_common (lsquic_cid_t cid, struct lsquic_engine_public *enpub, conn->fc_pub.packet_out_malo = lsquic_malo_create(sizeof(struct lsquic_packet_out)); conn->fc_pub.path = &conn->fc_path; + conn->fc_pub.max_peer_ack_usec = ACK_TIMEOUT; conn->fc_stream_ifs[STREAM_IF_STD].stream_if = enpub->enp_stream_if; conn->fc_stream_ifs[STREAM_IF_STD].stream_if_ctx = enpub->enp_stream_if_ctx; conn->fc_settings = &enpub->enp_settings; @@ -4391,7 +4392,7 @@ lsquic_gquic_full_conn_srej (struct lsquic_conn *lconn) if (cce->cce_hash_el.qhe_flags & QHE_HASHED) { lsquic_engine_retire_cid(conn->fc_enpub, lconn, cce_idx, - 0 /* OK to omit the `now' value */); + 0 /* OK to omit the `now' value */, 0); lconn->cn_cces_mask |= 1 << cce_idx; lsquic_generate_cid_gquic(&cce->cce_cid); if (0 != lsquic_engine_add_cid(conn->fc_enpub, lconn, cce_idx)) diff --git a/src/liblsquic/lsquic_full_conn_ietf.c b/src/liblsquic/lsquic_full_conn_ietf.c index 6f9f48ca3..f4eddc65e 100644 --- a/src/liblsquic/lsquic_full_conn_ietf.c +++ b/src/liblsquic/lsquic_full_conn_ietf.c @@ -480,7 +480,6 @@ struct ietf_full_conn unsigned ifc_ack_freq_seqno; unsigned ifc_last_pack_tol; unsigned ifc_max_ack_freq_seqno; /* Incoming */ - unsigned ifc_max_peer_ack_usec; unsigned short ifc_max_udp_payload; /* Cached TP */ lsquic_time_t ifc_last_live_update; struct conn_path ifc_paths[N_PATHS]; @@ -3118,11 +3117,28 @@ ietf_full_conn_ci_destroy (struct lsquic_conn *lconn) } +static uint64_t +calc_drain_time (const struct ietf_full_conn *conn) +{ + lsquic_time_t drain_time, pto, srtt, var; + + /* PTO Calculation: [draft-ietf-quic-recovery-18], Section 6.2.2.1; + * Drain time: [draft-ietf-quic-transport-19], Section 10.1. + */ + srtt = lsquic_rtt_stats_get_srtt(&conn->ifc_pub.rtt_stats); + var = lsquic_rtt_stats_get_rttvar(&conn->ifc_pub.rtt_stats); + pto = srtt + 4 * var + TP_DEF_MAX_ACK_DELAY * 1000; + drain_time = 3 * pto; + + return drain_time; +} + + static lsquic_time_t ietf_full_conn_ci_drain_time (const struct lsquic_conn *lconn) { struct ietf_full_conn *conn = (struct ietf_full_conn *) lconn; - lsquic_time_t drain_time, pto, srtt, var; + lsquic_time_t drain_time; /* Only applicable to a server whose connection was not timed out */ if ((conn->ifc_flags & (IFC_SERVER|IFC_TIMED_OUT)) != IFC_SERVER) @@ -3131,14 +3147,7 @@ ietf_full_conn_ci_drain_time (const struct lsquic_conn *lconn) return 0; } - /* PTO Calculation: [draft-ietf-quic-recovery-18], Section 6.2.2.1; - * Drain time: [draft-ietf-quic-transport-19], Section 10.1. - */ - srtt = lsquic_rtt_stats_get_srtt(&conn->ifc_pub.rtt_stats); - var = lsquic_rtt_stats_get_rttvar(&conn->ifc_pub.rtt_stats); - pto = srtt + 4 * var + TP_DEF_MAX_ACK_DELAY * 1000; - drain_time = 3 * pto; - + drain_time = calc_drain_time(conn); LSQ_DEBUG("drain time is %"PRIu64" usec", drain_time); return drain_time; } @@ -3510,7 +3519,7 @@ apply_trans_params (struct ietf_full_conn *conn, ? USHRT_MAX : params->tp_numerics[TPI_MAX_DATAGRAM_FRAME_SIZE]; } - conn->ifc_max_peer_ack_usec = params->tp_max_ack_delay * 1000; + conn->ifc_pub.max_peer_ack_usec = params->tp_max_ack_delay * 1000; if ((params->tp_set & (1 << TPI_MAX_UDP_PAYLOAD_SIZE)) /* Second check is so that we don't truncate a large value when @@ -4437,7 +4446,7 @@ generate_ack_frequency_frame (struct ietf_full_conn *conn, lsquic_time_t unused) need = conn->ifc_conn.cn_pf->pf_ack_frequency_frame_size( conn->ifc_ack_freq_seqno, conn->ifc_last_pack_tol, - conn->ifc_max_peer_ack_usec); + conn->ifc_pub.max_peer_ack_usec); packet_out = get_writeable_packet(conn, need); if (!packet_out) { @@ -4450,7 +4459,7 @@ generate_ack_frequency_frame (struct ietf_full_conn *conn, lsquic_time_t unused) packet_out->po_data + packet_out->po_data_sz, lsquic_packet_out_avail(packet_out), conn->ifc_ack_freq_seqno, conn->ifc_last_pack_tol, - conn->ifc_max_peer_ack_usec, ignore); + conn->ifc_pub.max_peer_ack_usec, ignore); if (sz < 0) { ABORT_ERROR("gen_ack_frequency_frame failed"); @@ -4466,7 +4475,7 @@ generate_ack_frequency_frame (struct ietf_full_conn *conn, lsquic_time_t unused) packet_out->po_frame_types |= QUIC_FTBIT_ACK_FREQUENCY; LSQ_DEBUG("Generated ACK_FREQUENCY(seqno: %u; pack_tol: %u; " "upd: %u; ignore: %d)", conn->ifc_ack_freq_seqno, - conn->ifc_last_pack_tol, conn->ifc_max_peer_ack_usec, ignore); + conn->ifc_last_pack_tol, conn->ifc_pub.max_peer_ack_usec, ignore); ++conn->ifc_ack_freq_seqno; conn->ifc_send_flags &= ~SF_SEND_ACK_FREQUENCY; } @@ -6105,14 +6114,17 @@ retire_cid (struct ietf_full_conn *conn, struct conn_cid_elem *cce, lsquic_time_t now) { struct lsquic_conn *const lconn = &conn->ifc_conn; + lsquic_time_t drain_time; - LSQ_DEBUGC("retiring CID %"CID_FMT"; seqno: %u; %s", - CID_BITS(&cce->cce_cid), cce->cce_seqno, - (cce->cce_flags & CCE_SEQNO) ? "" : "original"); + drain_time = calc_drain_time(conn); + LSQ_DEBUGC("retiring CID %"CID_FMT"; seqno: %u; %s; drain time %"PRIu64 + " usec", CID_BITS(&cce->cce_cid), cce->cce_seqno, + (cce->cce_flags & CCE_SEQNO) ? "" : "original", drain_time); if (cce->cce_flags & CCE_SEQNO) --conn->ifc_active_cids_count; - lsquic_engine_retire_cid(conn->ifc_enpub, lconn, cce - lconn->cn_cces, now); + lsquic_engine_retire_cid(conn->ifc_enpub, lconn, cce - lconn->cn_cces, now, + drain_time); memset(cce, 0, sizeof(*cce)); if (can_issue_cids(conn) diff --git a/src/liblsquic/lsquic_purga.h b/src/liblsquic/lsquic_purga.h index 896f68830..7a91665d8 100644 --- a/src/liblsquic/lsquic_purga.h +++ b/src/liblsquic/lsquic_purga.h @@ -26,8 +26,8 @@ enum purga_type struct purga_el { enum purga_type puel_type; - /* When puel_type is PUTY_CONN_DRAIN, puel_time specifies the time until - * the end of the drain period. + /* When puel_type is PUTY_CONN_DRAIN or PUTY_CID_RETIRED, puel_time + * specifies the time until the end of the drain period. * * When puel_type is PUTY_CONN_DELETED, puel_time specifies the time * until the next time stateless reset can be sent. puel_count is the diff --git a/src/liblsquic/lsquic_qdec_hdl.c b/src/liblsquic/lsquic_qdec_hdl.c index c173ec459..a2fedebb3 100644 --- a/src/liblsquic/lsquic_qdec_hdl.c +++ b/src/liblsquic/lsquic_qdec_hdl.c @@ -57,8 +57,8 @@ struct header_ctx */ union hblock_ctx { - struct header_ctx ctx; - struct uncompressed_headers uh; + struct header_ctx ctx; + unsigned char space_for_uh[sizeof(struct uncompressed_headers)]; }; @@ -596,6 +596,21 @@ static const struct lsqpack_dec_hset_if dhi_if = }; +static void +qdh_maybe_destroy_hblock_ctx (struct qpack_dec_hdl *qdh, + struct lsquic_stream *stream) +{ + if (stream->sm_hblock_ctx) + { + LSQ_DEBUG("destroy hblock_ctx of stream %"PRIu64, stream->id); + qdh->qdh_enpub->enp_hsi_if->hsi_discard_header_set( + stream->sm_hblock_ctx->ctx.hset); + free(stream->sm_hblock_ctx); + stream->sm_hblock_ctx = NULL; + } +} + + static enum lsqpack_read_header_status qdh_header_read_results (struct qpack_dec_hdl *qdh, struct lsquic_stream *stream, enum lsqpack_read_header_status rhs, @@ -619,7 +634,7 @@ qdh_header_read_results (struct qpack_dec_hdl *qdh, &stream->sm_hblock_ctx->ctx.ehp); } hset = stream->sm_hblock_ctx->ctx.hset; - uh = &stream->sm_hblock_ctx->uh; + uh = (void *) stream->sm_hblock_ctx; stream->sm_hblock_ctx = NULL; memset(uh, 0, sizeof(*uh)); uh->uh_stream_id = stream->id; @@ -631,8 +646,9 @@ qdh_header_read_results (struct qpack_dec_hdl *qdh, if (0 != qdh->qdh_enpub->enp_hsi_if ->hsi_process_header(hset, NULL)) { - LSQ_DEBUG("finishing HTTP/1.x hset failed"); + LSQ_DEBUG("finishing hset failed"); free(uh); + qdh->qdh_enpub->enp_hsi_if->hsi_discard_header_set(hset); return LQRHS_ERROR; } uh->uh_hset = hset; @@ -642,14 +658,14 @@ qdh_header_read_results (struct qpack_dec_hdl *qdh, { LSQ_DEBUG("could not give hset to stream %"PRIu64, stream->id); free(uh); + qdh->qdh_enpub->enp_hsi_if->hsi_discard_header_set(hset); return LQRHS_ERROR; } } else { LSQ_DEBUG("discard trailer header set"); - free(stream->sm_hblock_ctx); - stream->sm_hblock_ctx = NULL; + qdh_maybe_destroy_hblock_ctx(qdh, stream); } if (qdh->qdh_dec_sm_out) { @@ -664,6 +680,7 @@ qdh_header_read_results (struct qpack_dec_hdl *qdh, } else if (rhs == LQRHS_ERROR) { + qdh_maybe_destroy_hblock_ctx(qdh, stream); qerr = lsqpack_dec_get_err_info(&qdh->qdh_decoder); qdh->qdh_conn->cn_if->ci_abort_error(qdh->qdh_conn, 1, HEC_QPACK_DECOMPRESSION_FAILED, "QPACK decompression error; " @@ -787,6 +804,8 @@ lsquic_qdh_cancel_stream (struct qpack_dec_hdl *qdh, ssize_t nw; unsigned char buf[LSQPACK_LONGEST_CANCEL]; + qdh_maybe_destroy_hblock_ctx(qdh, stream); + if (!qdh->qdh_dec_sm_out) return; diff --git a/src/liblsquic/lsquic_send_ctl.c b/src/liblsquic/lsquic_send_ctl.c index f33098a05..c75e211cc 100644 --- a/src/liblsquic/lsquic_send_ctl.c +++ b/src/liblsquic/lsquic_send_ctl.c @@ -67,7 +67,7 @@ #define MAX_RTO_BACKOFFS 10 #define DEFAULT_RETX_DELAY 500000 /* Microseconds */ #define MAX_RTO_DELAY 60000000 /* Microseconds */ -#define MIN_RTO_DELAY 1000000 /* Microseconds */ +#define MIN_RTO_DELAY 200000 /* Microseconds */ #define N_NACKS_BEFORE_RETX 3 #define CGP(ctl) ((struct cong_ctl *) (ctl)->sc_cong_ctl) @@ -443,7 +443,7 @@ calculate_tlp_delay (lsquic_send_ctl_t *ctl) } else { - delay = srtt + srtt / 2 + MIN_RTO_DELAY; + delay = srtt + srtt / 2 + ctl->sc_conn_pub->max_peer_ack_usec; if (delay < 2 * srtt) delay = 2 * srtt; } @@ -1276,7 +1276,7 @@ lsquic_send_ctl_got_ack (lsquic_send_ctl_t *ctl, if (one_rtt_cnt) ctl->sc_flags |= SC_1RTT_ACKED; - if (lsquic_send_ctl_ecn_turned_on(ctl)) + if (lsquic_send_ctl_ecn_turned_on(ctl) && (acki->flags & AI_ECN)) { const uint64_t sum = acki->ecn_counts[ECN_ECT0] + acki->ecn_counts[ECN_ECT1] diff --git a/src/liblsquic/lsquic_stream.c b/src/liblsquic/lsquic_stream.c index ba68762ac..b5638f53d 100644 --- a/src/liblsquic/lsquic_stream.c +++ b/src/liblsquic/lsquic_stream.c @@ -701,8 +701,6 @@ lsquic_stream_destroy (lsquic_stream_t *stream) destroy_uh(stream); free(stream->sm_buf); free(stream->sm_header_block); - if (stream->sm_hblock_ctx) - free(stream->sm_hblock_ctx); LSQ_DEBUG("destroyed stream"); SM_HISTORY_DUMP_REMAINING(stream); free(stream);