-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fix Int32 overflow bug in buffering logic #60459
Conversation
Tagging subscribers to this area: @dotnet/area-system-io |
/backport to release/6.0 |
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1345172097 |
Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com>
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.
Found only nits, I also tried to see if I could find more overflow issues in BufferedFileStreamStrategy but didn't see any.
Thanks, @adamsitnik.
using (var stream = new FileStream(filePath, FileMode.Create, FileAccess.Write)) | ||
{ | ||
stream.Seek(position1, SeekOrigin.Begin); | ||
stream.Write(data1, 0, data1.Length); |
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.
nit: Use Span-based overloads
stream.Write(data1, 0, data1.Length); | |
stream.Write(data1); |
stream.Write(data1, 0, data1.Length); | ||
|
||
stream.Seek(position2, SeekOrigin.Begin); | ||
stream.Write(data2, 0, data2.Length); |
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.
ditto
stream.Write(data2, 0, data2.Length); | |
stream.Write(data2); |
using (var stream = new BufferedStream(new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.None, bufferSize: 0))) | ||
{ | ||
stream.Seek(position1, SeekOrigin.Begin); | ||
Assert.Equal(buffer.Length, stream.Read(buffer)); | ||
Assert.Equal(data1, buffer); | ||
|
||
stream.Seek(position2, SeekOrigin.Begin); | ||
Assert.Equal(buffer.Length, stream.Read(buffer)); | ||
Assert.Equal(data2, buffer); | ||
} |
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 think this piece of code should be a separate test in src/libraries/System.IO/tests/BufferedStream/BufferedStreamTests.cs
Merging to keep the backport PR as a clean port of this one i.e: to not have different commits. CI issue |
fixes #60430
For the following code:
For a
newPos > 4GB
and smalloldPos
and_readPos
we had an int32 overflow. Sample C# app to demonstrate the problem:It seems to be a very old bug that was always present in
BufferedStream
and I've copied it toBufferedFileStreamStrategy
in 6.0.1.0 branch:
https://github.com/dotnet/corefx/blob/c74ea81f2ddad07dc05d695d8244fb2ece7e398d/src/System.IO/src/System/IO/BufferedStream.cs#L1053-L1054
Since we are very close to 6.0 release I've focused on making smallest possible change instead of doing a refactor and providing code that would look better but would be harder to review.