From ab017f94b7caa4ac42b85e1428121c7da1ce0129 Mon Sep 17 00:00:00 2001 From: Christian Huitema Date: Mon, 1 Apr 2024 15:41:46 -0700 Subject: [PATCH 1/4] Add unit test of picoquic_provide_stream_data_buffer --- UnitTest1/unittest1.cpp | 7 ++ picoquic/frames.c | 13 +-- picoquic/picoquic_internal.h | 13 +++ picoquic_t/picoquic_t.c | 1 + picoquictest/picoquictest.h | 1 + picoquictest/stream0_frame_test.c | 152 ++++++++++++++++++++++++++++++ 6 files changed, 175 insertions(+), 12 deletions(-) diff --git a/UnitTest1/unittest1.cpp b/UnitTest1/unittest1.cpp index 3026840ea..26e8f6779 100644 --- a/UnitTest1/unittest1.cpp +++ b/UnitTest1/unittest1.cpp @@ -694,6 +694,13 @@ namespace UnitTest1 { int ret = stream_rank_test(); + Assert::AreEqual(ret, 0); + } + + TEST_METHOD(provide_stream_buffer) + { + int ret = provide_stream_buffer_test(); + Assert::AreEqual(ret, 0); } diff --git a/picoquic/frames.c b/picoquic/frames.c index 6b5a0aa8c..43ec6aabf 100644 --- a/picoquic/frames.c +++ b/picoquic/frames.c @@ -1506,17 +1506,6 @@ uint8_t * picoquic_format_blocked_frames(picoquic_cnx_t* cnx, uint8_t* bytes, ui /* handling of stream frames */ -typedef struct st_picoquic_stream_data_buffer_argument_t { - uint8_t* bytes; /* Points to the beginning of the encoding of the stream frame */ - size_t byte_index; /* Current index position after encoding type, stream-id and offset */ - size_t byte_space; /* Number of bytes available in the packet after the current index */ - size_t allowed_space; /* Maximum number of bytes that the application is authorized to write */ - size_t length; /* number of bytes that the application commits to write */ - int is_fin; /* Whether this is the end of the stream */ - int is_still_active; /* whether the stream is still considered active after this call */ - uint8_t* app_buffer; /* buffer provided to the application. */ -} picoquic_stream_data_buffer_argument_t; - static size_t picoquic_encode_length_of_stream_frame( uint8_t* bytes, size_t byte_index, size_t byte_space, size_t length, size_t *start_index) { @@ -1573,7 +1562,7 @@ uint8_t* picoquic_provide_stream_data_buffer(void* context, size_t length, int i return buffer; } -static uint8_t* picoquic_format_stream_frame_header(uint8_t* bytes, uint8_t* bytes_max, uint64_t stream_id, uint64_t offset) +uint8_t* picoquic_format_stream_frame_header(uint8_t* bytes, uint8_t* bytes_max, uint64_t stream_id, uint64_t offset) { uint8_t* bytes0 = bytes; if ((bytes = picoquic_frames_uint8_encode(bytes, bytes_max, picoquic_frame_type_stream_range_min)) != NULL && diff --git a/picoquic/picoquic_internal.h b/picoquic/picoquic_internal.h index 6f0b7a249..a30f29c6d 100644 --- a/picoquic/picoquic_internal.h +++ b/picoquic/picoquic_internal.h @@ -1889,8 +1889,21 @@ void picoquic_process_ack_of_frames(picoquic_cnx_t* cnx, picoquic_packet_t* p, int is_spurious, uint64_t current_time); /* Coding and decoding of frames */ +typedef struct st_picoquic_stream_data_buffer_argument_t { + uint8_t* bytes; /* Points to the beginning of the encoding of the stream frame */ + size_t byte_index; /* Current index position after encoding type, stream-id and offset */ + size_t byte_space; /* Number of bytes available in the packet after the current index */ + size_t allowed_space; /* Maximum number of bytes that the application is authorized to write */ + size_t length; /* number of bytes that the application commits to write */ + int is_fin; /* Whether this is the end of the stream */ + int is_still_active; /* whether the stream is still considered active after this call */ + uint8_t* app_buffer; /* buffer provided to the application. */ +} picoquic_stream_data_buffer_argument_t; int picoquic_is_stream_frame_unlimited(const uint8_t* bytes); + +uint8_t* picoquic_format_stream_frame_header(uint8_t* bytes, uint8_t* bytes_max, uint64_t stream_id, uint64_t offset); + int picoquic_parse_stream_header( const uint8_t* bytes, size_t bytes_max, uint64_t* stream_id, uint64_t* offset, size_t* data_length, int* fin, diff --git a/picoquic_t/picoquic_t.c b/picoquic_t/picoquic_t.c index 118091e72..5cdecfe16 100644 --- a/picoquic_t/picoquic_t.c +++ b/picoquic_t/picoquic_t.c @@ -145,6 +145,7 @@ static const picoquic_test_def_t test_table[] = { { "vn_tp", vn_tp_test }, { "vn_compat", vn_compat_test }, { "stream_rank", stream_rank_test }, + { "provide_stream_buffer", provide_stream_buffer_test }, { "transport_param", transport_param_test }, { "tls_api_sni", tls_api_sni_test }, { "tls_api_alpn", tls_api_alpn_test }, diff --git a/picoquictest/picoquictest.h b/picoquictest/picoquictest.h index 945e64479..2e1f3b18f 100644 --- a/picoquictest/picoquictest.h +++ b/picoquictest/picoquictest.h @@ -318,6 +318,7 @@ int bad_cnxid_test(); int stream_splay_test(); int stream_output_test(); int stream_rank_test(); +int provide_stream_buffer_test(); int not_before_cnxid_test(); int send_stream_blocked_test(); int stream_ack_test(); diff --git a/picoquictest/stream0_frame_test.c b/picoquictest/stream0_frame_test.c index 74aea950c..a9414c627 100644 --- a/picoquictest/stream0_frame_test.c +++ b/picoquictest/stream0_frame_test.c @@ -898,3 +898,155 @@ int stream_rank_test() return ret; } + +/* Unit test of "picoquic_provide_stream_data_buffer" + */ + +static int picoquic_set_stream_buffer_context(picoquic_stream_data_buffer_argument_t * stream_data_context, + uint8_t * bytes, uint8_t * bytes_max, uint64_t stream_id, uint64_t stream_offset) +{ + int ret = 0; + uint8_t* bytes0 = bytes; + memset(stream_data_context, 0, sizeof(picoquic_stream_data_buffer_argument_t)); + if ((bytes = picoquic_format_stream_frame_header(bytes, bytes_max, stream_id, stream_offset)) == NULL) { + ret = -1; + } + else { + /* Compute the length */ + stream_data_context->bytes = bytes0; + stream_data_context->byte_index = bytes - bytes0; + stream_data_context->allowed_space = bytes_max - bytes; + stream_data_context->byte_space = bytes_max - bytes; + stream_data_context->length = 0; + stream_data_context->is_fin = 0; + stream_data_context->is_still_active = 0; + stream_data_context->app_buffer = NULL; + } + return ret; +} + +typedef enum { + size_test_full = 0, + size_test_minus_1, + size_test_minus_2, + size_test_minus_3, + size_test_1, + size_test_0, + size_test_too_long, + size_test_last +} size_test_enum; + +int provide_stream_buffer_test_one(uint64_t stream_id, uint64_t stream_offset, size_test_enum size_test, int is_fin) +{ + uint8_t packet[512]; + uint8_t test_data[512]; + picoquic_stream_data_buffer_argument_t stream_data_context; + int ret = picoquic_set_stream_buffer_context(&stream_data_context, packet, packet + sizeof(packet), stream_id, stream_offset); + size_t length = 0; + + if (ret == 0) { + + switch (size_test) { + case size_test_full: + length = stream_data_context.byte_space; + break; + case size_test_minus_1: + length = stream_data_context.byte_space - 1; + break; + case size_test_minus_2: + length = stream_data_context.byte_space - 2; + break; + case size_test_minus_3: + length = stream_data_context.byte_space - 3; + break; + case size_test_1: + length = 1; + break; + case size_test_0: + length = 0; + break; + case size_test_too_long: + length = stream_data_context.byte_space; + stream_data_context.allowed_space = length - 1; + break; + default: + ret = -1; + break; + } + } + + if (ret == 0) { + uint8_t * data_ptr = picoquic_provide_stream_data_buffer(&stream_data_context, length, is_fin, 0); + + if (size_test == size_test_too_long) { + ret = (data_ptr == NULL) ? 0 : -1; + } + else if (data_ptr == NULL) { + ret = -1; + } + else if (data_ptr + length > packet + sizeof(packet)) { + ret = -1; + } + else { + uint8_t* packet_start = packet; + uint64_t received_stream_id; + uint64_t received_offset; + size_t received_length = 0; + size_t consumed = 0; + int received_fin = 0; + + if (length > 0) { + /* set test data value */ + for (int i = 0; i < length; i++) { + test_data[i] = (uint8_t)(i ^ 0x7f); + } + /* Fill the data space */ + memcpy(data_ptr, test_data, length); + } + /* Get the start point of the data frame */ + while (*packet_start == picoquic_frame_type_padding && packet_start < packet + sizeof(packet)) { + packet_start++; + } + /* decode the stream header */ + if (picoquic_parse_stream_header(packet_start, + sizeof(packet) - (packet_start - packet), &received_stream_id, &received_offset, + &received_length, &received_fin, &consumed) != 0) { + ret = -1; + } + else if (received_stream_id != stream_id || + received_offset != stream_offset || + received_length != length || + received_fin != is_fin) { + ret = -1; + } + else if (length > 0 && + memcmp(packet_start + consumed, test_data, length) != 0) { + ret = -1; + } + } + } + return ret; +} + +int provide_stream_buffer_test() +{ + uint64_t stream_ids[4] = { 0, 7, 127, 0x10000 }; + uint64_t offsets[4] = { 0, 1, 65, 0x10000 }; + int ret = 0; + + for (int i_stream = 0; ret == 0 && i_stream < 4; i_stream++) { + for (int i_offset = 0; ret == 0 && i_offset < 4; i_offset++) { + for (int is_fin = 0; ret == 0 && is_fin < 2; is_fin++) { + for (size_test_enum size_test = 0; ret == 0 && size_test < size_test_last; size_test++) { + ret = provide_stream_buffer_test_one(stream_ids[i_stream], offsets[i_offset], size_test, is_fin); + if (ret != 0) { + DBG_PRINTF("Fails for stream %" PRIu64 ", offset %" PRIu64 ", fin %d, test %d", + stream_ids[i_stream], offsets[i_offset], is_fin, size_test); + break; + } + } + } + } + } + return ret; +} \ No newline at end of file From aeeca3635c430389866a37ea16c8455a140ceff4 Mon Sep 17 00:00:00 2001 From: Christian Huitema Date: Mon, 1 Apr 2024 16:05:35 -0700 Subject: [PATCH 2/4] fix typo and warning --- picoquictest/openssl_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/picoquictest/openssl_test.c b/picoquictest/openssl_test.c index e9a3a9b59..56af70930 100644 --- a/picoquictest/openssl_test.c +++ b/picoquictest/openssl_test.c @@ -106,7 +106,7 @@ int openssl_cert_test() openssl_cert_test_one(TEST_CERT3, 2) != 0) { ret = -1; } - return 0; + return ret; } #endif /* !PTLS_WITHOUT_OPENSSL */ \ No newline at end of file From 4b55c5980d99d632570e9bf0e9fca3088b638c6c Mon Sep 17 00:00:00 2001 From: Christian Huitema Date: Mon, 1 Apr 2024 16:25:37 -0700 Subject: [PATCH 3/4] Fix int/size_t issue --- picoquictest/stream0_frame_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/picoquictest/stream0_frame_test.c b/picoquictest/stream0_frame_test.c index a9414c627..af48751be 100644 --- a/picoquictest/stream0_frame_test.c +++ b/picoquictest/stream0_frame_test.c @@ -997,7 +997,7 @@ int provide_stream_buffer_test_one(uint64_t stream_id, uint64_t stream_offset, s if (length > 0) { /* set test data value */ - for (int i = 0; i < length; i++) { + for (size_t i = 0; i < length; i++) { test_data[i] = (uint8_t)(i ^ 0x7f); } /* Fill the data space */ From 72a1a6cdb7a633c9803b1f475fb7985247c03ca5 Mon Sep 17 00:00:00 2001 From: Christian Huitema Date: Mon, 1 Apr 2024 19:03:51 -0700 Subject: [PATCH 4/4] Fix PN encryption for server busy packet --- picoquic/packet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/picoquic/packet.c b/picoquic/packet.c index d501ae4de..13249e059 100644 --- a/picoquic/packet.c +++ b/picoquic/packet.c @@ -1351,7 +1351,7 @@ int picoquic_queue_busy_packet( payload_length = picoquic_aead_encrypt_generic(bytes + header_length, payload, sizeof(payload), 0, bytes, header_length, aead_ctx); /* protect the PN */ - picoquic_protect_packet_header(bytes, pn_offset, 0x1F, pn_enc_ctx); + picoquic_protect_packet_header(bytes, pn_offset, 0x0F, pn_enc_ctx); /* Fill up control fields */ sp->length = byte_index + payload_length; sp->ptype = picoquic_packet_initial;