Skip to content

Commit

Permalink
Handle bad behavior by app in reliable shutdown path. (#3900)
Browse files Browse the repository at this point in the history 
* propose changes to flush and reliable path

* remove send flags and reformat header

* update CLOG

---------

Co-authored-by: Jack He <jackhe@microsoft.com>
  • Loading branch information
@ProjectsByJackHe
ProjectsByJackHe and ProjectsByJackHe committed Oct 10, 2023
1 parent 5b005b2 commit f54cf5a
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 64 deletions.
135 changes: 87 additions & 48 deletions src/core/stream_send.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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.
//
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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) {
//
Expand Down
14 changes: 7 additions & 7 deletions src/generated/linux/stream_send.c.clog.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 7 additions & 7 deletions src/generated/linux/stream_send.c.clog.h.lttng.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 10 additions & 2 deletions src/test/lib/DataTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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);
Expand Down

0 comments on commit f54cf5a

Please sign in to comment.