Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

[3.1] Don't call AdvanceTo(default) in PipeReader.CopyToAsync #42837

Merged
merged 3 commits into from
Feb 19, 2020

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Jan 23, 2020

3.1 backport of dotnet/runtime#1437.

Fixes dotnet/runtime#1436

Description

When the default PipeReader.CopyToAsync(destination) implementation 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.

Customer Impact

If a customer calls CopyToAsync with a stream that can throw, the original Stream.WriteAsync() exception is hidden by an exception from PipeReader.AdvanceTo(). 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

Regression?

Technically, no. But in 3.0, Kestrel started using a PipeReader to back its request body Streams which makes this issue more prominent.

Packaging reviewed?

Not yet, but I've followed the guide at https://github.com/dotnet/runtime/blob/0515878d1eb4fb544ae0219c436c299746fdb2b4/docs/project/library-servicing.md.

Risk

Extremely minimal. This PR is initializing the consumed SequencePosition to the safe value of ReadResult.Buffer.Start (indicating nothing was consumed) instead of default (effectively null). The default PipeReader.AdvanceTo implementation treats these the same.

@danmoseley
Copy link
Member

@halter73 please update top post to include the template. Here's an example: #42809 (comment)

Also I believe you will need to add some package authoring to this change since it's not part of netcoreapp. @Anipik can point you at instructions for doing that.

@Anipik
Copy link

Anipik commented Jan 28, 2020

@halter73 https://github.com/dotnet/runtime/blob/0515878d1eb4fb544ae0219c436c299746fdb2b4/docs/project/library-servicing.md
this doc contains guidelines for making servicing changes.
Let me know if you need anything.

@yshahin
Copy link

yshahin commented Feb 4, 2020

@halter73 any timeline when this patch will reach me?
Is there a way to apply it locally until this gets released?
thanks

@wtgodbe wtgodbe added the Servicing-consider Issue for next servicing release review label Feb 18, 2020
@wtgodbe
Copy link
Member

wtgodbe commented Feb 18, 2020

@halter73 is this for March (3.1.3)? If so please fixup conflicts & get this merged by EOD, or push out to April

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 18, 2020
@leecow leecow added this to the 3.1.3 milestone Feb 18, 2020
Copy link
Member

@wtgodbe wtgodbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Packaging changes look good, but I'm a little rusty - @ericstj please confirm

@halter73
Copy link
Member Author

@ericstj System.IO.Pipelines is not currently in the Microsoft.NETCore.App runtime, but it is included in the Microsoft.AspNetCore.App runtime.

Should we make sure the dll produced for the AspNetCore.App runtime doesn't bump the AssemblyVersion from 4.0.2.0 to 4.0.2.1 like we would if it was part of the NETCore.App runtime?

@ericstj
Copy link
Member

ericstj commented Feb 19, 2020

If you did that then an app referencing the fixed package would not get the fix when running on the un-patched Asp.NET shared framework, which you don't want. It's OK for a package to carry a new version.

It is up to ASP.NET shared framework to make sure it doesn't update the reference assembly for this, but does pick up the new implementation. @Pilchie @wtgodbe how do you pick up dependencies when building the ref pack for ASP.NET in servicing?

@Anipik
Copy link

Anipik commented Feb 19, 2020

@ericstj can i go ahead and merge this one ? do we need any code review here ?

@ericstj
Copy link
Member

ericstj commented Feb 19, 2020

This looks ok from corefx side. Just watch what it does in upstream repos: we don't want to ship the higher versioned reference assembly in a new AspNETCore.App ref pack.

@wtgodbe
Copy link
Member

wtgodbe commented Feb 19, 2020

@ericstj we reference the package directly at the latest version, and bundle its ref assembly with the ref pack whenever we ship it (which we won't in 3.1.3, and ideally never again in 3.1)

@wtgodbe
Copy link
Member

wtgodbe commented Feb 19, 2020

I suppose we'd need to either pin to the 4.7.0 version, or use the latest package for the implementation assembly, but the pinned package for the ref assembly? CC @dougbu

@wtgodbe
Copy link
Member

wtgodbe commented Feb 19, 2020

This made me nervous that we might have shipped a higher versioned CoreFx ref assembly with AspNetCore's 3.1.2 ref pack, but looks like we should be ok there - the only CoreFx package we depended on in 3.1.2 with a non-zero Patch version, was System.Text.Json, which isn't in our targeting pack: https://github.com/dotnet/aspnetcore/blob/c3acdcac86dad91c3d3fbc3b93ecc6b7ba494bdc/eng/Version.Details.xml#L368-L370, https://github.com/dotnet/aspnetcore/blob/v3.1.2/eng/SharedFramework.External.props#L54-L72.

However, as far as I can tell, we don't have a policy in place that will prevent us from shipping a versioned CoreFx ref assembly in our ref pack, in the event that CoreFx bumps a package we include in that package, and we're servicing the ref pack. That isn't the case this time around (we're not building the ref pack), so we could just take this as is, as long as we take a hard commitment to make sure the issue is resolved on our end before 3.1.4 (CC @dougbu @JunTaoLuo)

@ericstj
Copy link
Member

ericstj commented Feb 19, 2020

I'll reiterate that I think this change is OK to merge now, the discussion around ASP.NET is something we just need to "watch for" as this flows through.

@wtgodbe
Copy link
Member

wtgodbe commented Feb 19, 2020

dotnet/aspnetcore#19151

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants