From 292abba1f8221ad63d502982e9b4546cd531b33b Mon Sep 17 00:00:00 2001 From: Dmitri Tikhonov Date: Wed, 23 Dec 2020 09:06:05 -0500 Subject: [PATCH] Release 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. --- CHANGELOG | 11 +++ CONTRIBUTORS.txt | 1 + docs/apiref.rst | 2 +- docs/conf.py | 2 +- include/lsquic.h | 4 +- src/liblsquic/lsquic_packet_out.c | 8 +-- src/liblsquic/lsquic_send_ctl.c | 3 +- src/liblsquic/lsquic_stream.c | 40 ++++++++--- src/liblsquic/lsquic_stream.h | 9 ++- tests/test_elision.c | 6 +- tests/test_stream.c | 108 ++++++++++++++++++++++++++++-- 11 files changed, 163 insertions(+), 31 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 20bc12f11..4f5ab271a 100644 --- 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 index a96c3cd0b..01b3e9fe7 100644 --- 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 index f9330f76b..015c6a032 100644 --- 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 index 25d7d255d..0e957cf02 100644 --- 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 index e37c31794..005bcdf32 100644 --- 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 index 1bfc7b820..9bb6b8f6f 100644 --- 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 index c26bb6552..504877761 100644 --- 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 index b25041508..2e22f4af3 100644 --- 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 index 864f214dd..8a956db47 100644 --- 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 index 0bf1fbb62..6f9ef9970 100644 --- 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 index 7e103d40b..a8aab0d91 100644 --- 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, },