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

Issue #60416 - Correction of BufferedReadStream 's Position #60529

Merged
merged 5 commits into from
Feb 21, 2025

Conversation

nidaca
Copy link
Contributor

@nidaca nidaca commented Feb 20, 2025

Correction of Microsoft.AspNetCore.WebUtilities.BufferedReadStream Position property

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

The implementation of Position's property setter for Microsoft.AspNetCore.WebUtilities.BufferedReadStream class was incorrect and changed accordingly with the proposed solution described in the issue #60416 .

Description

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.
The solution was to 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.

Fixes #60416

@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 20, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Feb 20, 2025
@nidaca

This comment was marked as off-topic.

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

Thanks for reporting this issue and following through with the fix.

@nidaca nidaca requested a review from halter73 February 21, 2025 08:28
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

LGTM. Even though this is a bug fix, I don't think this meets the bar for patching given that it's unusual to seek a BufferedReadStream and it's rare for the inner stream to even be seekable which is probably why this took so long to discover.

@halter73 halter73 enabled auto-merge (squash) February 21, 2025 22:12
@halter73 halter73 merged commit 93f7682 into dotnet:main Feb 21, 2025
27 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview3 milestone Feb 21, 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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Microsoft.AspNetCore.WebUtilities.BufferedReadStream - incorrect implementation of Position property's setter
2 participants