Release 2.26.2

Dmitri Tikhonov · Dmitri Tikhonov · commit 292abba1f822 · 2020-12-23T09:06:05.000-05:00
- [BUGFIX] Do not drop incoming data when STOP_SENDING is received.
- [BUGFIX] Receipt of STOP_SENDING should not cause read-reset.
- [BUGFIX] Allow stream writes after receiving RESET.
- [BUGFIX] Typo in stream: ANDing enum with wrong flag.
- [BUGFIX] Reset elision: do not use zero as special stream ID value,
  for zero is a valid stream ID in IETF QUIC.
- [API] Add optional on_conncloseframe_received() callback.
- Use zero error code in RESET stream sent in response to STOP_SENDING.
diff --git a/CHANGELOG b/CHANGELOG
@@ -1,3 +1,14 @@
+2020-12-23
+    - 2.26.2
+    - [BUGFIX] Do not drop incoming data when STOP_SENDING is received.
+    - [BUGFIX] Receipt of STOP_SENDING should not cause read-reset.
+    - [BUGFIX] Allow stream writes after receiving RESET.
+    - [BUGFIX] Typo in stream: ANDing enum with wrong flag.
+    - [BUGFIX] Reset elision: do not use zero as special stream ID value,
+      for zero is a valid stream ID in IETF QUIC.
+    - [API] Add optional on_conncloseframe_received() callback.
+    - Use zero error code in RESET stream sent in response to STOP_SENDING.
+
 2020-12-17
     - 2.26.1
     - [BUGFIX] Migration corner cases: drop or pad over path challenge
diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt
@@ -13,6 +13,7 @@ to the LiteSpeed QUIC and HTTP/3 Library:
     - Victor Stewart -- Generate SCIDs API (connection ID steering)
     - Aaron France -- Shared library support and Lisp bindings
     - Suma Subbarao -- Use callback to supply client's SSL_CTX
+    - Paul Sheer -- Callback on arrival of CONNECTION_CLOSE frame
 
 Thank you!
 
diff --git a/docs/apiref.rst b/docs/apiref.rst
@@ -1294,7 +1294,7 @@ the engine to communicate with the user code:
         signals the user to stop reading, writing, or both.
 
         Note that resets differ in gQUIC and IETF QUIC.  In gQUIC, `how` is
-        always 2; in IETF QUIC, `how` is either 0 or 1 because on can reset
+        always 2; in IETF QUIC, `how` is either 0 or 1 because one can reset
         just one direction in IETF QUIC.
 
         This callback is optional.  The reset error can still be collected
diff --git a/docs/conf.py b/docs/conf.py
@@ -26,7 +26,7 @@
 # The short X.Y version
 version = u'2.26'
 # The full version, including alpha/beta/rc tags
-release = u'2.26.1'
+release = u'2.26.2'
 
 
 # -- 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 26
-#define LSQUIC_PATCH_VERSION 1
+#define LSQUIC_PATCH_VERSION 2
 
 /**
  * Engine flags:
@@ -217,7 +217,7 @@ struct lsquic_stream_if {
      * signals the user to stop reading, writing, or both.
      *
      * Note that resets differ in gQUIC and IETF QUIC.  In gQUIC, `how' is
-     * always 2; in IETF QUIC, `how' is either 0 or 1 because on can reset
+     * always 2; in IETF QUIC, `how' is either 0 or 1 because one can reset
      * just one direction in IETF QUIC.
      */
     void (*on_reset)    (lsquic_stream_t *s, lsquic_stream_ctx_t *h, int how);
diff --git a/src/liblsquic/lsquic_packet_out.c b/src/liblsquic/lsquic_packet_out.c
@@ -297,7 +297,7 @@ lsquic_packet_out_destroy (lsquic_packet_out_t *packet_out,
 }
 
 
-/* If `stream_id' is zero, stream frames from all reset streams are elided.
+/* If `stream_id' is UINT64_MAX, stream frames from all reset streams are elided.
  * Otherwise, elision is limited to the specified stream.
  */
 unsigned
