Race causing request corruption/data loss #782
Description
This issue was first caught by this sometimes-failing test:
xUnit.net Runner (64-bit win10-x64)
Discovering: Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
Discovered: Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
Starting: Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
Microsoft.AspNetCore.Server.Kestrel.FunctionalTests.RequestTests.LargeMultipartUpload [FAIL]
System.Threading.Tasks.TaskCanceledException : A task was canceled.
Stack Trace:
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
C:\Users\shalter\dev\Universe\KestrelHttpServer\test\Microsoft.AspNetCore.Server.Kestrel.FunctionalTests\RequestTests.cs(115,0): at Microsoft.AspNetCore.Server.Kestrel.FunctionalTests.RequestTests.<LargeMultipartUpload>d__1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
Basically what's happening here is that the final call to MessageBody.ForContentLength.ReadAsyncImplementation stalls because even though the client sent the full request body, SocketInputExtensions.ReadAsync did not return it all throughout the course of the request. When the HttpClient eventually reset the connection after hitting the default 100 second timeout, ReadAsyncImplementation would incorrectly throw a BadHttpRequestException: Unexpected end of request content.
Ultimately the reason SocketInputExtensions.ReadAsync lost some request data was due to a race condition in MemoryPoolIterator.CopyTo. In between calculation following and checking if block.Next == null, request data was both written after block.End and more data was then added into block.Next. Here is a fix to CopyTo that we ultimately probably aren't going to take because we would have to also fix Seek, Peek, Skip, Take and possibly other not yet considered methods.
--- a/src/Microsoft.AspNetCore.Server.Kestrel/Infrastructure/MemoryPoolIterator.cs
+++ b/src/Microsoft.AspNetCore.Server.Kestrel/Infrastructure/MemoryPoolIterator.cs
@@ -679,6 +679,11 @@ public MemoryPoolIterator CopyTo(byte[] array, int offset, int count, out int ac
var remaining = count;
while (true)
{
+ // Determine if we might attempt to copy data from block.Next before
+ // calculating "following" so we don't risk skipping data that could
+ // be added after block.End when we decide to copy from block.Next.
+ // block.End will always be advanced before block.Next is set.
+ var isLastBlock = block.Next == null;
var following = block.End - index;
if (remaining <= following)
{
actual = count;
if (array != null)
{
Buffer.BlockCopy(block.Array, index, array, offset, remaining);
}
return new MemoryPoolIterator(block, index + remaining);
}
- else if (block.Next == null)
+ else if (isLastBlock)
{
actual = count - remaining + following;
if (array != null)
Instead we are likely going to stop reading new request data into "unoccupied tail-space".