Release 2.27.3

- [BUGFIX] gQUIC: do not destroy critical streams when connection is closed. See issue #201. - [BUGFIX] Drop #if LSQUIC_CONN_STATS from lsquic.h. See issue #211. - [BUGFIX] Challenge cancellation when path validation fails. - [BUGFIX] Do not send FIN if RST is scheduled to be sent on a stream. - [BUGFIX] gQUIC's is_tickable() when connection is closing. - [BUGFIX] Q050 processing of GOAWAY frames.
litespeedtech · Jan 8, 2021 · e2c4907 · e2c4907
1 parent 1a0003e
commit e2c4907
Show file tree

Hide file tree

Showing 10 changed files with 85 additions and 54 deletions.
diff --git a/CHANGELOG b/CHANGELOG
@@ -1,3 +1,13 @@
+2021-01-08
+    - 2.27.3
+    - [BUGFIX] gQUIC: do not destroy critical streams when connection is
+      closed.  See issue #201.
+    - [BUGFIX] Drop #if LSQUIC_CONN_STATS from lsquic.h.  See issue #211.
+    - [BUGFIX] Challenge cancellation when path validation fails.
+    - [BUGFIX] Do not send FIN if RST is scheduled to be sent on a stream.
+    - [BUGFIX] gQUIC's is_tickable() when connection is closing.
+    - [BUGFIX] Q050 processing of GOAWAY frames.
+
 2021-01-06
     - 2.27.2
     - [BUGFIX] Memory corruption in receive history copy-ranges function.

diff --git a/docs/conf.py b/docs/conf.py
@@ -26,7 +26,7 @@
 # The short X.Y version
 version = u'2.27'
 # The full version, including alpha/beta/rc tags
-release = u'2.27.2'
+release = u'2.27.3'
 
 
 # -- 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 27
-#define LSQUIC_PATCH_VERSION 2
+#define LSQUIC_PATCH_VERSION 3
 
 /**
  * Engine flags:
@@ -1320,13 +1320,13 @@ struct lsquic_engine_api
      */
     const struct lsquic_hset_if         *ea_hsi_if;
     void                                *ea_hsi_ctx;
-#if LSQUIC_CONN_STATS
+
     /**
      * If set, engine will print cumulative connection statistics to this
-     * file just before it is destroyed.
+     * file just before it is destroyed.  (Must be compiled with
+     * -DLSQUIC_CONN_STATS=1).
      */
     void /* FILE, really */             *ea_stats_fh;
-#endif
 
     /**
      * The optional ALPN string is used by the client if @ref LSENG_HTTP

diff --git a/src/liblsquic/lsquic_full_conn.c b/src/liblsquic/lsquic_full_conn.c
@@ -84,6 +84,9 @@ enum stream_if { STREAM_IF_STD, STREAM_IF_HSK, STREAM_IF_HDR, N_STREAM_IFS };
 /* Maximum number of ACK ranges that can fit into gQUIC ACK frame */
 #define MAX_ACK_RANGES 256
 
+/* HANDSHAKE and HEADERS streams are always open in gQUIC connection */
+#define N_SPECIAL_STREAMS 2
+
 /* IMPORTANT: Keep values of FC_SERVER and FC_HTTP same as LSENG_SERVER
  * and LSENG_HTTP.
  */
@@ -1828,7 +1831,7 @@ reset_local_streams_over_goaway (struct full_conn *conn)
                                  el = lsquic_hash_next(conn->fc_pub.all_streams))
     {
         stream = lsquic_hashelem_getdata(el);
-        if (stream->id > conn->fc_goaway_stream_id &&
+        if ((int64_t) stream->id > (int64_t) conn->fc_goaway_stream_id &&
             ((stream->id & 1) ^ is_server /* Locally initiated? */))
         {
             lsquic_stream_received_goaway(stream);
@@ -2083,8 +2086,6 @@ static unsigned
 process_connection_close_frame (struct full_conn *conn, lsquic_packet_in_t *packet_in,
                                 const unsigned char *p, size_t len)
 {
-    lsquic_stream_t *stream;
-    struct lsquic_hash_elem *el;
     uint64_t error_code;
     uint16_t reason_len;
     uint8_t reason_off;
@@ -2101,17 +2102,7 @@ process_connection_close_frame (struct full_conn *conn, lsquic_packet_in_t *pack
     if (conn->fc_stream_ifs[STREAM_IF_STD].stream_if->on_conncloseframe_received)
         conn->fc_stream_ifs[STREAM_IF_STD].stream_if->on_conncloseframe_received(
             &conn->fc_conn, -1, error_code, (const char *) p + reason_off, reason_len);
-    conn->fc_flags |= FC_RECV_CLOSE;
-    if (!(conn->fc_flags & FC_CLOSING))
-    {
-        for (el = lsquic_hash_first(conn->fc_pub.all_streams); el;
-                                     el = lsquic_hash_next(conn->fc_pub.all_streams))
-        {
-            stream = lsquic_hashelem_getdata(el);
-            lsquic_stream_shutdown_internal(stream);
-        }
-        conn->fc_flags |= FC_CLOSING;
-    }
+    conn->fc_flags |= FC_RECV_CLOSE|FC_CLOSING;
     return parsed_len;
 }
 
@@ -2411,7 +2402,7 @@ process_regular_packet (struct full_conn *conn, lsquic_packet_in_t *packet_in)
         {
             frame_types = packet_in->pi_frame_types;
             if ((conn->fc_flags & FC_GOING_AWAY)
-                && lsquic_hash_count(conn->fc_pub.all_streams) < 3)
+                && lsquic_hash_count(conn->fc_pub.all_streams) <= N_SPECIAL_STREAMS)
             {
                 /* Ignore PING frames if we are going away and there are no
                  * active streams.  (HANDSHAKE and HEADERS streams are the
@@ -2627,7 +2618,7 @@ maybe_close_conn (struct full_conn *conn)
 
     if ((conn->fc_flags & (FC_CLOSING|FC_GOAWAY_SENT|FC_SERVER))
                                             == (FC_GOAWAY_SENT|FC_SERVER)
-        && lsquic_hash_count(conn->fc_pub.all_streams) == 2)
+        && lsquic_hash_count(conn->fc_pub.all_streams) == N_SPECIAL_STREAMS)
     {
 #ifndef NDEBUG
         for (el = lsquic_hash_first(conn->fc_pub.all_streams); el;
@@ -3193,7 +3184,7 @@ conn_ok_to_close (const struct full_conn *conn)
         || (conn->fc_flags & FC_RECV_CLOSE)
         || (
                !lsquic_send_ctl_have_outgoing_stream_frames(&conn->fc_send_ctl)
-            && lsquic_hash_count(conn->fc_pub.all_streams) == 0
+            && lsquic_hash_count(conn->fc_pub.all_streams) <= N_SPECIAL_STREAMS
             && lsquic_send_ctl_have_unacked_stream_frames(&conn->fc_send_ctl) == 0);
 }
 
@@ -3807,7 +3798,8 @@ full_conn_ci_close (struct lsquic_conn *lconn)
                                      el = lsquic_hash_next(conn->fc_pub.all_streams))
         {
             stream = lsquic_hashelem_getdata(el);
-            lsquic_stream_shutdown_internal(stream);
+            if (!lsquic_stream_is_critical(stream))
+                lsquic_stream_reset(stream, 0);
         }
         conn->fc_flags |= FC_CLOSING;
         if (!(conn->fc_flags & FC_GOAWAY_SENT))
@@ -4264,7 +4256,7 @@ full_conn_ci_is_tickable (lsquic_conn_t *lconn)
             !lsquic_send_ctl_sched_is_blocked(&conn->fc_send_ctl)))
     {
         const enum full_conn_flags send_flags = FC_SEND_GOAWAY
-                |FC_SEND_STOP_WAITING|FC_SEND_PING|FC_SEND_WUF|FC_CLOSING;
+                |FC_SEND_STOP_WAITING|FC_SEND_PING|FC_SEND_WUF;
         if (conn->fc_flags & send_flags)
         {
             LSQ_DEBUG("tickable: flags: 0x%X", conn->fc_flags & send_flags);

diff --git a/src/liblsquic/lsquic_full_conn_ietf.c b/src/liblsquic/lsquic_full_conn_ietf.c
@@ -705,8 +705,10 @@ cid_throt_alarm_expired (enum alarm_id al_id, void *ctx,
 static void
 wipe_path (struct ietf_full_conn *conn, unsigned path_id)
 {
+    void *peer_ctx = conn->ifc_paths[path_id].cop_path.np_peer_ctx;
     memset(&conn->ifc_paths[path_id], 0, sizeof(conn->ifc_paths[0]));
     conn->ifc_paths[path_id].cop_path.np_path_id = path_id;
+    conn->ifc_paths[path_id].cop_path.np_peer_ctx = peer_ctx;
 }
 
 
@@ -2916,7 +2918,7 @@ ietf_full_conn_ci_close (struct lsquic_conn *lconn)
             stream = lsquic_hashelem_getdata(el);
             sd = (stream->id >> SD_SHIFT) & 1;
             if (SD_BIDI == sd)
-                lsquic_stream_shutdown_internal(stream);
+                lsquic_stream_reset(stream, 0);
         }
         conn->ifc_flags |= IFC_CLOSING;
         conn->ifc_send_flags |= SF_SEND_CONN_CLOSE;
@@ -5933,8 +5935,6 @@ static unsigned
 process_connection_close_frame (struct ietf_full_conn *conn,
         struct lsquic_packet_in *packet_in, const unsigned char *p, size_t len)
 {
-    lsquic_stream_t *stream;
-    struct lsquic_hash_elem *el;
     uint64_t error_code;
     uint16_t reason_len;
     uint8_t reason_off;
@@ -5971,17 +5971,7 @@ process_connection_close_frame (struct ietf_full_conn *conn,
     if (conn->ifc_enpub->enp_stream_if->on_conncloseframe_received)
         conn->ifc_enpub->enp_stream_if->on_conncloseframe_received(
             &conn->ifc_conn, app_error, error_code, (const char *) p + reason_off, reason_len);
-    conn->ifc_flags |= IFC_RECV_CLOSE;
-    if (!(conn->ifc_flags & IFC_CLOSING))
-    {
-        for (el = lsquic_hash_first(conn->ifc_pub.all_streams); el;
-                             el = lsquic_hash_next(conn->ifc_pub.all_streams))
-        {
-            stream = lsquic_hashelem_getdata(el);
-            lsquic_stream_shutdown_internal(stream);
-        }
-        conn->ifc_flags |= IFC_CLOSING;
-    }
+    conn->ifc_flags |= IFC_RECV_CLOSE|IFC_CLOSING;
     return parsed_len;
 }
 
@@ -8890,6 +8880,7 @@ cancel_push_promise (struct ietf_full_conn *conn, struct push_promise *promise)
     /* But let stream dtor free the promise object as sm_promise may yet
      * be used by the stream in some ways.
      */
+    /* TODO: drop lsquic_stream_shutdown_internal, use something else */
     lsquic_stream_shutdown_internal(promise->pp_pushed_stream);
     if (0 != lsquic_hcso_write_cancel_push(&conn->ifc_hcso, promise->pp_id))
         ABORT_WARN("cannot write CANCEL_PUSH");

diff --git a/src/liblsquic/lsquic_send_ctl.c b/src/liblsquic/lsquic_send_ctl.c
@@ -3617,9 +3617,10 @@ lsquic_send_ctl_cancel_chals (struct lsquic_send_ctl *ctl,
                                                             packet_out = next)
     {
         next = TAILQ_NEXT(packet_out, po_next);
-        if (packet_out->po_path == path
-                && packet_out->po_frame_types == QUIC_FTBIT_PATH_CHALLENGE)
+        if (packet_out->po_path == path)
         {
+            assert(packet_out->po_frame_types & QUIC_FTBIT_PATH_CHALLENGE);
+            assert(!(packet_out->po_frame_types & ctl->sc_retx_frames));
             send_ctl_maybe_renumber_sched_to_right(ctl, packet_out);
             send_ctl_sched_remove(ctl, packet_out);
             assert(packet_out->po_loss_chain == packet_out);

diff --git a/src/liblsquic/lsquic_stream.c b/src/liblsquic/lsquic_stream.c
@@ -1837,7 +1837,8 @@ stream_shutdown_write (lsquic_stream_t *stream)
      * the flags indicate that nothing else should be written.
      */
     if (!(stream->sm_bflags & SMBF_CRYPTO)
-            && !(stream->stream_flags & (STREAM_FIN_SENT|STREAM_RST_SENT))
+            && !((stream->stream_flags & (STREAM_FIN_SENT|STREAM_RST_SENT))
+                    || (stream->sm_qflags & SMQF_SEND_RST))
                 && !stream_is_incoming_unidir(stream)
                         /* In gQUIC, receiving a RESET means "stop sending" */
                     && !(!(stream->sm_bflags & SMBF_IETF)

diff --git a/tests/test_h3_framing.c b/tests/test_h3_framing.c
@@ -331,7 +331,9 @@ static const struct conn_iface our_conn_if =
 };
 
 
+#if LSQUIC_CONN_STATS
 static struct conn_stats s_conn_stats;
+#endif
 
 static void
 init_test_objs (struct test_objs *tobjs, unsigned initial_conn_window,
@@ -361,7 +363,9 @@ init_test_objs (struct test_objs *tobjs, unsigned initial_conn_window,
     tobjs->conn_pub.packet_out_malo =
                         lsquic_malo_create(sizeof(struct lsquic_packet_out));
     tobjs->conn_pub.path = &network_path;
+#if LSQUIC_CONN_STATS
     tobjs->conn_pub.conn_stats = &s_conn_stats;
+#endif
     tobjs->initial_stream_window = initial_stream_window;
     tobjs->eng_pub.enp_settings.es_cc_algo = 1;  /* Cubic */
     tobjs->eng_pub.enp_hsi_if = &tobjs->hsi_if;

diff --git a/tests/test_send_headers.c b/tests/test_send_headers.c
@@ -174,7 +174,9 @@ static const struct conn_iface our_conn_if =
 
 static struct http1x_ctor_ctx ctor_ctx = { .is_server = 0, };
 
+#if LSQUIC_CONN_STATS
 static struct conn_stats s_conn_stats;
+#endif
 
 static void
 init_test_objs (struct test_objs *tobjs, unsigned initial_conn_window,
@@ -204,7 +206,9 @@ init_test_objs (struct test_objs *tobjs, unsigned initial_conn_window,
     tobjs->conn_pub.packet_out_malo =
                         lsquic_malo_create(sizeof(struct lsquic_packet_out));
     tobjs->conn_pub.path = &network_path;
+#if LSQUIC_CONN_STATS
     tobjs->conn_pub.conn_stats = &s_conn_stats;
+#endif
     tobjs->initial_stream_window = initial_stream_window;
     lsquic_send_ctl_init(&tobjs->send_ctl, &tobjs->alset, &tobjs->eng_pub,
         &tobjs->ver_neg, &tobjs->conn_pub, 0);

diff --git a/tests/test_stream.c b/tests/test_stream.c
@@ -350,7 +350,9 @@ static const struct conn_iface our_conn_if =
     .ci_write_ack     = write_ack,
 };
 
+#if LSQUIC_CONN_STATS
 static struct conn_stats s_conn_stats;
+#endif
 
 static void
 init_test_objs (struct test_objs *tobjs, unsigned initial_conn_window,
@@ -381,7 +383,9 @@ init_test_objs (struct test_objs *tobjs, unsigned initial_conn_window,
     tobjs->conn_pub.packet_out_malo =
                         lsquic_malo_create(sizeof(struct lsquic_packet_out));
     tobjs->conn_pub.path = &network_path;
+#if LSQUIC_CONN_STATS
     tobjs->conn_pub.conn_stats = &s_conn_stats;
+#endif
     tobjs->initial_stream_window = initial_stream_window;
     lsquic_send_ctl_init(&tobjs->send_ctl, &tobjs->alset, &tobjs->eng_pub,
         &tobjs->ver_neg, &tobjs->conn_pub, 0);
@@ -801,8 +805,15 @@ test_rem_data_loc_close_and_rst_in (struct test_objs *tobjs)
     s = lsquic_stream_shutdown(stream, 1);
     assert(0 == s);
 
-    assert(1 == lsquic_send_ctl_n_scheduled(&tobjs->send_ctl)); /* Shutdown performs a flush */
-    assert(stream->n_unacked == 1);
+    if (stream->sm_bflags & SMBF_IETF)
+    {
+        assert(1 == lsquic_send_ctl_n_scheduled(&tobjs->send_ctl)); /* Shutdown performs a flush */
+        assert(stream->n_unacked == 1);
+    }
+    else
+    {
+        /* gQUIC has RST scheduled to be sent, so no FIN is written */
+    }
 
     assert(!TAILQ_EMPTY(&tobjs->conn_pub.service_streams));
     assert((stream->sm_qflags & (SMQF_SERVICE_FLAGS)) == SMQF_CALL_ONCLOSE);
@@ -820,12 +831,20 @@ test_rem_data_loc_close_and_rst_in (struct test_objs *tobjs)
     assert(stream->sm_qflags & SMQF_CALL_ONCLOSE);
 
     lsquic_stream_rst_frame_sent(stream);
-    stream->n_unacked++;    /* RESET frame take a reference */
-    assert(!(stream->sm_qflags & SMQF_FREE_STREAM));    /* Not yet,
-        because: */ assert(stream->n_unacked == 2);
+    if (stream->sm_bflags & SMBF_IETF)
+    {
+        stream->n_unacked++;    /* RESET frame take a reference */
+        assert(!(stream->sm_qflags & SMQF_FREE_STREAM));    /* Not yet,
+            because: */ assert(stream->n_unacked == 2);
+    }
 
-    lsquic_stream_acked(stream, QUIC_FRAME_STREAM);
-    lsquic_stream_acked(stream, QUIC_FRAME_RST_STREAM);
+    if (stream->sm_bflags & SMBF_IETF)
+    {
+        lsquic_stream_acked(stream, QUIC_FRAME_STREAM);
+        lsquic_stream_acked(stream, QUIC_FRAME_RST_STREAM);
+    }
+    else
+        assert(stream->n_unacked == 0); /* STREAM frame was elided */
     assert(stream->sm_qflags & SMQF_FREE_STREAM);       /* OK, now */
 
     lsquic_stream_destroy(stream);
@@ -869,13 +888,15 @@ test_rem_data_loc_close (struct test_objs *tobjs)
     s = lsquic_stream_shutdown(stream, 1);
     assert(0 == s);
 
-    assert(1 == lsquic_send_ctl_n_scheduled(&tobjs->send_ctl)); /* Shutdown performs a flush */
+    if (stream->sm_bflags & SMBF_IETF)
+        assert(1 == lsquic_send_ctl_n_scheduled(&tobjs->send_ctl)); /* Shutdown performs a flush */
 
     assert(!TAILQ_EMPTY(&tobjs->conn_pub.service_streams));
     assert(stream->sm_qflags & SMQF_CALL_ONCLOSE);
 
     assert(!(stream->sm_qflags & SMQF_FREE_STREAM));
-    lsquic_stream_acked(stream, QUIC_FRAME_STREAM);
+    if (stream->sm_bflags & SMBF_IETF)
+        lsquic_stream_acked(stream, QUIC_FRAME_STREAM);
 
     lsquic_stream_rst_frame_sent(stream);
     stream->n_unacked++;    /* RESET frame take a reference */
@@ -1225,8 +1246,11 @@ test_loc_RST_rem_FIN (struct test_objs *tobjs)
     ++stream->n_unacked;    /* Fake sending of packet with RST_STREAM */
     assert(!TAILQ_EMPTY(&tobjs->conn_pub.sending_streams));
     assert((stream->sm_qflags & SMQF_SENDING_FLAGS) == SMQF_SEND_RST);
-    sss = lsquic_stream_sending_state(stream);
-    assert(SSS_DATA_SENT == sss);    /* FIN was packetized */
+    if (stream->sm_bflags & SMBF_IETF)
+    {
+        sss = lsquic_stream_sending_state(stream);
+        assert(SSS_DATA_SENT == sss);    /* FIN was packetized */
+    }
 
     s = lsquic_stream_frame_in(stream, new_frame_in(tobjs, 0, 90, 1));
     assert(s == 0);
@@ -1246,8 +1270,12 @@ test_loc_RST_rem_FIN (struct test_objs *tobjs)
     assert(TAILQ_EMPTY(&tobjs->conn_pub.sending_streams));
 
     lsquic_stream_call_on_close(stream);
-    assert(TAILQ_EMPTY(&tobjs->conn_pub.service_streams));  /* Not acked yet */
-    lsquic_stream_acked(stream, QUIC_FRAME_STREAM);
+
+    if (stream->sm_bflags & SMBF_IETF)
+    {
+        assert(TAILQ_EMPTY(&tobjs->conn_pub.service_streams));  /* Not acked yet */
+        lsquic_stream_acked(stream, QUIC_FRAME_STREAM);
+    }
 
     assert(!TAILQ_EMPTY(&tobjs->conn_pub.service_streams));
     assert((stream->sm_qflags & SMQF_SERVICE_FLAGS) == SMQF_FREE_STREAM);