PipeReader.CopyToAsync(destination) calls AdvanceTo(default) when destination.WriteAsync throws

[halter73]

When the default PipeReader.CopyToAsync(destination) implemention is called with a Stream that throws from WriteAsync(), it will call PipeReader.AdvanceTo(consumed) with the default SequencePosition instead of ReadResult.Buffer.Start as expected. With a custom PipeReader, this can result in the PipeReader being left in an unusable state.

Here's an example that calls ASP.NET Core 3.1's HttpContext.Request.Body.CopyToAsync() method which uses the default PipeReader.CopyToAsync() implementation under the covers:

var throwingStream = new ThrowingStream();

try
{
    await context.Request.Body.CopyToAsync(throwingStream);
}
catch (Exception ex)
{
    Console.WriteLine("CopyToAsync Ex: {0}", ex);
}

CopyToAsync Ex: System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values. (Parameter 'position')
   at System.ThrowHelper.ThrowArgumentOutOfRangeException_PositionOutOfRange()
   at System.Buffers.ReadOnlySequence`1.BoundsCheck(UInt32 sliceStartIndex, Object sliceStartObject, UInt32 sliceEndIndex, Object sliceEndObject)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.Http1ContentLengthMessageBody.AdvanceTo(SequencePosition consumed, SequencePosition examined) in C:\dev\aspnet\AspNetCore\src\Servers\Kestrel\Core\src\Internal\Http\Http1ContentLengthMessageBody.cs:line 217
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.Http1ContentLengthMessageBody.AdvanceTo(SequencePosition consumed) in C:\dev\aspnet\AspNetCore\src\Servers\Kestrel\Core\src\Internal\Http\Http1ContentLengthMessageBody.cs:line 202   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpRequestPipeReader.AdvanceTo(SequencePosition consumed) in C:\dev\aspnet\AspNetCore\src\Servers\Kestrel\Core\src\Internal\Http\HttpRequestPipeReader.cs:line 31
   at System.IO.Pipelines.PipeReader.CopyToAsyncCore[TStream](TStream destination, Func`4 writeAsync, CancellationToken cancellationToken)
   at SampleApp.Startup.<>c__DisplayClass0_0.<<Configure>b__1>d.MoveNext() in C:\dev\aspnet\AspNetCore\src\Servers\Kestrel\samples\SampleApp\Startup.cs:line 49

There's already a test that verifies that a throwing destination stream passed to PipeReader.CopyToAsync() doesn't leave the PipeReader in a broken state, but it passes because the default PipeReader.AdvanceTo(consumed, examined) implementation will effectively ignore default consumed and examined SequencePositions (treating it as if nothing new was consumed or examined) and move out of the reading state.

runtime/src/libraries/System.IO.Pipelines/tests/PipeReaderCopyToAsyncTests.cs

Lines 247 to 267 in 339eef5

	[Fact]
	public async Task ThrowingFromStreamDoesNotLeavePipeReaderInBrokenState()
	{
	var pipe = new Pipe(s_testOptions);
	var stream = new ThrowingStream();
	Task task = pipe.Reader.CopyToAsync(stream);
	pipe.Writer.WriteEmpty(10);
	await pipe.Writer.FlushAsync();
	await Assert.ThrowsAsync<InvalidOperationException>(() => task);
	pipe.Writer.WriteEmpty(10);
	await pipe.Writer.FlushAsync();
	pipe.Writer.Complete();
	ReadResult result = await pipe.Reader.ReadAsync();
	Assert.True(result.IsCompleted);
	Assert.Equal(20, result.Buffer.Length);
	pipe.Reader.Complete();
	}

In Kestrel's Http1ContentLengthMessageBody implementation, it tries to use the consumed SequencePosition to create the ReadResult to be returned from the next call to PipeReader.ReadAsync leading to the ArgumentOutOfRangeException seen above.

https://github.com/dotnet/aspnetcore/blob/c17ce436b8703470231e7979cead042f5682c3f5/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs#L214-L217

While it's possible for custom PipeReaders to try to treat default SequencePositions the same as the default implementation and effectively ignore them, it shouldn't be necessary. It's still better to have the default PipeReader.CopyToAsync(destination) implementation to pass ReadResult.Buffer.Start to PipeReader.AdvanceTo() when call to destination.WriteAsync throws.

[halter73]

While not exactly the same thing. I found this while investigating dotnet/aspnetcore#18138.

[davidfowl]

Seems patch worthy @danmosemsft ?

[yshahin]

@danmosemsft, yes please have this as a patch. It is causing 500 in our service.
The sooner it is released the better.

[danmoseley]

Sounds like it makes sense if the risk is acceptable. @halter73 I believe you're the owner of this code. Do you support porting?

If so, can you please throw up a PR against release/3.1 (in dotnet/corefx), put the usual template in that PR, and set the "servicing-consider" label?

[halter73]

@danmosemsft I do support porting this. I made this fixn in master, so I suppose I own it. I'm OOF today, but I can port the fix tomorrow.

[danmoseley]

@halter73 I was just going by https://github.com/dotnet/runtime/blob/master/docs/area-owners.md

Sounds good.

[halter73]

@danmosemsft Here's the patch PR: dotnet/corefx#42837

[GrabYourPitchforks]

Closing as fixed in 5.0 (#1437) and 3.1 ( dotnet/corefx#42837)