-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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.
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
Conversation
| throw new ArgumentNullException(nameof(boundary)); | ||
| } | ||
|
|
||
| boundary = HeaderUtilities.RemoveQuotes(new StringSegment(boundary)).ToString(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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()?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification
|
Thanks @aclark09 |
Remove quotes from MultipartReader boundary
Description
When a request is submitted by HttpClient via MultipartFormDataContent, the boundary value in multipart/formdata request header is enclosed with double quotes. In this case, when using MediaTypeHeaderValue.TryParse to parse the header, the MediaTypeHeaderValue will include those double quotes as boundary when parsing the header. This leads to an exception being thrown.
The solution for now is to remove quotes from the boundary passed in to the MultipartReader constructor. I also added a unit test that checks to see if a boundary with double quotes works.
Fixes #41237