@@ -320,16 +320,16 @@ lsquic_packet_out_elide_reset_stream_frames (lsquic_packet_out_t *packet_out,
         {
             ++n_stream_frames;
 
-            if (stream_id)
+            if (stream_id != UINT64_MAX)
             {
                 victim = frec->fe_stream->id == stream_id;
                 if (victim)
                 {
-                    assert(lsquic_stream_is_reset(frec->fe_stream));
+                    assert(lsquic_stream_is_write_reset(frec->fe_stream));
                 }
             }
             else
-                victim = lsquic_stream_is_reset(frec->fe_stream);
+                victim = lsquic_stream_is_write_reset(frec->fe_stream);
 
             if (victim)
             {
diff --git a/src/liblsquic/lsquic_send_ctl.c b/src/liblsquic/lsquic_send_ctl.c
@@ -1430,7 +1430,8 @@ send_ctl_next_lost (lsquic_send_ctl_t *ctl)
         {
             if (0 == (lost_packet->po_flags & PO_MINI))
             {
-                lsquic_packet_out_elide_reset_stream_frames(lost_packet, 0);
+                lsquic_packet_out_elide_reset_stream_frames(lost_packet,
+                                                                    UINT64_MAX);
                 if (lost_packet->po_regen_sz >= lost_packet->po_data_sz)
                 {
                     LSQ_DEBUG("Dropping packet %"PRIu64" from lost queue",
diff --git a/src/liblsquic/lsquic_stream.c b/src/liblsquic/lsquic_stream.c
@@ -872,6 +872,17 @@ stream_readable_discard (struct lsquic_stream *stream)
 }
 
 
+static int
+stream_is_read_reset (const struct lsquic_stream *stream)
+{
+    if (stream->sm_bflags & SMBF_IETF)
+        return stream->stream_flags & STREAM_RST_RECVD;
+    else
+        return (stream->stream_flags & (STREAM_RST_RECVD|STREAM_RST_SENT))
+            || (stream->sm_qflags & SMQF_SEND_RST);
+}
+
+
 int
 lsquic_stream_readable (struct lsquic_stream *stream)
 {
@@ -885,13 +896,27 @@ lsquic_stream_readable (struct lsquic_stream *stream)
          *   lsquic_stream_read() will return -1 (we want the user to be
          *   able to collect the error).
          */
-        ||  lsquic_stream_is_reset(stream)
+        ||  stream_is_read_reset(stream)
         /* Type-dependent readability check: */
         ||  stream->sm_readable(stream);
     ;
 }
 
 
+/* Return true if write end of the stream has been reset.
+ * Note that the logic for gQUIC is the same for write and read resets.
+ */
+int
+lsquic_stream_is_write_reset (const struct lsquic_stream *stream)
+{
+    if (stream->sm_bflags & SMBF_IETF)
+        return stream->stream_flags & STREAM_SS_RECVD;
+    else
+        return (stream->stream_flags & (STREAM_RST_RECVD|STREAM_RST_SENT))
+            || (stream->sm_qflags & SMQF_SEND_RST);
+}
+
+
 static int
 stream_writeable (struct lsquic_stream *stream)
 {
@@ -901,7 +926,7 @@ stream_writeable (struct lsquic_stream *stream)
          *   lsquic_stream_write() will return -1 (we want the user to be
          *   able to collect the error).
          */
-           lsquic_stream_is_reset(stream)
+           lsquic_stream_is_write_reset(stream)
         /* - Data can be written to stream: */
         || lsquic_stream_write_avail(stream)
     ;
@@ -1294,13 +1319,12 @@ lsquic_stream_stop_sending_in (struct lsquic_stream *stream,
     maybe_conn_to_tickable_if_writeable(stream, 0);
 
     lsquic_sfcw_consume_rem(&stream->fc);
-    drop_frames_in(stream);
     drop_buffered_data(stream);
     maybe_elide_stream_frames(stream);
 
     if (!(stream->stream_flags & (STREAM_RST_SENT|STREAM_FIN_SENT))
                                     && !(stream->sm_qflags & SMQF_SEND_RST))
-        lsquic_stream_reset_ext(stream, error_code, 0);
+        lsquic_stream_reset_ext(stream, 0, 0);
 
     maybe_finish_stream(stream);
     maybe_schedule_call_on_close(stream);
@@ -1628,7 +1652,7 @@ lsquic_stream_readf (struct lsquic_stream *stream,
 
     SM_HISTORY_APPEND(stream, SHE_USER_READ);
 
-    if (lsquic_stream_is_reset(stream))
+    if (stream_is_read_reset(stream))
     {
         if (stream->stream_flags & STREAM_RST_RECVD)
             stream->stream_flags |= STREAM_RST_READ;
@@ -1816,7 +1840,7 @@ stream_shutdown_write (lsquic_stream_t *stream)
             && !(stream->stream_flags & (STREAM_FIN_SENT|STREAM_RST_SENT))
                 && !stream_is_incoming_unidir(stream)
                         /* In gQUIC, receiving a RESET means "stop sending" */
-                    && !(!(stream->sm_qflags & SMBF_IETF)
+                    && !(!(stream->sm_bflags & SMBF_IETF)
                                 && (stream->stream_flags & STREAM_RST_RECVD)))
     {
         if ((stream->sm_bflags & SMBF_USE_HEADERS)
@@ -2530,7 +2554,7 @@ lsquic_stream_flush_threshold (const struct lsquic_stream *stream,
             return -1;                                                      \
         }                                                                   \
     }                                                                       \
-    if (lsquic_stream_is_reset(stream))                                     \
+    if (lsquic_stream_is_write_reset(stream))                               \
     {                                                                       \
         LSQ_INFO("Attempt to write to stream after it had been reset");     \
         errno = ECONNRESET;                                                 \
@@ -4600,7 +4624,7 @@ lsquic_stream_get_hset (struct lsquic_stream *stream)
 {
     void *hset;
 
-    if (lsquic_stream_is_reset(stream))
+    if (stream_is_read_reset(stream))
     {
         LSQ_INFO("%s: stream is reset, no headers returned", __func__);
         errno = ECONNRESET;
diff --git a/src/liblsquic/lsquic_stream.h b/src/liblsquic/lsquic_stream.h
@@ -429,16 +429,15 @@ lsquic_stream_call_on_new (lsquic_stream_t *);
 void
 lsquic_stream_destroy (lsquic_stream_t *);
 
-/* Any of these flags will cause user-facing read and write and
- * shutdown calls to return an error.  They also make the stream
- * both readable and writeable, as we want the user to collect
- * the error.
- */
+/* True if either read or write side of the stream has been reset */
 #define lsquic_stream_is_reset(stream) \
     (((stream)->stream_flags & \
                     (STREAM_RST_RECVD|STREAM_RST_SENT|STREAM_SS_RECVD)) \
         || ((stream)->sm_qflags & SMQF_SEND_RST))
 
+int
+lsquic_stream_is_write_reset (const struct lsquic_stream *);
+
 /* Data that from the network gets inserted into the stream using
  * lsquic_stream_frame_in() function.  Returns 0 on success, -1 on
  * failure.  The latter may be caused by flow control violation or
diff --git a/tests/test_elision.c b/tests/test_elision.c
@@ -124,7 +124,7 @@ elide_single_stream_frame (void)
 
     streams[0].stream_flags |= STREAM_RST_SENT;
 
-    lsquic_packet_out_elide_reset_stream_frames(packet_out, 0);
+    lsquic_packet_out_elide_reset_stream_frames(packet_out, UINT64_MAX);
     assert(0 == streams[0].n_unacked);
     assert(0 == packet_out->po_frame_types);
     assert(!lsquic_pofi_first(&pofi, packet_out));
@@ -193,7 +193,7 @@ shrink_packet_post_elision (void)
 
     streams[0].stream_flags |= STREAM_RST_SENT;
 
-    lsquic_packet_out_elide_reset_stream_frames(packet_out, 0);
+    lsquic_packet_out_elide_reset_stream_frames(packet_out, UINT64_MAX);
     assert(0 == streams[0].n_unacked);
 
     assert(QUIC_FTBIT_STREAM == packet_out->po_frame_types);
@@ -364,7 +364,7 @@ elide_three_stream_frames (int chop_regen)
 
     if (chop_regen)
         lsquic_packet_out_chop_regen(packet_out);
-    lsquic_packet_out_elide_reset_stream_frames(packet_out, 0);
+    lsquic_packet_out_elide_reset_stream_frames(packet_out, UINT64_MAX);
 
     assert(ref_out->po_data_sz == packet_out->po_data_sz + (chop_regen ? 5 : 0));
     assert(ref_out->po_regen_sz == packet_out->po_regen_sz + (chop_regen ? 5 : 0));
diff --git a/tests/test_stream.c b/tests/test_stream.c
@@ -1030,18 +1030,113 @@ test_loc_data_rem_RST (struct test_objs *tobjs)
     s = lsquic_stream_frame_in(stream, new_frame_in(tobjs, 0, 100, 0));
     assert(0 == s);
     s_onreset_called = (struct reset_call_ctx) { NULL, -1, };
+    s = lsquic_stream_rst_in(stream, 200, 0);
+    assert(0 == s);
+    assert(s_onreset_called.stream == stream);
     if (stream->sm_bflags & SMBF_IETF)
-        lsquic_stream_stop_sending_in(stream, 12345);
+        assert(s_onreset_called.how == 0);
     else
+        assert(s_onreset_called.how == 2);
+
+    ack_packet(&tobjs->send_ctl, 1);
+
+    if (!(stream->sm_bflags & SMBF_IETF))
     {
-        s = lsquic_stream_rst_in(stream, 200, 0);
-        assert(0 == s);
+        assert(!TAILQ_EMPTY(&tobjs->conn_pub.sending_streams));
+        assert((stream->sm_qflags & SMQF_SENDING_FLAGS) == SMQF_SEND_RST);
     }
-    assert(s_onreset_called.stream == stream);
+
+    /* Not yet closed: error needs to be collected */
+    assert(TAILQ_EMPTY(&tobjs->conn_pub.service_streams));
+    assert(0 == (stream->sm_qflags & SMQF_SERVICE_FLAGS));
+
+    n = lsquic_stream_write(stream, buf, 100);
     if (stream->sm_bflags & SMBF_IETF)
-        assert(s_onreset_called.how == 1);
+        assert(100 == n);   /* Write successful after reset in IETF */
     else
-        assert(s_onreset_called.how == 2);
+        assert(-1 == n);    /* Error collected */
+    s = lsquic_stream_close(stream);
+    assert(0 == s);     /* Stream successfully closed */
+
+    if (stream->sm_bflags & SMBF_IETF)
+        assert(stream->n_unacked == 1);
+
+    assert(!TAILQ_EMPTY(&tobjs->conn_pub.service_streams));
+    assert((stream->sm_qflags & SMQF_SERVICE_FLAGS) == SMQF_CALL_ONCLOSE);
+
+    if (!(stream->sm_bflags & SMBF_IETF))
+        lsquic_stream_rst_frame_sent(stream);
+
+    lsquic_stream_call_on_close(stream);
+
+    assert(TAILQ_EMPTY(&tobjs->conn_pub.sending_streams));
+
+    if (stream->sm_bflags & SMBF_IETF)
+    {
+        /* FIN packet has not been acked yet: */
+        assert(TAILQ_EMPTY(&tobjs->conn_pub.service_streams));
+        /* Now ack it: */
+        ack_packet(&tobjs->send_ctl, 1);
+    }
+
+    assert(!TAILQ_EMPTY(&tobjs->conn_pub.service_streams));
+    assert((stream->sm_qflags & SMQF_SERVICE_FLAGS) == SMQF_FREE_STREAM);
+
+    lsquic_stream_destroy(stream);
+    assert(TAILQ_EMPTY(&tobjs->conn_pub.service_streams));
+
+    assert(200 == tobjs->conn_pub.cfcw.cf_max_recv_off);
+    assert(200 == tobjs->conn_pub.cfcw.cf_read_off);
+}
+
+
+/* Client: we send some data (no FIN), and remote end sends some data and
+ * then sends STOP_SENDING
+ */
+static void
+test_loc_data_rem_SS (struct test_objs *tobjs)
+{
+    lsquic_packet_out_t *packet_out;
+    lsquic_stream_t *stream;
+    char buf_out[0x100];
+    unsigned char buf[0x100];
+    ssize_t n;
+    int s, fin;
+
+    init_buf(buf_out, sizeof(buf_out));
+
+    stream = new_stream(tobjs, 345);
+    assert(stream->sm_bflags & SMBF_IETF);  /* STOP_SENDING is IETF-only */
+    n = lsquic_stream_write(stream, buf_out, 100);
+    assert(n == 100);
+    assert(0 == lsquic_send_ctl_n_scheduled(&tobjs->send_ctl));
+
+    s = lsquic_stream_flush(stream);
+    assert(1 == lsquic_send_ctl_n_scheduled(&tobjs->send_ctl));
+
+    n = read_from_scheduled_packets(&tobjs->send_ctl, stream->id, buf,
+                                                    sizeof(buf), 0, &fin, 0);
+    assert(100 == n);
+    assert(0 == memcmp(buf_out, buf, 100));
+    assert(!fin);
+
+    /* Pretend we sent out a packet: */
+    packet_out = lsquic_send_ctl_next_packet_to_send(&tobjs->send_ctl, 0);
+    lsquic_send_ctl_sent_packet(&tobjs->send_ctl, packet_out);
+
+    s = lsquic_stream_frame_in(stream, new_frame_in(tobjs, 0, 100, 0));
+    assert(0 == s);
+    s_onreset_called = (struct reset_call_ctx) { NULL, -1, };
+    lsquic_stream_stop_sending_in(stream, 12345);
+    assert(s_onreset_called.stream == stream);
+    assert(s_onreset_called.how == 1);
+
+    /* Incoming STOP_SENDING should not affect the ability to read from
+     * stream.
+     */
+    unsigned char mybuf[123];
+    const ssize_t nread = lsquic_stream_read(stream, mybuf, sizeof(mybuf));
+    assert(nread == 100);
 
     ack_packet(&tobjs->send_ctl, 1);
 
@@ -1474,6 +1569,7 @@ test_termination (void)
         { 1, 0, test_rem_data_loc_close, },
         { 1, 1, test_loc_FIN_rem_RST, },
         { 1, 1, test_loc_data_rem_RST, },
+        { 0, 1, test_loc_data_rem_SS, },
         { 1, 0, test_loc_RST_rem_FIN, },
         { 1, 1, test_gapless_elision_beginning, },
         { 1, 1, test_gapless_elision_middle, },

Original file line number	Diff line number	Diff line change
`@@ -297,7 +297,7 @@ lsquic_packet_out_destroy (lsquic_packet_out_t *packet_out,`
`297`	`297`	`}`
`298`	`298`
`299`	`299`
`300`		-/* If `stream_id' is zero, stream frames from all reset streams are elided.
	`300`	+/* If `stream_id' is UINT64_MAX, stream frames from all reset streams are elided.
`301`	`301`	`* Otherwise, elision is limited to the specified stream.`
`302`	`302`	`*/`
`303`	`303`	`unsigned`
`@@ -320,16 +320,16 @@ lsquic_packet_out_elide_reset_stream_frames (lsquic_packet_out_t *packet_out,`
`320`	`320`	`{`
`321`	`321`	`++n_stream_frames;`
`322`	`322`
`323`		`- if (stream_id)`
	`323`	`+ if (stream_id != UINT64_MAX)`
`324`	`324`	`{`
`325`	`325`	`victim = frec->fe_stream->id == stream_id;`
`326`	`326`	`if (victim)`
`327`	`327`	`{`
`328`		`- assert(lsquic_stream_is_reset(frec->fe_stream));`
	`328`	`+ assert(lsquic_stream_is_write_reset(frec->fe_stream));`
`329`	`329`	`}`
`330`	`330`	`}`
`331`	`331`	`else`
`332`		`- victim = lsquic_stream_is_reset(frec->fe_stream);`
	`332`	`+ victim = lsquic_stream_is_write_reset(frec->fe_stream);`
`333`	`333`
`334`	`334`	`if (victim)`
`335`	`335`	`{`
Original file line number	Diff line number	Diff line change
`@@ -1430,7 +1430,8 @@ send_ctl_next_lost (lsquic_send_ctl_t *ctl)`
`1430`	`1430`	`{`
`1431`	`1431`	`if (0 == (lost_packet->po_flags & PO_MINI))`
`1432`	`1432`	`{`
`1433`		`- lsquic_packet_out_elide_reset_stream_frames(lost_packet, 0);`
	`1433`	`+ lsquic_packet_out_elide_reset_stream_frames(lost_packet,`
	`1434`	`+ UINT64_MAX);`
`1434`	`1435`	`if (lost_packet->po_regen_sz >= lost_packet->po_data_sz)`
`1435`	`1436`	`{`
`1436`	`1437`	`LSQ_DEBUG("Dropping packet %"PRIu64" from lost queue",`