diff --git a/CHANGELOG b/CHANGELOG index e99cf654a..40d107e8c 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,8 @@ +2018-08-22 + + - 1.12.3 + - [BUGFIX] Fix duplicate STREAM frame detection + 2018-08-20 - 1.12.2 diff --git a/include/lsquic.h b/include/lsquic.h index 7656e3d02..f028334fc 100644 --- a/include/lsquic.h +++ b/include/lsquic.h @@ -25,7 +25,7 @@ extern "C" { #define LSQUIC_MAJOR_VERSION 1 #define LSQUIC_MINOR_VERSION 12 -#define LSQUIC_PATCH_VERSION 2 +#define LSQUIC_PATCH_VERSION 3 /** * Engine flags: diff --git a/src/liblsquic/lsquic_di_nocopy.c b/src/liblsquic/lsquic_di_nocopy.c index 3e75d8b0d..604843821 100644 --- a/src/liblsquic/lsquic_di_nocopy.c +++ b/src/liblsquic/lsquic_di_nocopy.c @@ -196,6 +196,11 @@ frame_list_is_sane (const struct nocopy_data_in *ncdi) #define CASE(letter) ((int) (letter) << 8) +/* Not all errors are picked up by this function, as it is expensive (and + * potentially error-prone) to check for all possible error conditions. + * It an error be misclassified as an overlap or dup, in the worst case + * we end up with an application error instead of protocol violation. + */ static int insert_frame (struct nocopy_data_in *ncdi, struct stream_frame *new_frame, uint64_t read_offset, unsigned *p_n_frames) @@ -217,6 +222,13 @@ insert_frame (struct nocopy_data_in *ncdi, struct stream_frame *new_frame, return INS_FRAME_ERR | CASE('C'); if (DF_END(new_frame) > ncdi->ncdi_fin_off) return INS_FRAME_ERR | CASE('D'); + if (read_offset == DF_END(new_frame)) + return INS_FRAME_DUP | CASE('M'); + } + else + { + if (read_offset == DF_END(new_frame) && !DF_FIN(new_frame)) + return INS_FRAME_DUP | CASE('L'); } /* Find position in the list, going backwards. We go backwards because @@ -401,9 +413,17 @@ nocopy_di_get_frame (struct data_in *data_in, uint64_t read_offset) struct stream_frame *frame = TAILQ_FIRST(&ncdi->ncdi_frames_in); if (frame && frame->data_frame.df_offset + frame->data_frame.df_read_off == read_offset) + { + LSQ_DEBUG("get_frame: frame (off: %"PRIu64", size: %u, fin: %d), at " + "read offset %"PRIu64, DF_OFF(frame), DF_SIZE(frame), DF_FIN(frame), + read_offset); return &frame->data_frame; + } else + { + LSQ_DEBUG("get_frame: no frame at read offset %"PRIu64, read_offset); return NULL; + } } @@ -424,6 +444,8 @@ nocopy_di_frame_done (struct data_in *data_in, struct data_frame *data_frame) ncdi->ncdi_flags |= NCDI_FIN_REACHED; LSQ_DEBUG("FIN has been reached at offset %"PRIu64, DF_END(frame)); } + LSQ_DEBUG("frame (off: %"PRIu64", size: %u, fin: %d) done", + DF_OFF(frame), DF_SIZE(frame), DF_FIN(frame)); lsquic_packet_in_put(ncdi->ncdi_conn_pub->mm, frame->packet_in); lsquic_malo_put(frame); } diff --git a/test/unittests/CMakeLists.txt b/test/unittests/CMakeLists.txt index 2e67890bc..6c5cfeed8 100644 --- a/test/unittests/CMakeLists.txt +++ b/test/unittests/CMakeLists.txt @@ -221,6 +221,10 @@ add_executable(test_cubic test_cubic.c) target_link_libraries(test_cubic lsquic pthread libssl.a libcrypto.a m ${LIBS}) add_test(cubic test_cubic) +add_executable(test_di_nocopy test_di_nocopy.c) +target_link_libraries(test_di_nocopy lsquic pthread libssl.a libcrypto.a m ${LIBS}) +add_test(di_nocopy test_di_nocopy) + add_executable(test_dec test_dec.c) target_link_libraries(test_dec libssl.a libcrypto.a z m pthread ${LIBS}) @@ -437,6 +441,10 @@ add_executable(test_cubic test_cubic.c ../../wincompat/getopt.c ../../wincompat/ target_link_libraries(test_cubic lsquic ${LIBS_LIST}) add_test(cubic test_cubic) +add_executable(test_di_nocopy test_di_nocopy.c) +target_link_libraries(test_di_nocopy lsquic pthread libssl.a libcrypto.a m ${LIBS}) +add_test(di_nocopy test_di_nocopy) + add_executable(test_dec test_dec.c ../../wincompat/getopt.c ../../wincompat/getopt1.c) target_link_libraries(test_dec ${LIBS_LIST}) diff --git a/test/unittests/test_di_nocopy.c b/test/unittests/test_di_nocopy.c new file mode 100644 index 000000000..3a79f6c58 --- /dev/null +++ b/test/unittests/test_di_nocopy.c @@ -0,0 +1,333 @@ +/* Copyright (c) 2017 - 2018 LiteSpeed Technologies Inc. See LICENSE. */ +/* + * Test the "nocopy" data in stream + */ + +#include +#include +#include +#include +#include + +#include "lsquic.h" +#include "lsquic_int_types.h" +#include "lsquic_sfcw.h" +#include "lsquic_rtt.h" +#include "lsquic_conn_flow.h" +#include "lsquic_stream.h" +#include "lsquic_conn.h" +#include "lsquic_conn_public.h" +#include "lsquic_malo.h" +#include "lsquic_packet_common.h" +#include "lsquic_packet_in.h" +#include "lsquic_packet_out.h" +#include "lsquic_mm.h" +#include "lsquic_logger.h" +#include "lsquic_data_in_if.h" + + +struct nocopy_test +{ + int lineno; + + /* Setup: initial set of frames to insert and read until some offset */ + unsigned n_init_frames; + struct data_frame initial_frames[5]; + unsigned read_until; + + /* Test: data frame to insert and expected insert result */ + struct data_frame data_frame; + enum ins_frame ins; +}; + + +#define F(off, size, fin) { .df_offset = (off), .df_fin = (fin), .df_size = (size), } + +static const struct nocopy_test tests[] = +{ + + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(0, 300, 0), }, + .read_until = 300, + .data_frame = F(200, 100, 0), + .ins = INS_FRAME_DUP, + }, + + { .lineno = __LINE__, + .n_init_frames = 2, + .initial_frames = { F(0, 300, 0), F(300, 100, 0), }, + .read_until = 300, + .data_frame = F(200, 100, 0), + .ins = INS_FRAME_DUP, + }, + + { .lineno = __LINE__, + .n_init_frames = 2, + .initial_frames = { F(0, 300, 0), F(300, 0, 1), }, + .read_until = 300, + .data_frame = F(200, 100, 1), + .ins = INS_FRAME_DUP, + }, + + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(0, 301, 0), }, + .read_until = 301, + .data_frame = F(200, 100, 1), + .ins = INS_FRAME_ERR, + }, + + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(0, 400, 0), }, + .read_until = 301, + .data_frame = F(200, 100, 0), + .ins = INS_FRAME_DUP, + }, + + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(200, 100, 1), }, + .read_until = 0, + .data_frame = F(200, 50, 1), + .ins = INS_FRAME_ERR, + }, + + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(200, 100, 1), }, + .read_until = 0, + .data_frame = F(200, 150, 1), + .ins = INS_FRAME_ERR, + }, + + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(200, 100, 1), }, + .read_until = 0, + .data_frame = F(200, 101, 0), + .ins = INS_FRAME_ERR, + }, + + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(200, 100, 1), }, + .read_until = 0, + .data_frame = F(500, 1, 0), + .ins = INS_FRAME_ERR, + }, + + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(0, 100, 0), }, + .read_until = 100, + .data_frame = F(0, 100, 1), + .ins = INS_FRAME_OVERLAP, + }, + + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(0, 100, 1), }, + .read_until = 100, + .data_frame = F(0, 100, 1), + .ins = INS_FRAME_DUP, + }, + + /* TODO: Case 'F' and 'L' -- remove "case 'F'" */ + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(0, 100, 0), }, + .read_until = 100, + .data_frame = F(0, 100, 0), + .ins = INS_FRAME_DUP, + }, + + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(0, 100, 1), }, + .read_until = 10, + .data_frame = F(0, 100, 0), + .ins = INS_FRAME_DUP, + }, + + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(0, 100, 0), }, + .read_until = 10, + .data_frame = F(0, 100, 1), + .ins = INS_FRAME_OVERLAP, + }, + + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(0, 100, 0), }, + .read_until = 100, + .data_frame = F(100, 0, 0), + .ins = INS_FRAME_DUP, + }, + + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(0, 100, 0), }, + .read_until = 0, + .data_frame = F(50, 100, 0), + .ins = INS_FRAME_OVERLAP, + }, + + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(0, 100, 1), }, + .read_until = 0, + .data_frame = F(50, 100, 0), + .ins = INS_FRAME_ERR, + }, + + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(100, 100, 0), }, + .read_until = 0, + .data_frame = F(50, 100, 0), + .ins = INS_FRAME_OVERLAP, + }, + + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(100, 100, 0), }, + .read_until = 0, + .data_frame = F(50, 100, 1), + .ins = INS_FRAME_OVERLAP, /* This is really an error, + * but we ignore it. + */ + }, + + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(100, 100, 1), }, + .read_until = 0, + .data_frame = F(50, 100, 0), + .ins = INS_FRAME_OVERLAP, + }, + + { .lineno = __LINE__, + .n_init_frames = 1, + .initial_frames = { F(0, 100, 1), }, + .read_until = 60, + .data_frame = F(50, 2, 0), + .ins = INS_FRAME_DUP, + }, + + { .lineno = __LINE__, + .n_init_frames = 2, + .initial_frames = { F(0, 100, 0), F(200, 100, 0), }, + .read_until = 0, + .data_frame = F(50, 200, 0), + .ins = INS_FRAME_OVERLAP, + }, + + { .lineno = __LINE__, + .n_init_frames = 2, + .initial_frames = { F(0, 100, 0), F(200, 100, 0), }, + .read_until = 0, + .data_frame = F(100, 100, 0), + .ins = INS_FRAME_OK, + }, + + { .lineno = __LINE__, + .n_init_frames = 2, + .initial_frames = { F(0, 100, 0), F(200, 100, 0), }, + .read_until = 0, + .data_frame = F(100, 100, 1), + .ins = INS_FRAME_OK, /* Ignore another error */ + }, + +}; + + +static void +run_di_nocopy_test (const struct nocopy_test *test) +{ + struct lsquic_mm mm; + struct lsquic_conn_public conn_pub; + struct lsquic_conn conn; + struct stream_frame *frame; + struct data_in *di; + struct data_frame *data_frame; + enum ins_frame ins; + unsigned i; + unsigned nread, n_to_read; + + LSQ_NOTICE("running test on line %d", test->lineno); + + lsquic_mm_init(&mm); + memset(&conn, 0, sizeof(conn)); + conn_pub.lconn = &conn; + conn_pub.mm = &mm; + + di = data_in_nocopy_new(&conn_pub, 3); + + for (i = 0; i < test->n_init_frames; ++i) + { + frame = lsquic_malo_get(mm.malo.stream_frame); + frame->packet_in = lsquic_mm_get_packet_in(&mm); + frame->packet_in->pi_refcnt = 1; + frame->data_frame = test->initial_frames[i]; + ins = di->di_if->di_insert_frame(di, frame, 0); + assert(INS_FRAME_OK == ins); /* Self-test */ + } + + nread = 0; + while (nread < test->read_until) + { + data_frame = di->di_if->di_get_frame(di, nread); + assert(data_frame); /* Self-check */ + n_to_read = test->read_until - nread > data_frame->df_size - data_frame->df_read_off + ? data_frame->df_size - data_frame->df_read_off : test->read_until - nread; + data_frame->df_read_off += n_to_read; + nread += n_to_read; + if (data_frame->df_read_off == data_frame->df_size) + di->di_if->di_frame_done(di, data_frame); + else + { + assert(nread == test->read_until); + break; + } + } + + frame = lsquic_malo_get(mm.malo.stream_frame); + frame->packet_in = lsquic_mm_get_packet_in(&mm); + frame->packet_in->pi_refcnt = 1; + frame->data_frame = test->data_frame; + ins = di->di_if->di_insert_frame(di, frame, test->read_until); + assert(test->ins == ins); + + di->di_if->di_destroy(di); + lsquic_mm_cleanup(&mm); +} + + +int +main (int argc, char **argv) +{ + const struct nocopy_test *test; + int opt; + + lsquic_log_to_fstream(stderr, LLTS_NONE); + + while (-1 != (opt = getopt(argc, argv, "l:"))) + { + switch (opt) + { + case 'l': + lsquic_logger_lopt(optarg); + break; + default: + return 1; + } + } + + for (test = tests; test < tests + sizeof(tests) / sizeof(tests[0]); ++test) + run_di_nocopy_test(test); + + return 0; +} diff --git a/test/unittests/test_ver_nego.c b/test/unittests/test_ver_nego.c index 68660cef5..917bf16a0 100644 --- a/test/unittests/test_ver_nego.c +++ b/test/unittests/test_ver_nego.c @@ -20,7 +20,6 @@ #include "lsquic_version.h" - /* The struct is used to test both generation and parsing of version * negotiation packet. */ @@ -65,7 +64,6 @@ static const struct gen_ver_nego_test tests[] = { .gvnt_len = -1, /* Invalid version specified in the bitmask */ }, - { .gvnt_lineno = __LINE__, .gvnt_cid = 0x0102030405060708UL, .gvnt_versions = (1 << LSQVER_035) | (1 << LSQVER_039) | (1 << LSQVER_043), @@ -80,6 +78,21 @@ static const struct gen_ver_nego_test tests[] = { 'Q', '0', '4', '3', }, }, + + { .gvnt_lineno = __LINE__, + .gvnt_cid = 0x0102030405060708UL, + .gvnt_versions = (1 << LSQVER_044), + .gvnt_bufsz = 18, + .gvnt_len = 18, + .gvnt_buf = { + 0x80, + 0x00, 0x00, 0x00, 0x00, /* Version negotiation indicator */ + 0x05, /* SCIL */ + 0x08, 0x07, 0x06, 0x05, 0x04, 0x03, 0x02, 0x01, /* Connection ID */ + 'Q', '0', '4', '4', + }, + }, + };