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

Backport #32277 to 5.0 #32470

Conversation

TanayParikh
Copy link
Contributor

@TanayParikh TanayParikh commented May 6, 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
  • No

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

Risk

  • High
  • Medium
  • Low

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

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

Addresses #31299

@TanayParikh TanayParikh changed the base branch from main to release/5.0 May 6, 2021 17:55
@TanayParikh TanayParikh changed the title Backport https://github.com/dotnet/aspnetcore/pull/32277 to 5.0 Backport #32277 to 5.0 May 6, 2021
@TanayParikh TanayParikh added the Servicing-consider Shiproom approval is required for the issue label May 6, 2021
@ghost
Copy link

ghost commented May 6, 2021

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

@mkArtakMSFT mkArtakMSFT added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 6, 2021
@mkArtakMSFT mkArtakMSFT added this to the 5.0.x milestone May 6, 2021
@mkArtakMSFT mkArtakMSFT added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels May 9, 2021
@mkArtakMSFT
Copy link
Member

This has been approved by Tactics.

Thanks @TanayParikh!

@mkArtakMSFT mkArtakMSFT merged commit b124d50 into release/5.0 May 9, 2021
@mkArtakMSFT mkArtakMSFT deleted the taparik/release5CherryPick80f0a03068065c8609b19d6bfa228accd18ae468 branch May 9, 2021 03:26
@mkArtakMSFT mkArtakMSFT modified the milestones: 5.0.x, 5.0.6 May 11, 2021
@Theoistic
Copy link

So this was suppose to fix this issue??:

ArgumentOutOfRangeException: Specified argument was out of the range of valid values. (Parameter 'index') Microsoft.AspNetCore.Mvc.ViewFeatures.Buffers.ViewBufferTextWriter.Write(char[] buffer, int index, int count) System.IO.TextWriter.Write(ReadOnlySpan<char> buffer) System.IO.TextWriterExtensions.WritePartialString(TextWriter writer, string value, int offset, int count) System.Text.Encodings.Web.TextEncoder.Encode(TextWriter output, string value, int startIndex, int characterCount) Microsoft.AspNetCore.Mvc.Rendering.TagBuilder.AppendAttributes(TextWriter writer, HtmlEncoder encoder) Microsoft.AspNetCore.Mvc.Rendering.TagBuilder.WriteTo(TagBuilder tagBuilder, TextWriter writer, HtmlEncoder encoder, TagRenderMode tagRenderMode)

This last patch broke production, and since this has high-jacked the action setup script on github.
/home/runner/work/_actions/actions/setup-dotnet/v1/externals/install-dotnet.sh --version 5.0.300
Is there like a SUPER fast way to revert this?

@ghost
Copy link

ghost commented Jun 1, 2021

Hi @Theoistic. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@Theoistic
Copy link

@sebastienros .. fyi
I believe the tests for these changes needs to be broader.

@ghost
Copy link

ghost commented Jun 1, 2021

Hi @Theoistic. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@TanayParikh
Copy link
Contributor Author

Thanks for reaching out @Theoistic. Can you please file a new issue with those details, as well as any other information you may have (input arguments to that function, repro steps, etc.).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Servicing-approved Shiproom has approved the issue
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants