Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ArgumentOutOfRangeException in ViewBufferTextWriter with latest servicing release #31299

Closed
sebastienros opened this issue Mar 26, 2021 · 10 comments · Fixed by #32277
Closed
Assignees
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed feature-mvc-razor-views Features related to the Razor view engine for Razor pages and MVC views
Milestone

Comments

@sebastienros
Copy link
Member

This line
https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.ViewFeatures/src/Buffers/ViewBufferTextWriter.cs#L113

throws when invoked with index: 0, count: 0 which is the case when using the new html encoder TextWriterExtensions
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Encodings.Web/src/System/IO/TextWriterExtensions.cs#L33

when invoking this method with a string that starts with a chars that needs to be encoded:
https://github.com/dotnet/runtime/blob/1a35f1db908f96f2bddd9d23c780c515d14f16e0/src/libraries/System.Text.Encodings.Web/src/System/Text/Encodings/Web/TextEncoder.cs#L204

I have multiple reports of Orchard users with applications on Azure that stopped working when the release was pushed.
OrchardCMS/OrchardCore#8887

@pranavkm
Copy link
Contributor

FYI @GrabYourPitchforks

@pranavkm pranavkm added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 26, 2021
@GrabYourPitchforks
Copy link
Member

I'm aware. Looks like our March servicing release exposed this latent bug in MVC. Will spin up a conversation with shiproom about how we should address this.

@GrabYourPitchforks
Copy link
Member

BTW, I suggest that when you fix this, you also audit your existing TextWriter and Stream subclasses. The default implementation of virtual methods like Write(ROS<>) might not be as efficient as you'd expect. By overriding these methods and providing optimized implementations, you may realize a small perf boost.

@mkArtakMSFT mkArtakMSFT added bug This issue describes a behavior which is not expected - a bug. Servicing-consider Shiproom approval is required for the issue labels Mar 29, 2021
@mkArtakMSFT mkArtakMSFT added this to the Next sprint planning milestone Mar 29, 2021
@ghost
Copy link

ghost commented Mar 29, 2021

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@mkArtakMSFT mkArtakMSFT removed the Servicing-consider Shiproom approval is required for the issue label Apr 1, 2021
@javiercn javiercn added the feature-mvc-razor-views Features related to the Razor view engine for Razor pages and MVC views label Apr 18, 2021
@mkArtakMSFT mkArtakMSFT added this to 6.0-preview5 (16th May) in ASP.NET Core Blazor & MVC 6.0 Apr 20, 2021
@TanayParikh TanayParikh moved this from 6.0-preview5 (16th May) to In Progress in ASP.NET Core Blazor & MVC 6.0 Apr 21, 2021
@TanayParikh TanayParikh moved this from In Progress to 6.0-preview5 (16th May) in ASP.NET Core Blazor & MVC 6.0 Apr 22, 2021
@TanayParikh TanayParikh moved this from 6.0-preview5 (16th May) to In Progress in ASP.NET Core Blazor & MVC 6.0 Apr 29, 2021
ASP.NET Core Blazor & MVC 6.0 automation moved this from In Progress to Done Apr 29, 2021
@ghost ghost added Done This issue has been fixed and removed Working labels Apr 29, 2021
@Lizzy-Gallagher
Copy link

@TanayParikh @mkArtakMSFT - Is there an estimate of when this fix will go out?

Some context:

We have spot fixes to unblock updates, but would be very excited to have this reliably fixed.

@TanayParikh
Copy link
Contributor

This was fixed via #32277 & will be part of the .net 6 Preview 5 release.

@sebastienros
Copy link
Member Author

@Lizzy-Gallagher this is scheduled for 6.0. Can you provide some code that shows how you are hitting this issue? Orchard is using a custom view engine, and we couldn't find a way to trigger the behavior otherwise.

@Lizzy-Gallagher
Copy link

Lizzy-Gallagher commented May 5, 2021

Thanks for the quick response @sebastienros and @TanayParikh, I have put together a minimal repro in .NET Core 3.1:
https://github.com/Lizzy-Gallagher/ViewBufferTextWriterBugRepro

Steps to repro

  1. Clone the repo
  2. In VS, use the ViewBufferTextWriterBugRepro launch profile
  3. Click the "Fail" link to see the ArgumentOutOfRangeException

Gif of the repro:

repro

