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.

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

Jump to bottom

Use ReadOnlySpan<byte> for parsing FileWatcher responses #42474

Merged
merged 2 commits into from
Sep 19, 2020
Merged

Use ReadOnlySpan<byte> for parsing FileWatcher responses #42474

merged 2 commits into from
Sep 19, 2020

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Sep 18, 2020

No description provided.

@jkotas jkotas mentioned this pull request Sep 18, 2020
Merged
jkotas
jkotas commented Sep 18, 2020
View reviewed changes
src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Win32.cs Outdated
break;
}

if (info.NextEntryOffset == 0 || info.NextEntryOffset > (uint)buffer.Length)
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes another potential problem where things can wrong in the original code. For example, the original code can get into infinite loop with corrupted data here. We will just exit the loop now.

@tarekgh
Copy link
Member

tarekgh commented Sep 18, 2020

In general I like the change here. I left one comment regarding allow throwing when getting wrong size from the OS. other than that LGTM.

@tarekgh
Copy link
Member

tarekgh commented Sep 18, 2020

@jkotas do you think your change here would be risky if we port it to 3.1 (instead of @carlossanlop fix)? I am seeing the change size is almost same.

@jkotas
Copy link
Member Author

jkotas commented Sep 18, 2020

I think that the original @carlossanlop change is more appropriate for servicing. It is a bit less invasive.

@jkotas jkotas merged commit 1bdb45a into dotnet:master Sep 19, 2020
@jkotas jkotas deleted the FileSystemWatcher branch September 19, 2020 04:31
@briansull briansull mentioned this pull request Sep 22, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jozkee jozkee jozkee left review comments

@ericstj ericstj ericstj left review comments

@tarekgh tarekgh tarekgh approved these changes

@stephentoub stephentoub stephentoub approved these changes

@carlossanlop carlossanlop Awaiting requested review from carlossanlop

Assignees
No one assigned
Labels
area-System.IO
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jkotas @tarekgh @stephentoub @ericstj @jozkee @Dotnet-GitSync-Bot