-
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 CA2022 warnings (Avoid inexact read with 'Stream.Read') #100352
Conversation
@@ -963,7 +963,7 @@ public string ReadExisting() | |||
Buffer.BlockCopy(_inBuffer, _readPos, bytesReceived, 0, CachedBytesToRead); | |||
} | |||
|
|||
_internalSerialStream.Read(bytesReceived, CachedBytesToRead, bytesReceived.Length - (CachedBytesToRead)); // get everything | |||
_internalSerialStream.ReadExactly(bytesReceived, CachedBytesToRead, bytesReceived.Length - (CachedBytesToRead)); // get everything |
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.
This won't compile as written. This library builds for multiple target frameworks, some of which don't have ReadExactly.
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.
Thanks, I've added a check for NET7_0_OR_GREATER
before using ReadExactly
.
Is there a build target to check locally if I've used a function that is not available in a target framework? I ran the tests locally with .\build.cmd -subset clr+mono+libs+libs.tests -test -rc Release
but got no build error.
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.
Thanks. That will only end up building for the current platform. There used to be a command for building all flavors of all libraries, but I can't find it in the docs now. @ViktorHofer? In the meantime, you can cd into the src folder for the library and dotnet build
will build all targets for it.
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.
build.cmd libs -allconfigurations
@@ -86,7 +86,7 @@ public void Close() | |||
_bufferState = BufferState.Reading; | |||
_buffer = new byte[_stream.Length]; | |||
_stream.Position = 0; | |||
_stream.Read(_buffer, 0, _buffer.Length); | |||
_stream.ReadExactly(_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.
Same here
@@ -121,7 +121,7 @@ internal void PlayWaveFile(AudioData audio) | |||
try | |||
{ | |||
byte[] data = new byte[(int)audio._stream.Length]; | |||
audio._stream.Read(data, 0, data.Length); | |||
audio._stream.ReadExactly(data); |
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.
And here
@@ -174,7 +174,7 @@ public Stream LoadResource(Uri uri, string mediaType) | |||
int cLen = (int)stream.Length; | |||
MemoryStream memStream = new(cLen); | |||
byte[] ab = new byte[cLen]; | |||
stream.Read(ab, 0, ab.Length); | |||
stream.ReadExactly(ab); |
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.
Basically all of them :)
src/libraries/System.ServiceModel.Syndication/src/System/ServiceModel/XmlBuffer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Ports/src/System/IO/Ports/SerialPort.cs
Outdated
Show resolved
Hide resolved
while (totalRead < audio._stream.Length) | ||
{ | ||
int bytesRead = audio._stream.Read(data, totalRead, audio._stream.Length - totalRead); |
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:
while (totalRead < audio._stream.Length) | |
{ | |
int bytesRead = audio._stream.Read(data, totalRead, audio._stream.Length - totalRead); | |
while (totalRead < data.Length) | |
{ | |
int bytesRead = audio._stream.Read(data, totalRead, data.Length - totalRead); |
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.
Thanks, I will use data.Length
here, but I am not sure why this does not trigger CS1503 as well.
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.
LGTM, thanks!
Follow-up from dotnet/roslyn-analyzers#7208.
The following finding from the initial analyzer PR that flags an
UnmanagedMemoryStream
was not fixed, as we excluded well known reliable stream types (MemoryStream
andUnmanagedMemoryStream
, see dotnet/roslyn-analyzers#7268):runtime/src/libraries/System.Private.CoreLib/src/System/IO/UnmanagedMemoryStreamWrapper.cs
Line 93 in 79dd9ba