-
Notifications
You must be signed in to change notification settings - Fork 467
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
Add CA2022: Avoid inexact read with 'Stream.Read' #7208
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
130 changes: 130 additions & 0 deletions
130
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/AvoidUnreliableStreamRead.Fixer.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,130 @@ | ||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Collections.Immutable; | ||
using System.Composition; | ||
using System.Linq; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Analyzer.Utilities; | ||
using Analyzer.Utilities.Extensions; | ||
using Microsoft.CodeAnalysis; | ||
using Microsoft.CodeAnalysis.CodeActions; | ||
using Microsoft.CodeAnalysis.CodeFixes; | ||
using Microsoft.CodeAnalysis.Editing; | ||
using Microsoft.CodeAnalysis.Operations; | ||
|
||
namespace Microsoft.NetCore.Analyzers.Runtime | ||
{ | ||
using static MicrosoftNetCoreAnalyzersResources; | ||
|
||
/// <summary> | ||
/// CA2022: <inheritdoc cref="AvoidUnreliableStreamReadTitle"/> | ||
/// </summary> | ||
[ExportCodeFixProvider(LanguageNames.CSharp, LanguageNames.VisualBasic), Shared] | ||
public sealed class AvoidUnreliableStreamReadFixer : CodeFixProvider | ||
{ | ||
private const string Async = nameof(Async); | ||
private const string ReadExactly = nameof(ReadExactly); | ||
private const string ReadExactlyAsync = nameof(ReadExactlyAsync); | ||
|
||
public sealed override ImmutableArray<string> FixableDiagnosticIds { get; } = | ||
ImmutableArray.Create(AvoidUnreliableStreamReadAnalyzer.RuleId); | ||
|
||
public sealed override FixAllProvider GetFixAllProvider() | ||
{ | ||
return WellKnownFixAllProviders.BatchFixer; | ||
} | ||
|
||
public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) | ||
{ | ||
var root = await context.Document.GetRequiredSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); | ||
var node = root.FindNode(context.Span, getInnermostNodeForTie: true); | ||
|
||
if (node is null) | ||
{ | ||
return; | ||
} | ||
|
||
var semanticModel = await context.Document.GetRequiredSemanticModelAsync(context.CancellationToken).ConfigureAwait(false); | ||
var operation = semanticModel.GetOperation(node, context.CancellationToken); | ||
|
||
if (operation is not IInvocationOperation invocation || | ||
invocation.Instance is null) | ||
{ | ||
return; | ||
} | ||
|
||
var compilation = semanticModel.Compilation; | ||
var streamType = compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemIOStream); | ||
|
||
if (streamType is null) | ||
{ | ||
return; | ||
} | ||
|
||
var readExactlyMethods = streamType.GetMembers(ReadExactly) | ||
.OfType<IMethodSymbol>() | ||
.ToImmutableArray(); | ||
|
||
if (readExactlyMethods.IsEmpty) | ||
{ | ||
return; | ||
} | ||
|
||
var codeAction = CodeAction.Create( | ||
AvoidUnreliableStreamReadCodeFixTitle, | ||
ct => ReplaceWithReadExactlyCall(context.Document, ct), | ||
nameof(AvoidUnreliableStreamReadCodeFixTitle)); | ||
|
||
context.RegisterCodeFix(codeAction, context.Diagnostics); | ||
|
||
async Task<Document> ReplaceWithReadExactlyCall(Document document, CancellationToken cancellationToken) | ||
{ | ||
var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false); | ||
var generator = editor.Generator; | ||
var arguments = invocation.Arguments.GetArgumentsInParameterOrder(); | ||
|
||
var isAsyncInvocation = invocation.TargetMethod.Name.EndsWith(Async, StringComparison.Ordinal); | ||
var methodExpression = generator.MemberAccessExpression( | ||
invocation.Instance.Syntax, | ||
isAsyncInvocation ? ReadExactlyAsync : ReadExactly); | ||
var methodInvocation = CanUseSpanOverload() | ||
? generator.InvocationExpression( | ||
methodExpression, | ||
isAsyncInvocation && arguments.Length == 4 | ||
// Stream.ReadExactlyAsync(buffer, ct) | ||
?[arguments[0].Syntax, arguments[3].Syntax] | ||
// Stream.ReadExactly(buffer) and Stream.ReadExactlyAsync(buffer) | ||
:[arguments[0].Syntax]) | ||
: generator.InvocationExpression( | ||
methodExpression, | ||
invocation.Arguments.Where(a => !a.IsImplicit).Select(a => a.Syntax)); | ||
|
||
editor.ReplaceNode(invocation.Syntax, methodInvocation.WithTriviaFrom(invocation.Syntax)); | ||
|
||
return document.WithSyntaxRoot(editor.GetChangedRoot()); | ||
|
||
bool CanUseSpanOverload() | ||
{ | ||
return arguments.Length >= 3 && | ||
arguments[2].Value is IPropertyReferenceOperation propertyRef && | ||
propertyRef.Property.Name.Equals(WellKnownMemberNames.LengthPropertyName, StringComparison.Ordinal) && | ||
AreSameInstance(arguments[0].Value, propertyRef.Instance); | ||
} | ||
|
||
static bool AreSameInstance(IOperation? operation1, IOperation? operation2) | ||
{ | ||
return (operation1, operation2) switch | ||
{ | ||
(IFieldReferenceOperation fieldRef1, IFieldReferenceOperation fieldRef2) => fieldRef1.Member == fieldRef2.Member, | ||
(IPropertyReferenceOperation propRef1, IPropertyReferenceOperation propRef2) => propRef1.Member == propRef2.Member, | ||
(IParameterReferenceOperation paramRef1, IParameterReferenceOperation paramRef2) => paramRef1.Parameter == paramRef2.Parameter, | ||
(ILocalReferenceOperation localRef1, ILocalReferenceOperation localRef2) => localRef1.Local == localRef2.Local, | ||
_ => false, | ||
}; | ||
} | ||
} | ||
} | ||
} | ||
} |
153 changes: 153 additions & 0 deletions
153
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/AvoidUnreliableStreamRead.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,153 @@ | ||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
||
using System.Collections.Immutable; | ||
using System.Diagnostics.CodeAnalysis; | ||
using System.Linq; | ||
using Analyzer.Utilities; | ||
using Analyzer.Utilities.Extensions; | ||
using Microsoft.CodeAnalysis; | ||
using Microsoft.CodeAnalysis.Diagnostics; | ||
using Microsoft.CodeAnalysis.Operations; | ||
|
||
namespace Microsoft.NetCore.Analyzers.Runtime | ||
{ | ||
using static MicrosoftNetCoreAnalyzersResources; | ||
|
||
/// <summary> | ||
/// CA2022: <inheritdoc cref="AvoidUnreliableStreamReadTitle"/> | ||
/// </summary> | ||
[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)] | ||
public sealed class AvoidUnreliableStreamReadAnalyzer : DiagnosticAnalyzer | ||
{ | ||
internal const string RuleId = "CA2022"; | ||
|
||
private const string Read = nameof(Read); | ||
private const string ReadAsync = nameof(ReadAsync); | ||
|
||
internal static readonly DiagnosticDescriptor Rule = DiagnosticDescriptorHelper.Create( | ||
RuleId, | ||
CreateLocalizableResourceString(nameof(AvoidUnreliableStreamReadTitle)), | ||
CreateLocalizableResourceString(nameof(AvoidUnreliableStreamReadMessage)), | ||
DiagnosticCategory.Reliability, | ||
RuleLevel.BuildWarning, | ||
CreateLocalizableResourceString(nameof(AvoidUnreliableStreamReadDescription)), | ||
isPortedFxCopRule: false, | ||
isDataflowRule: false); | ||
|
||
public sealed override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); | ||
|
||
public sealed override void Initialize(AnalysisContext context) | ||
{ | ||
context.EnableConcurrentExecution(); | ||
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); | ||
context.RegisterCompilationStartAction(OnCompilationStart); | ||
} | ||
|
||
private void OnCompilationStart(CompilationStartAnalysisContext context) | ||
{ | ||
if (!RequiredSymbols.TryGetSymbols(context.Compilation, out var symbols)) | ||
{ | ||
return; | ||
} | ||
|
||
context.RegisterOperationAction(AnalyzeInvocation, OperationKind.Invocation); | ||
|
||
void AnalyzeInvocation(OperationAnalysisContext context) | ||
{ | ||
var invocation = (IInvocationOperation)context.Operation; | ||
|
||
if (symbols.IsAnyStreamReadMethod(invocation.TargetMethod)) | ||
{ | ||
if (invocation.Parent is not IExpressionStatementOperation) | ||
{ | ||
return; | ||
} | ||
} | ||
else if (symbols.IsAnyStreamReadAsyncMethod(invocation.TargetMethod)) | ||
{ | ||
if (invocation.Parent is not IAwaitOperation awaitOperation || | ||
awaitOperation.Parent is not IExpressionStatementOperation) | ||
{ | ||
return; | ||
} | ||
} | ||
else | ||
{ | ||
return; | ||
} | ||
|
||
context.ReportDiagnostic(invocation.CreateDiagnostic(Rule, invocation.TargetMethod.ToDisplayString())); | ||
} | ||
} | ||
|
||
internal sealed class RequiredSymbols | ||
{ | ||
private RequiredSymbols( | ||
ImmutableArray<IMethodSymbol> streamReadMethods, | ||
ImmutableArray<IMethodSymbol> streamReadAsyncMethods) | ||
{ | ||
_streamReadMethods = streamReadMethods; | ||
_streamReadAsyncMethods = streamReadAsyncMethods; | ||
} | ||
|
||
public static bool TryGetSymbols(Compilation compilation, [NotNullWhen(true)] out RequiredSymbols? symbols) | ||
{ | ||
symbols = default; | ||
|
||
var streamType = compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemIOStream); | ||
|
||
if (streamType is null) | ||
{ | ||
return false; | ||
} | ||
|
||
var streamReadMethods = streamType.GetMembers(Read) | ||
.OfType<IMethodSymbol>() | ||
.ToImmutableArray(); | ||
var streamReadAsyncMethods = streamType.GetMembers(ReadAsync) | ||
.OfType<IMethodSymbol>() | ||
.ToImmutableArray(); | ||
|
||
if (streamReadMethods.IsEmpty && streamReadAsyncMethods.IsEmpty) | ||
{ | ||
return false; | ||
} | ||
|
||
symbols = new RequiredSymbols(streamReadMethods, streamReadAsyncMethods); | ||
|
||
return true; | ||
} | ||
|
||
public bool IsAnyStreamReadMethod(IMethodSymbol method) | ||
{ | ||
return _streamReadMethods.Any(m => | ||
SymbolEqualityComparer.Default.Equals(method, m) || IsOverrideOf(method, m)); | ||
} | ||
|
||
public bool IsAnyStreamReadAsyncMethod(IMethodSymbol method) | ||
{ | ||
return _streamReadAsyncMethods.Any(m => | ||
SymbolEqualityComparer.Default.Equals(method, m) || IsOverrideOf(method, m)); | ||
} | ||
|
||
private static bool IsOverrideOf(IMethodSymbol method, IMethodSymbol baseMethod) | ||
{ | ||
var overriddenMethod = method.OverriddenMethod; | ||
while (overriddenMethod is not null) | ||
{ | ||
if (SymbolEqualityComparer.Default.Equals(overriddenMethod, baseMethod)) | ||
{ | ||
return true; | ||
} | ||
|
||
overriddenMethod = overriddenMethod.OverriddenMethod; | ||
} | ||
|
||
return false; | ||
} | ||
|
||
private readonly ImmutableArray<IMethodSymbol> _streamReadMethods; | ||
private readonly ImmutableArray<IMethodSymbol> _streamReadAsyncMethods; | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
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, 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!
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 can take a look at this.