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

Don't read past the end of the HTTP stream #3300

Merged
merged 1 commit into from
Jun 5, 2023

Conversation

JohnMcPMS
Copy link
Member

@JohnMcPMS JohnMcPMS commented Jun 2, 2023

Fixes #3297 (based on crash reports + code review + issue report alignment)

There is a bug in this stream implementation that allows it to attempt to read past the end of the stream if requested. That aligns with the issue reports, which appear to be crashes in the attempt to update the source information, which uses this code. It also aligns with the crash reports, which show an access violation on a page boundary when reading from the stream.

Change

Ensures that the trim start location is at least not past the end of the given buffer. Ensures that the array_view will not reference bytes past the end of the given buffer.

Validation

Added a new test to ensure that we get the number of bytes expected based on the stream size (the 0 byte file issue persists, so I put a retry loop on the test).

Microsoft Reviewers: Open in CodeFlow

@JohnMcPMS JohnMcPMS requested a review from a team as a code owner June 2, 2023 21:50
@microsoft-github-policy-service microsoft-github-policy-service bot added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Jun 2, 2023
@yao-msft
Copy link
Contributor

yao-msft commented Jun 2, 2023

I guess we should make the fix to the AppInstaller copy as well?

@JohnMcPMS
Copy link
Member Author

I guess we should make the fix to the AppInstaller copy as well?

Yes, I was going to do that.

@ryfu-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JohnMcPMS JohnMcPMS merged commit eb3d451 into microsoft:master Jun 5, 2023
@JohnMcPMS JohnMcPMS deleted the fix-http-stream branch June 5, 2023 17:06
JohnMcPMS added a commit to JohnMcPMS/winget-cli that referenced this pull request Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Most commands fail silently
3 participants