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

Microsoft.AspNetCore.WebUtilities.BufferedReadStream - incorrect implementation of Position property's setter #60416

Closed
1 task done
nidaca opened this issue Feb 15, 2025 · 5 comments · Fixed by #60529
Closed
1 task done
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Milestone

Comments

@nidaca
Copy link
Contributor

nidaca commented Feb 15, 2025

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

Hi. By trying to use Microsoft.AspNetCore.WebUtilities.MultipartReader in a way that is directly using the section streams outside the initial reading loop (made through ReadNextSectionAsync), I've come to at least one case (when dealing with more than two section streams) where the conclusion is that the Position property of BufferedReadStream has an incorrect implementation when trying to position inside the internal buffer. I guess it's a pretty invalidating bug, making the whole MultipartReader support unstable / unpredictible / unusable.

To be more exact, these two lines are incorrect, in my opinion:

// Yes, just skip some of the buffered data
_bufferOffset += innerOffset;
_bufferCount -= innerOffset;

Explaination

It should advance in the internal buffer not with the offset between old position and new position (i.e. backward innerOffset), but until that offset / distance, so in the end the new Position will reflect the desired input value when getting the property. That means we should advance _bufferCount (number of bytes left for reading until old position) minus innerOffset (previously mentioned offset).

Expected Behavior

It should have been replaced by these lines:

_bufferOffset += _bufferCount - innerOffset;
_bufferCount = innerOffset;

Steps To Reproduce

Try to use a Microsoft.AspNetCore.WebUtilities.MultipartReader in a request having at least two uploaded form data files and store the section streams for later reading (not some copies of them), it will be impossible to correctly read the second stream because the functionality has unpredictible behavior (see description of the problem).

Exceptions (if any)

In some scenarios it throws: IOException("Unexpected end of Stream, the content may have already been read by another component. ")

.NET Version

No response

Anything else?

It would be nice if this can be fixed in order to not be forced to make copies of large uploaded files. I can contribute with the fix for it (described above), if this would be the easiest and fastest way to push the fix, after checking the validity of the issue. Thank you.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Feb 15, 2025
@nidaca
Copy link
Contributor Author

nidaca commented Feb 19, 2025

No evaluation? Any review / help would be great, seems like a real big problem, easy to solve. Thank you: @Tratcher @pranavkm @BrennanConroy @JamesNK

@BrennanConroy
Copy link
Member

That code is 11 years old and doesn't seem to have any test coverage. I wouldn't be surprised if it had a bug. Calling MultipartReader unusable is a bit of an overexaggeration, since it seems like you're the only one to have hit this issue in the 11+ years it's been out.

Can you provide a PoC that shows the bug? Even better, if you want to contribute a fix then adding test cases in the product would be great.

@nidaca
Copy link
Contributor Author

nidaca commented Feb 20, 2025

@BrennanConroy sorry, the 'unusable' was a bit too strong in this case. I imagine this was just ignored, because everybody had an escape room: make copies one more time at startup.
Yes, I can come with a PoC example .....and even make it together with a fix PR (let me find some proper time to get used with the platform just a bit, I'm not a regular contributor). Any guidance / advice will be more than welcome.

@BrennanConroy
Copy link
Member

https://github.com/dotnet/aspnetcore/blob/main/docs/BuildFromSource.md contains the main instructions for locally building. You should try to restrict yourself to only building in the src/http/ directory to avoid excess build times.

nidaca added a commit to nidaca/aspnetcore that referenced this issue Feb 20, 2025
Correction of BufferedReadStream's Position property setter.
@nidaca
Copy link
Contributor Author

nidaca commented Feb 20, 2025

Hi @BrennanConroy . Managed to make the PR, if you are available to take a look / review it .....it will be great. Thank you.

#60529

@BrennanConroy BrennanConroy added this to the 10.0-preview3 milestone Feb 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants