Handle bad behavior by app in reliable shutdown path. (#3900)

* propose changes to flush and reliable path * remove send flags and reformat header * update CLOG --------- Co-authored-by: Jack He <[email protected]>
microsoft · Oct 10, 2023 · f54cf5a · f54cf5a
1 parent 5b005b2
commit f54cf5a
Show file tree

Hide file tree

Showing 4 changed files with 111 additions and 64 deletions.
diff --git a/src/core/stream_send.c b/src/core/stream_send.c
@@ -47,6 +47,16 @@ QuicStreamCompleteSendRequest(
     _In_ BOOLEAN PreviouslyPosted
     );
 
+//
+// Enqueues a SendRequest from the temporary queue to the actual queue.
+//
+_IRQL_requires_max_(PASSIVE_LEVEL)
+void
+QuicStreamEnqueueSendRequest(
+    _In_ QUIC_STREAM* Stream,
+    _Inout_ QUIC_SEND_REQUEST* SendRequest
+    );
+
 #if DEBUG
 
 _IRQL_requires_max_(PASSIVE_LEVEL)
@@ -249,6 +259,25 @@ QuicStreamSendShutdown(
         }
         Stream->Flags.LocalCloseResetReliable = TRUE;
         Stream->SendShutdownErrorCode = ErrorCode;
+
+        while (ApiSendRequests != NULL) {
+            //
+            // App queued some data around the same time it called shutdown. Bad App!
+            // Enqueue all the stuff in ApiSendRequests to SendRequests.
+            //
+            QUIC_SEND_REQUEST* SendRequest = ApiSendRequests;
+            ApiSendRequests = ApiSendRequests->Next;
+            SendRequest->Next = NULL;
+            QuicStreamEnqueueSendRequest(Stream, SendRequest);
+
+            if (Stream->Connection->Settings.SendBufferingEnabled) {
+                QuicSendBufferFill(Stream->Connection);
+            }
+
+            CXPLAT_DBG_ASSERT(Stream->SendRequests != NULL);
+            QuicStreamSendDumpState(Stream);
+        }
+
         //
         // Queue up a RESET RELIABLE STREAM frame to be sent. We will clear up any flags later.
         //
@@ -514,6 +543,63 @@ QuicStreamSendBufferRequest(
     return QUIC_STATUS_SUCCESS;
 }
 
+_IRQL_requires_max_(PASSIVE_LEVEL)
+void
+QuicStreamEnqueueSendRequest(
+    _In_ QUIC_STREAM* Stream,
+    _Inout_ QUIC_SEND_REQUEST* SendRequest
+    )
+{
+     Stream->Connection->SendBuffer.PostedBytes += SendRequest->TotalLength;
+
+    //
+    // Queue up the send request.
+    //
+
+    QuicStreamRemoveOutFlowBlockedReason(Stream, QUIC_FLOW_BLOCKED_APP);
+
+    SendRequest->StreamOffset = Stream->QueuedSendOffset;
+    Stream->QueuedSendOffset += SendRequest->TotalLength;
+
+    if (SendRequest->Flags & QUIC_SEND_FLAG_ALLOW_0_RTT &&
+        Stream->Queued0Rtt == SendRequest->StreamOffset) {
+        Stream->Queued0Rtt = Stream->QueuedSendOffset;
+    }
+
+    //
+    // The bookmarks are set to NULL once the entire request queue is
+    // consumed. So if a bookmark is NULL here, we should set it to
+    // point to the new request at the end of the queue, to prevent
+    // a subsequent search over the entire queue in the code that
+    // uses the bookmark.
+    //
+    if (Stream->SendBookmark == NULL) {
+        Stream->SendBookmark = SendRequest;
+    }
+    if (Stream->SendBufferBookmark == NULL) {
+        //
+        // If we have no SendBufferBookmark, that must mean we have no
+        // unbuffered send requests queued currently.
+        //
+        CXPLAT_DBG_ASSERT(
+            Stream->SendRequests == NULL ||
+            !!(Stream->SendRequests->Flags & QUIC_SEND_FLAG_BUFFERED));
+        Stream->SendBufferBookmark = SendRequest;
+    }
+
+    *Stream->SendRequestsTail = SendRequest;
+    Stream->SendRequestsTail = &SendRequest->Next;
+
+    QuicTraceLogStreamVerbose(
+        SendQueued,
+        Stream,
+        "Send Request [%p] queued with %llu bytes at offset %llu (flags 0x%x)",
+        SendRequest,
+        SendRequest->TotalLength,
+        SendRequest->StreamOffset,
+        SendRequest->Flags);
+}
+
 _IRQL_requires_max_(PASSIVE_LEVEL)
 void
 QuicStreamSendFlush(
@@ -545,54 +631,7 @@ QuicStreamSendFlush(
             continue;
         }
 
-        Stream->Connection->SendBuffer.PostedBytes += SendRequest->TotalLength;
-
-        //
-        // Queue up the send request.
-        //
-
-        QuicStreamRemoveOutFlowBlockedReason(Stream, QUIC_FLOW_BLOCKED_APP);
-
-        SendRequest->StreamOffset = Stream->QueuedSendOffset;
-        Stream->QueuedSendOffset += SendRequest->TotalLength;
-
-        if (SendRequest->Flags & QUIC_SEND_FLAG_ALLOW_0_RTT &&
-            Stream->Queued0Rtt == SendRequest->StreamOffset) {
-            Stream->Queued0Rtt = Stream->QueuedSendOffset;
-        }
-
-        //
-        // The bookmarks are set to NULL once the entire request queue is
-        // consumed. So if a bookmark is NULL here, we should set it to
-        // point to the new request at the end of the queue, to prevent
-        // a subsequent search over the entire queue in the code that
-        // uses the bookmark.
-        //
-        if (Stream->SendBookmark == NULL) {
-            Stream->SendBookmark = SendRequest;
-        }
-        if (Stream->SendBufferBookmark == NULL) {
-            //
-            // If we have no SendBufferBookmark, that must mean we have no
-            // unbuffered send requests queued currently.
-            //
-            CXPLAT_DBG_ASSERT(
-                Stream->SendRequests == NULL ||
-                !!(Stream->SendRequests->Flags & QUIC_SEND_FLAG_BUFFERED));
-            Stream->SendBufferBookmark = SendRequest;
-        }
-
-        *Stream->SendRequestsTail = SendRequest;
-        Stream->SendRequestsTail = &SendRequest->Next;
-
-        QuicTraceLogStreamVerbose(
-            SendQueued,
-            Stream,
-            "Send Request [%p] queued with %llu bytes at offset %llu (flags 0x%x)",
-            SendRequest,
-            SendRequest->TotalLength,
-            SendRequest->StreamOffset,
-            SendRequest->Flags);
+        QuicStreamEnqueueSendRequest(Stream, SendRequest);
 
         if (SendRequest->Flags & QUIC_SEND_FLAG_START && !Stream->Flags.Started) {
             //

diff --git a/src/generated/linux/stream_send.c.clog.h b/src/generated/linux/stream_send.c.clog.h
@@ -87,13 +87,13 @@ tracepoint(CLOG_STREAM_SEND_C, IndicateSendComplete , arg1, arg3);\
 // Decoder Ring for SendQueued
 // [strm][%p] Send Request [%p] queued with %llu bytes at offset %llu (flags 0x%x)
 // QuicTraceLogStreamVerbose(
-            SendQueued,
-            Stream,
-            "Send Request [%p] queued with %llu bytes at offset %llu (flags 0x%x)",
-            SendRequest,
-            SendRequest->TotalLength,
-            SendRequest->StreamOffset,
-            SendRequest->Flags);
+        SendQueued,
+        Stream,
+        "Send Request [%p] queued with %llu bytes at offset %llu (flags 0x%x)",
+        SendRequest,
+        SendRequest->TotalLength,
+        SendRequest->StreamOffset,
+        SendRequest->Flags);
 // arg1 = arg1 = Stream = arg1
 // arg3 = arg3 = SendRequest = arg3
 // arg4 = arg4 = SendRequest->TotalLength = arg4

diff --git a/src/generated/linux/stream_send.c.clog.h.lttng.h b/src/generated/linux/stream_send.c.clog.h.lttng.h
@@ -70,13 +70,13 @@ TRACEPOINT_EVENT(CLOG_STREAM_SEND_C, IndicateSendComplete,
 // Decoder Ring for SendQueued
 // [strm][%p] Send Request [%p] queued with %llu bytes at offset %llu (flags 0x%x)
 // QuicTraceLogStreamVerbose(
-            SendQueued,
-            Stream,
-            "Send Request [%p] queued with %llu bytes at offset %llu (flags 0x%x)",
-            SendRequest,
-            SendRequest->TotalLength,
-            SendRequest->StreamOffset,
-            SendRequest->Flags);
+        SendQueued,
+        Stream,
+        "Send Request [%p] queued with %llu bytes at offset %llu (flags 0x%x)",
+        SendRequest,
+        SendRequest->TotalLength,
+        SendRequest->StreamOffset,
+        SendRequest->Flags);
 // arg1 = arg1 = Stream = arg1
 // arg3 = arg3 = SendRequest = arg3
 // arg4 = arg4 = SendRequest->TotalLength = arg4

