Merge pull request #1598 from private-octopus/reset-race-condition

Verify processing of stream control frames after reset

Author: huitema

CMakeLists.txt

@@ -8,7 +8,7 @@ else()
 endif()
 
 project(picoquic
-        VERSION 1.1.16.1
+        VERSION 1.1.16.2
         DESCRIPTION "picoquic library"
         LANGUAGES C CXX)
 

UnitTest1/unittest1.cpp

@@ -160,7 +160,7 @@ namespace UnitTest1
             Assert::AreEqual(ret, 0);
         }
 
-        TEST_METHOD(test_varints)
+        TEST_METHOD(varints)
         {
             int ret = varint_test();
 
@@ -1621,6 +1621,59 @@ namespace UnitTest1
             Assert::AreEqual(ret, 0);
         }
 
+        TEST_METHOD(reset_ack_max)
+        {
+            int ret = reset_ack_max_test();
+
+            Assert::AreEqual(ret, 0);
+        }
+        TEST_METHOD(reset_ack_reset)
+        {
+            int ret = reset_ack_reset_test();
+
+            Assert::AreEqual(ret, 0);
+        }
+
+        TEST_METHOD(reset_extra_max)
+        {
+            int ret = reset_extra_max_test();
+
+            Assert::AreEqual(ret, 0);
+        }
+
+        TEST_METHOD(reset_extra_reset)
+        {
+            int ret = reset_extra_reset_test();
+
+            Assert::AreEqual(ret, 0);
+        }
+        TEST_METHOD(reset_extra_stop)
+        {
+            int ret = reset_extra_stop_test();
+
+            Assert::AreEqual(ret, 0);
+        }
+
+        TEST_METHOD(reset_need_max)
+        {
+            int ret = reset_need_max_test();
+
+            Assert::AreEqual(ret, 0);
+        }
+        TEST_METHOD(reset_need_reset)
+        {
+            int ret = reset_need_reset_test();
+
+            Assert::AreEqual(ret, 0);
+        }
+
+        TEST_METHOD(reset_need_stop)
+        {
+            int ret = reset_need_stop_test();
+
+            Assert::AreEqual(ret, 0);
+        }
+
         TEST_METHOD(ready_to_send)
         {
             int ret = ready_to_send_test();

picoquic/frames.c

@@ -267,8 +267,15 @@ const uint8_t* picoquic_decode_stream_reset_frame(picoquic_cnx_t* cnx, const uin
         picoquic_connection_error(cnx, PICOQUIC_TRANSPORT_FRAME_FORMAT_ERROR,
             picoquic_frame_type_reset_stream);
     } else if ((stream = picoquic_find_or_create_stream(cnx, stream_id, 1)) == NULL) {
-        bytes = NULL;  // error already signaled
-
+        /* Not finding the stream is only an error if the stream
+         * was expected to be present, or created on demand. If the
+         * stream was already created and then deleted, there is no harm.
+         * If the "return NULL" is in a normal scenario, the connection state
+         * will remain "ready" or "almost ready"
+         */
+        if (cnx->cnx_state > picoquic_state_ready) {
+            bytes = NULL;  /* error already signaled */
+        }
     } else if ((stream->fin_received || stream->reset_received) && final_offset != stream->fin_offset) {
         picoquic_connection_error(cnx, PICOQUIC_TRANSPORT_FINAL_OFFSET_ERROR,
             picoquic_frame_type_reset_stream);
@@ -319,8 +326,8 @@ int picoquic_process_ack_of_reset_stream_frame(picoquic_cnx_t * cnx, const uint8
         ret = -1;
     } else {
         *consumed = bytes - byte_first;
-        /* Find the stream */
-        if ((stream = picoquic_find_or_create_stream(cnx, stream_id, 0)) != NULL) {
+        /* Find the stream, if it exists. If it was already deleted, do nothing. */
+        if ((stream = picoquic_find_stream(cnx, stream_id)) != NULL) {
             /* mark reset as acked by peer */
             stream->reset_acked = 1;
             /* Delete stream if closed. */
@@ -347,7 +354,7 @@ int picoquic_check_reset_stream_needs_repeat(picoquic_cnx_t* cnx, const uint8_t*
         /* Internal error -- cannot parse the stored packet */
         ret = -1;
     }
-    else if ((stream = picoquic_find_or_create_stream(cnx, stream_id, 0)) == NULL ||
+    else if ((stream = picoquic_find_stream(cnx, stream_id)) == NULL ||
         stream->reset_acked) {
         *no_need_to_repeat = 1;
     }
@@ -837,6 +844,35 @@ const uint8_t* picoquic_skip_stop_sending_frame(const uint8_t* bytes, const uint
 }
 
 
+int picoquic_check_stop_sending_needs_repeat(picoquic_cnx_t* cnx, const uint8_t* bytes, size_t bytes_size, int* no_need_to_repeat)
+{
+    uint64_t stream_id = 0;
+    uint64_t error_code = 0;
+    const uint8_t* bytes_max = bytes + bytes_size;
+    picoquic_stream_head_t* stream;
+    int ret = 0;
+
+    *no_need_to_repeat = 0;
+
+    if ((bytes = picoquic_frames_varint_decode(bytes+1, bytes_max, &stream_id))  == NULL ||
+        (bytes = picoquic_frames_varint_decode(bytes,   bytes_max, &error_code)) == NULL)
+    {
+        /* If the frame cannot be decoded, do not repeat it */
+        *no_need_to_repeat = 1;
+    }
+    else if ((stream = picoquic_find_stream(cnx, stream_id)) == NULL) {
+        /* If the stream is deleted, no need to repeat this frame. */
+        *no_need_to_repeat = 1;
+    }
+    else if (stream->fin_received || stream->reset_received) {
+        /* No point repeating if the stream is closed by the peer */
+        *no_need_to_repeat = 1;
+    }
+
+    return ret;
+}
+
+
 /*
  * STREAM frames implicitly create a stream and carry stream data.
  */
@@ -3123,6 +3159,9 @@ int picoquic_check_frame_needs_repeat(picoquic_cnx_t* cnx, const uint8_t* bytes,
         case picoquic_frame_type_reset_stream:
             ret = picoquic_check_reset_stream_needs_repeat(cnx, bytes, bytes_max, no_need_to_repeat);
             break;
+        case picoquic_frame_type_stop_sending:
+            ret = picoquic_check_stop_sending_needs_repeat(cnx, bytes, bytes_max, no_need_to_repeat);
+            break;
         default: {
             uint64_t frame_id64;
             *no_need_to_repeat = 0;

picoquic/picoquic.h

@@ -40,7 +40,7 @@
 extern "C" {
 #endif
 
-#define PICOQUIC_VERSION "1.1.16.1"
+#define PICOQUIC_VERSION "1.1.16.2"
 #define PICOQUIC_ERROR_CLASS 0x400
 #define PICOQUIC_ERROR_DUPLICATE (PICOQUIC_ERROR_CLASS + 1)
 #define PICOQUIC_ERROR_AEAD_CHECK (PICOQUIC_ERROR_CLASS + 3)

picoquic/quicctx.c

@@ -539,7 +539,7 @@ static picosplay_node_t* picoquic_registered_token_create(void* value)
 
 static void* picoquic_registered_token_value(picosplay_node_t* node)
 {
-    return (void*)((char*)node - offsetof(struct st_picoquic_registered_token_t, registered_token_node));
+    return (void*)((node == NULL)?NULL:((char*)node - offsetof(struct st_picoquic_registered_token_t, registered_token_node)));
 }
 
 static void picoquic_registered_token_delete(void* tree, picosplay_node_t* node)

picoquic/tls_api.h

@@ -193,6 +193,7 @@ void picoquic_aes128_ecb_free(void* v_aesecb);
 
 void picoquic_aes128_ecb_encrypt(void* v_aesecb, uint8_t* output, const uint8_t* input, size_t len);
 
+void picoquic_tls_api_init();
 void picoquic_tls_api_unload();
 void picoquic_tls_api_reset(uint64_t init_flags);
 

picoquic_t/picoquic_t.c

@@ -273,6 +273,14 @@ static const picoquic_test_def_t test_table[] = {
     { "error_reason", error_reason_test },
     { "idle_server", idle_server_test },
     { "idle_timeout", idle_timeout_test },
+    { "reset_ack_max", reset_ack_max_test },
+    { "reset_ack_reset", reset_ack_reset_test },
+    { "reset_extra_max", reset_extra_max_test },
+    { "reset_extra_reset", reset_extra_reset_test },
+    { "reset_extra_stop", reset_extra_stop_test },
+    { "reset_need_max", reset_need_max_test },
+    { "reset_need_reset", reset_need_reset_test },
+    { "reset_need_stop", reset_need_stop_test },
     { "ready_to_send", ready_to_send_test },
     { "ready_to_skip", ready_to_skip_test },
     { "ready_to_zfin", ready_to_zfin_test },

picoquictest/cleartext_aead_test.c

@@ -329,8 +329,11 @@ int pn_ctr_test()
     uint8_t in_bytes[16];
     uint8_t out_bytes[16];
     uint8_t decoded[16];
+
+    picoquic_tls_api_init();
+
     ptls_aead_algorithm_t* aead = (ptls_aead_algorithm_t*)picoquic_get_aes128gcm_v(0);
-    ptls_cipher_context_t *pn_enc = ptls_cipher_new(aead->ctr_cipher, 1, key);
+    ptls_cipher_context_t *pn_enc = (aead == NULL)?NULL:ptls_cipher_new(aead->ctr_cipher, 1, key);
 
     if (pn_enc == NULL) {
         ret = -1;