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

Fix CA2022 warnings (Avoid inexact read with 'Stream.Read') #100352

Merged
merged 7 commits into from
Mar 29, 2024

Conversation

mpidash
Copy link
Contributor

@mpidash mpidash commented Mar 27, 2024

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 and UnmanagedMemoryStream, see dotnet/roslyn-analyzers#7268):

_unmanagedStream.Read(buffer, 0, (int)_unmanagedStream.Length);

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 27, 2024
@@ -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
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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);
Copy link
Member

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);
Copy link
Member

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);
Copy link
Member

Choose a reason for hiding this comment

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

Basically all of them :)

Comment on lines 129 to 131
while (totalRead < audio._stream.Length)
{
int bytesRead = audio._stream.Read(data, totalRead, audio._stream.Length - totalRead);
Copy link
Member

Choose a reason for hiding this comment

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

NIT:

Suggested change
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);

Copy link
Contributor Author

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.

Copy link
Member

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta 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.

4 participants