diff --git a/src/test/lib/DataTest.cpp b/src/test/lib/DataTest.cpp
@@ -3455,6 +3455,7 @@ QuicTestStreamReliableResetMultipleSends(
     SendContext send3 = {FALSE, 0};
     SendContext send4 = {FALSE, 0};
     SendContext send5 = {FALSE, 0};
+    SendContext send6 = {FALSE, 0};
     TEST_QUIC_SUCCEEDED(Stream.Send(&SendBuffer, 1, QUIC_SEND_FLAG_DELAY_SEND, &send1));
     TEST_QUIC_SUCCEEDED(Stream.Send(&SendBuffer, 1, QUIC_SEND_FLAG_DELAY_SEND, &send2));
     TEST_QUIC_SUCCEEDED(Stream.Send(&SendBuffer, 1, QUIC_SEND_FLAG_DELAY_SEND, &send3));
@@ -3463,8 +3464,15 @@ QuicTestStreamReliableResetMultipleSends(
     TEST_QUIC_SUCCEEDED(Stream.SetReliableOffset(RELIABLE_SIZE_MULTI_SENDS));
 
     const QUIC_UINT62 AbortShutdownErrorCode = 0x696969696969;
-    TEST_QUIC_SUCCEEDED(Stream.Shutdown(AbortShutdownErrorCode)); // Queues up a shutdown operation.
-    // Should behave similar to QUIC_STREAM_SHUTDOWN_FLAG_GRACEFUL, with some restrictions.
+    TEST_QUIC_SUCCEEDED(Stream.Shutdown(AbortShutdownErrorCode));
+
+    //
+    // An app shouldn't be sending after it just called shutdown, but we want to make sure this
+    // doesn't cause a memory leak or other problems.
+    //
+    Stream.Send(&SendBuffer, 1, QUIC_SEND_FLAG_NONE, &send6); // This may or may not succeed (race condition).
+
+
     TEST_TRUE(Context.ClientStreamShutdownComplete.WaitTimeout(TestWaitTimeout));
     TEST_TRUE(Context.ServerStreamShutdownComplete.WaitTimeout(TestWaitTimeout));
     TEST_TRUE(Context.ReceivedBufferSize >= RELIABLE_SIZE_MULTI_SENDS);