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

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

Merged
merged 4 commits into from
Mar 26, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ CA1514 | Maintainability | Info | AvoidLengthCheckWhenSlicingToEndAnalyzer, [Doc
CA1515 | Maintainability | Disabled | MakeTypesInternal, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1515)
CA1871 | Performance | Info | DoNotPassNonNullableValueToArgumentNullExceptionThrowIfNull, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1871)
CA1872 | Performance | Info | PreferConvertToHexStringOverBitConverterAnalyzer, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1872)
CA2022 | Reliability | Warning | AvoidUnreliableStreamReadAnalyzer, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2022)
CA2262 | Usage | Info | ProvideHttpClientHandlerMaxResponseHeaderLengthValueCorrectly, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2262)
CA2263 | Usage | Info | PreferGenericOverloadsAnalyzer, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2263)
CA2264 | Usage | Warning | DoNotPassNonNullableValueToArgumentNullExceptionThrowIfNull, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2264)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,18 @@
<value>Prefer 'static readonly' fields over constant array arguments if the called method is called repeatedly and is not mutating the passed array</value>
<comment>{Locked="static readonly"}</comment>
</data>
<data name="AvoidUnreliableStreamReadTitle" xml:space="preserve">
<value>Avoid unreliable call to 'Stream.Read'</value>
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
</data>
<data name="AvoidUnreliableStreamReadCodeFixTitle" xml:space="preserve">
<value>Use 'Stream.ReadExactly'</value>
</data>
<data name="AvoidUnreliableStreamReadDescription" xml:space="preserve">
<value>A call to 'Stream.Read' may return fewer bytes than requested, resulting in unreliable code if the return value is not checked.</value>
</data>
<data name="AvoidUnreliableStreamReadMessage" xml:space="preserve">
<value>Avoid unreliable call to '{0}'</value>
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
</data>
<data name="TestForEmptyStringsUsingStringLengthTitle" xml:space="preserve">
<value>Test for empty strings using string length</value>
</data>
Expand Down
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 isAsyncInvocation = invocation.TargetMethod.Name.EndsWith(Async, StringComparison.Ordinal);
var readExactlyMethods = streamType.GetMembers(isAsyncInvocation ? ReadExactlyAsync : ReadExactly)
.OfType<IMethodSymbol>()
.ToImmutableArray();
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

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.

{
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;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,26 @@
<target state="translated">Nepoužívejte parametry StringBuilder pro volání nespravovaného kódu</target>
<note />
</trans-unit>
<trans-unit id="AvoidUnreliableStreamReadCodeFixTitle">
<source>Use 'Stream.ReadExactly'</source>
<target state="new">Use 'Stream.ReadExactly'</target>
<note />
</trans-unit>
<trans-unit id="AvoidUnreliableStreamReadDescription">
<source>A call to 'Stream.Read' may return fewer bytes than requested, resulting in unreliable code if the return value is not checked.</source>
<target state="new">A call to 'Stream.Read' may return fewer bytes than requested, resulting in unreliable code if the return value is not checked.</target>
<note />
</trans-unit>
<trans-unit id="AvoidUnreliableStreamReadMessage">
<source>Avoid unreliable call to '{0}'</source>
<target state="new">Avoid unreliable call to '{0}'</target>
<note />
</trans-unit>
<trans-unit id="AvoidUnreliableStreamReadTitle">
<source>Avoid unreliable call to 'Stream.Read'</source>
<target state="new">Avoid unreliable call to 'Stream.Read'</target>
<note />
</trans-unit>
<trans-unit id="AvoidUnsealedAttributesDescription">
<source>The .NET Framework class library provides methods for retrieving custom attributes. By default, these methods search the attribute inheritance hierarchy. Sealing the attribute eliminates the search through the inheritance hierarchy and can improve performance.</source>
<target state="translated">Knihovna tříd .NET Framework poskytuje metody pro načítání vlastních atributů. Ve výchozím nastavení tyto metody prohledávají hierarchii dědičnosti atributů. Zapečetění atributu eliminuje prohledávání hierarchie dědičnosti a může zvýšit výkon.</target>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,26 @@
<target state="translated">StringBuilder-Parameter für "P/Invokes" vermeiden</target>
<note />
</trans-unit>
<trans-unit id="AvoidUnreliableStreamReadCodeFixTitle">
<source>Use 'Stream.ReadExactly'</source>
<target state="new">Use 'Stream.ReadExactly'</target>
<note />
</trans-unit>
<trans-unit id="AvoidUnreliableStreamReadDescription">
<source>A call to 'Stream.Read' may return fewer bytes than requested, resulting in unreliable code if the return value is not checked.</source>
<target state="new">A call to 'Stream.Read' may return fewer bytes than requested, resulting in unreliable code if the return value is not checked.</target>
<note />
</trans-unit>
<trans-unit id="AvoidUnreliableStreamReadMessage">
<source>Avoid unreliable call to '{0}'</source>
<target state="new">Avoid unreliable call to '{0}'</target>
<note />
</trans-unit>
<trans-unit id="AvoidUnreliableStreamReadTitle">
<source>Avoid unreliable call to 'Stream.Read'</source>
<target state="new">Avoid unreliable call to 'Stream.Read'</target>
<note />
</trans-unit>
<trans-unit id="AvoidUnsealedAttributesDescription">
<source>The .NET Framework class library provides methods for retrieving custom attributes. By default, these methods search the attribute inheritance hierarchy. Sealing the attribute eliminates the search through the inheritance hierarchy and can improve performance.</source>
<target state="translated">Die .NET Framework-Klassenbibliothek stellt Methoden zum Abrufen benutzerdefinierter Attribute bereit. Standardmäßig durchsuchen diese Methoden die Attributvererbungshierarchie. Durch das Versiegeln des Attributs entfällt das Durchsuchen der Vererbungshierarchie, und die Leistung kann gesteigert werden.</target>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,26 @@
<target state="translated">Evitar los parámetros "StringBuilder" para los elementos P/Invoke</target>
<note />
</trans-unit>
<trans-unit id="AvoidUnreliableStreamReadCodeFixTitle">
<source>Use 'Stream.ReadExactly'</source>
<target state="new">Use 'Stream.ReadExactly'</target>
<note />
</trans-unit>
<trans-unit id="AvoidUnreliableStreamReadDescription">
<source>A call to 'Stream.Read' may return fewer bytes than requested, resulting in unreliable code if the return value is not checked.</source>
<target state="new">A call to 'Stream.Read' may return fewer bytes than requested, resulting in unreliable code if the return value is not checked.</target>
<note />
</trans-unit>
<trans-unit id="AvoidUnreliableStreamReadMessage">
<source>Avoid unreliable call to '{0}'</source>
<target state="new">Avoid unreliable call to '{0}'</target>
<note />
</trans-unit>
<trans-unit id="AvoidUnreliableStreamReadTitle">
<source>Avoid unreliable call to 'Stream.Read'</source>
<target state="new">Avoid unreliable call to 'Stream.Read'</target>
<note />
</trans-unit>
<trans-unit id="AvoidUnsealedAttributesDescription">
<source>The .NET Framework class library provides methods for retrieving custom attributes. By default, these methods search the attribute inheritance hierarchy. Sealing the attribute eliminates the search through the inheritance hierarchy and can improve performance.</source>
<target state="translated">La biblioteca de clases de .NET Framework proporciona los métodos para recuperar los atributos personalizados. De forma predeterminada, estos métodos buscan la jerarquía de herencia del atributo. Al sellar el atributo, se elimina la búsqueda a través de la jerarquía de herencia y puede mejorarse el rendimiento.</target>
Expand Down
Loading