Add CA2022: Avoid inexact read with 'Stream.Read'

[mpidash]

Fixes dotnet/runtime#69159.

Category: Reliability
Severity: Warning

This analyzer detects calls to Stream.Read or Stream.ReadAsync that do not check the return value. These methods may return fewer bytes than requested, resulting in unreliable code.
The fixer will recommend replacing the call with Stream.ReadExactly or Stream.ReadExactlyAsync, if available.

---

All findings look legit, mostly calls in test code, although there are a few hits in non-test code in dotnet/runtime:

1. https://github.com/dotnet/runtime/blob/79dd9bae9bb881eb716b608577c4cedc6c9cba72/src/libraries/System.Private.CoreLib/src/System/IO/UnmanagedMemoryStreamWrapper.cs#L93
2. https://github.com/dotnet/runtime/blob/79dd9bae9bb881eb716b608577c4cedc6c9cba72/src/libraries/System.ServiceModel.Syndication/src/System/ServiceModel/XmlBuffer.cs#L89
3. https://github.com/dotnet/runtime/blob/79dd9bae9bb881eb716b608577c4cedc6c9cba72/src/libraries/System.IO.Ports/src/System/IO/Ports/SerialPort.cs#L966
4. https://github.com/dotnet/runtime/blob/79dd9bae9bb881eb716b608577c4cedc6c9cba72/src/libraries/System.Speech/src/Internal/Synthesis/AudioBase.cs#L124
5. https://github.com/dotnet/runtime/blob/79dd9bae9bb881eb716b608577c4cedc6c9cba72/src/libraries/System.Speech/src/Internal/Synthesis/EngineSite.cs#L177

---

I've also added all findings from dotnet/roslyn, dotnet/runtime and dotnet/aspnetcore (no warnings found in dotnet/roslyn-analyzer:
roslyn.txt
runtime.txt
aspnetcore.txt

[codecov]

Codecov Report

Attention: Patch coverage is 97.15302% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 96.48%. Comparing base (29bdbf5) to head (254f3a8).
Report is 3 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #7208    +/-   ##
========================================
  Coverage   96.48%   96.48%            
========================================
  Files        1439     1442     +3     
  Lines      344369   345212   +843     
  Branches    11324    11345    +21     
========================================
+ Hits       332259   333081   +822     
- Misses       9244     9256    +12     
- Partials     2866     2875     +9     

[buyaa-n]

Thank you @mpidash, left a few comment, mostly for messages. Overall looks good 👍

[buyaa-n]

Looks good to me, thank you @mpidash!

One of the CI legs somehow failed, might be one of the docs are not in sync that needs build with /pack PTAL

[buyaa-n]

All findings look legit, mostly calls in test code, although there are a few hits in non-test code in dotnet/runtime:

Runtime findings in src looks legit, please let us know if you would like to put up a fix for them, and also set dotnet_diagnostic.CA2022.severity = warning for source projects and set dotnet_diagnostic.CA2022.severity = none for test projects, we will not fix any findings in test.

This better to be done before the merged code flows to runtime, as the analyzer warns by default it would fail the roslyn-analyzers => runtime code-flow PRs.

[mpidash]

Thanks for the review!

One of the CI legs somehow failed, might be one of the docs are not in sync that needs build with /pack PTAL

I ran the pack task again locally and got no changes, but it seems like the CI ran successfully now.

This better to be done before the merged code flows to runtime, as the analyzer warns by default it would fail the roslyn-analyzers => runtime code-flow PRs.

I can prepare a MR tomorrow to fix the findings and enable the rule (and disable it for tests).

[buyaa-n]

I ran the pack task again locally and got no changes, but it seems like the CI ran successfully now.

Yeah, I hit the rerun button and now it is passed, gonna merge now

I can prepare a MR tomorrow to fix the findings and enable the rule (and disable it for tests).

Great, thank you!

[stephentoub]

Can we special-case MemoryStream and UnmanagedMemoryStream where if we can prove the target Stream is one of those we don't issue diagnostics? Those, in particular the former, are commonly used and we know they are actually safe for this usage; I'd like to avoid causing a lot of noise around them. While we don't necessarily document it's safe, we also don't need to cause folks to do a lot of work in response.

[buyaa-n]

Those, in particular the former, are commonly used and we know they are actually safe for this usage; I'd like to avoid causing a lot of noise around them. While we don't necessarily document it's safe, we also don't need to cause folks to do a lot of work in response.

Thanks, created an issue, FYI @mpidash as the analyzer warns by default we need to fix such breaking false positives ASAP, please let me know ASAP if you have no time for this. Thanks!

[mpidash]

I can take a look at this.