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.
litespeedtech · Nov 18, 2020 · 4580fab · 4580fab
1 parent 2ed0721
commit 4580fab
Show file tree

Hide file tree

Showing 12 changed files with 108 additions and 55 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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]