Fail.cshtml:

@{ Html.HtmlHelpers().CreateRepro(new StringHtmlContent("¿Están bien evaluados los clientes del grupo experimental y de control?")); }

Pass.cshtml:

@{ Html.HtmlHelpers().CreateRepro(new StringHtmlContent("Están bien evaluados los clientes del grupo experimental y de control?")); }

To be honest, I am not positive of the link between this error and System.Text.Encodings.Web update we did. This is mostly because I don't completely understand how this .NET Core 3.1 app gets its copy of these classes. Our apps hitting this in production get it from installed NuGet packages as they are .NET Framework. Perhaps this is what you meant by this being a "latent bug in MVC"?

I don't fully understand the dependency tree here, but we would be very interested in a fix that is not just in .NET 6 as we have .NET Framework apps affected by this. 😬

@sebastienros
Copy link
Member Author

Here is the interesting part

public void CreateRepro(IHtmlContent text)
{
	// In production code, we are doing something more complicated with this
	// ViewContext.Writer and would like to continue using it if possible.
	var writer = this._htmlHelper.ViewContext.Writer;
	
	var headerTextDiv = new TagBuilder("div");
	writer.Write(headerTextDiv.RenderStartTag());
	
	headerTextDiv.InnerHtml.AppendHtml(text);
	headerTextDiv.WriteTo(writer, HtmlEncoder.Default);

	writer.Write(headerTextDiv.RenderEndTag());

	// NOTE: This issue is only present when the text is inside
	// a TagBuilder. The following does not throw the ArgumentOutOfRangeException:
	// writer.Write(text);
}

Thanks. As a mitigation you can deploy the previous .NET version as self-contained to prevent the system from using a more recent version, until this is fixed, probably in next service release (though I don't want to promise anything).

@sebastienros
Copy link
Member Author

sebastienros commented May 5, 2021

Another mitigation that I haven't tried but might just work as well is to use a zero-width space (http://www.unicode-symbol.com/u/200B.html) as the first char, since the issue only occurs when the first char needs to be encoded, hoping it doesn't have to.

@TanayParikh TanayParikh mentioned this issue May 6, 2021
10 tasks
@TanayParikh TanayParikh mentioned this issue May 6, 2021
10 tasks
mkArtakMSFT pushed a commit that referenced this issue May 9, 2021
## Description
Update guard logic to permit writing 0 length buffers. This can happen in certain globalized scenarios (ex. `¿` with Portugese) where the first char needs to be encoded.

Patch #31299 for 5.0 by cherry-picking the #32277 squash commit.


## Customer Impact
Writing 0-length char buffers leads to an ArgumentOutOfRangeException. This may occur in a globalization context with certain chars such as `¿`.

#31299 (comment)

Also impacts Orchard: #31299 (comment)


## Regression?
- [ ] Yes
- [x] No

[If yes, specify the version the behavior has regressed from]

## Risk
- [ ] High
- [ ] Medium
- [x] Low

Low risk as we've just updated the logic slightly to permit 0 length buffers.

## Verification
- [x] Manual (required)
- [x] Automated

## Packaging changes reviewed?
- [ ] Yes
- [ ] No
- [x] N/A


Addresses #31299
mkArtakMSFT pushed a commit that referenced this issue May 10, 2021
## Description
Update guard logic to permit writing 0 length buffers. This can happen in certain globalized scenarios (ex. `¿` with Portugese) where the first char needs to be encoded.

Patch #31299 for 3.1 by cherry-picking the #32277 squash commit.


## Customer Impact
Writing 0-length char buffers leads to an ArgumentOutOfRangeException. This may occur in a globalization context with certain chars such as `¿`.

#31299 (comment)

Also impacts Orchard: #31299 (comment)


## Regression?
- [ ] Yes
- [x] No

[If yes, specify the version the behavior has regressed from]

## Risk
- [ ] High
- [ ] Medium
- [x] Low

Low risk as we've just updated the logic slightly to permit 0 length buffers.

## Verification
- [x] Manual (required)
- [x] Automated

## Packaging changes reviewed?
- [ ] Yes
- [ ] No
- [x] N/A


Addresses #31299
@dotnet dotnet locked as resolved and limited conversation to collaborators Jun 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed feature-mvc-razor-views Features related to the Razor view engine for Razor pages and MVC views
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

7 participants