Skip to content

Remove quotes from MultipartReader boundary #41308

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 22, 2022
Merged

Remove quotes from MultipartReader boundary #41308

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
74754ba
Added unit test that accepts a Boundary with quotes to make sure no e…
aclark09 Apr 21, 2022
344667d
Remove quotes from incoming boundary in the MultipartReader constructor
aclark09 Apr 21, 2022
293d34b
Organize code by moving boundary variable cleanup to line before it's…
aclark09 Apr 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/Http/WebUtilities/src/MultipartReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.Extensions.Primitives;
using Microsoft.Net.Http.Headers;

namespace Microsoft.AspNetCore.WebUtilities;

Expand Down Expand Up @@ -51,6 +52,8 @@ public MultipartReader(string boundary, Stream stream, int bufferSize)
throw new ArgumentNullException(nameof(boundary));
}

boundary = HeaderUtilities.RemoveQuotes(new StringSegment(boundary)).ToString();
Copy link
Member

@Tratcher Tratcher Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Organization: Move this down to where _boundary is being initialized.

Copy link
Contributor Author

@aclark09 aclark09 Apr 21, 2022

Choose a reason for hiding this comment

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

So just put it right above that line? Yeah I can do that.

Copy link
Member

@davidfowl davidfowl Apr 22, 2022

Choose a reason for hiding this comment

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

Does this allocate when there's nothing to do?

Copy link
Contributor Author

@aclark09 aclark09 Apr 22, 2022

Choose a reason for hiding this comment

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

Are you talking about that fact that RemoveQuotes is called before all of the null checks? I just pushed up a change so that it's now being called right before the boundary variable is actually used, so it shouldn't be allocated unless it's used now.

Copy link
Member

@Tratcher Tratcher Apr 22, 2022

Choose a reason for hiding this comment

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

It doesn't allocate.

aspnetcore/src/Http/Headers/src/HeaderUtilities.cs

Lines 570 to 574 in bcdb13d

if (IsQuoted(input))
{
input = input.Subsegment(1, input.Length - 2);
}
return input;

Copy link

@rpnetcoder rpnetcoder Apr 26, 2022

Choose a reason for hiding this comment

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

Have you checked if there is no string allocation when calling ToString() when double quotes are not present in an input boundary? What about checking if there are double quotes first and only after that to remove them and call ToString()?

Copy link
Member

@Tratcher Tratcher Apr 26, 2022

Choose a reason for hiding this comment

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

I checked, there's no allocation since this ends up returning the whole original string.
https://github.com/dotnet/runtime/blob/2ea67f1f42e8170676c9e9591a974e39513390db/src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs#L661
https://github.com/dotnet/runtime/blob/2ea67f1f42e8170676c9e9591a974e39513390db/src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs#L81
https://github.com/dotnet/runtime/blob/215b39abf947da7a40b0cb137eab4bceb24ad3e3/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs#L1831-L1834

Copy link

@rpnetcoder rpnetcoder Apr 27, 2022

Choose a reason for hiding this comment

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

Thanks for the clarification


if (stream == null)
{
throw new ArgumentNullException(nameof(stream));
Expand Down
12 changes: 12 additions & 0 deletions src/Http/WebUtilities/test/MultipartReaderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ namespace Microsoft.AspNetCore.WebUtilities;
public class MultipartReaderTests
{
private const string Boundary = "9051914041544843365972754266";
private const string BoundaryWithQuotes = @"""9051914041544843365972754266""";
// Note that CRLF (\r\n) is required. You can't use multi-line C# strings here because the line breaks on Linux are just LF.
private const string OnePartBody =
"--9051914041544843365972754266\r\n" +
Expand Down Expand Up @@ -377,4 +378,15 @@ public async Task MultipartReader_ReadInvalidUtf8SurrogateHeader_ReplacementChar

Assert.Null(await reader.ReadNextSectionAsync());
}

// MultiPartReader should strip any quotes from the boundary passed in instead of throwing an exception
[Fact]
public async Task MultipartReader_StripQuotesFromBoundary()
{
var stream = MakeStream(OnePartBody);
var reader = new MultipartReader(BoundaryWithQuotes, stream);

var section = await reader.ReadNextSectionAsync();
Assert.NotNull(section);
}
}