Skip to content

Commit

Permalink
[IAST] Safeguard Method Replace aspects with try/catch (#5841)
Browse files Browse the repository at this point in the history 
## Summary of changes
Covered all `AspectMethodReplace` aspects with try catch clauses to
ensure no crash will bubble up to client, following new analyzer rules.

Disabled some weird casts and processes to support some functions not
present en NetCore 2.0, but present in 2.1 (netstandard2 assembly is
loaded in netcore2.1 apps).

Disabled some overloads receiving generic undefined arguments until
proper callsite support is implemented.

## Reason for change
SSI will make the tracer enabled for a lot more of services when
available. We must ensure we do not break any of them, and if so, that
we provide a fast answer.

## Implementation details
Apply analyzer suggestions adding a try / catch clause in all
`Methodreplace` aspects

## Test coverage

## Other details
<!-- Fixes #{issue} -->

<!-- ⚠️ Note: where possible, please obtain 2 approvals prior to
merging. Unless CODEOWNERS specifies otherwise, for external teams it is
typically best to have one review from a team member, and one review
from apm-dotnet. Trivial changes do not require 2 reviews. -->
  • Loading branch information
@daniel-romano-DD
daniel-romano-DD authored Aug 6, 2024
1 parent 51fa7a5 commit 8a6b8e2
Show file tree
Hide file tree
Showing 28 changed files with 1,901 additions and 563 deletions.
49 changes: 28 additions & 21 deletions tracer/src/Datadog.Trace.Tools.Analyzers/AspectAnalyzers/ReplaceAspectAnalyzer.cs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// <copyright file="ReplaceAspectAnalyzer.cs" company="Datadog">
// <copyright file="ReplaceAspectAnalyzer.cs" company="Datadog">
// Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License.
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
// </copyright>
Expand Down Expand Up @@ -28,7 +28,7 @@ public class ReplaceAspectAnalyzer : DiagnosticAnalyzer
/// <summary>
/// The severity of the diagnostic
/// </summary>
public const DiagnosticSeverity Severity = DiagnosticSeverity.Info;
public const DiagnosticSeverity Severity = DiagnosticSeverity.Error;

#pragma warning disable RS2008 // Enable analyzer release tracking for the analyzer project
private static readonly DiagnosticDescriptor MissingTryCatchRule = new(
Expand Down Expand Up @@ -92,6 +92,9 @@ private void AnalyseMethod(SyntaxNodeAnalysisContext context)
}

var bodyBlock = methodDeclaration.Body;
var isVoidMethod = methodDeclaration.ReturnType is PredefinedTypeSyntax { Keyword.Text: "void" };
int expectedStatements = isVoidMethod ? 2 : 3;

if (bodyBlock is null)
{
// If we don't have a bodyBlock, it's probably a lambda or expression bodied member
Expand All @@ -107,15 +110,15 @@ private void AnalyseMethod(SyntaxNodeAnalysisContext context)
return;
}

if (bodyBlock.Statements.Count != 3)
if (bodyBlock.Statements.Count != expectedStatements)
{
// We require exactly 3 statements, so this must be an error
// We require exactly a predefined amount of statements, so this must be an error
context.ReportDiagnostic(Diagnostic.Create(MissingTryCatchRule, bodyBlock.GetLocation()));
return;
}

// check the first statement
if (bodyBlock.Statements[0] is not LocalDeclarationStatementSyntax localDeclaration)
if (!isVoidMethod && bodyBlock.Statements[0] is not LocalDeclarationStatementSyntax)
{
// this is an error, and we can't go much further
context.ReportDiagnostic(Diagnostic.Create(MissingTryCatchRule, bodyBlock.GetLocation()));
Expand Down Expand Up @@ -180,25 +183,29 @@ private void AnalyseMethod(SyntaxNodeAnalysisContext context)
}

// final check, do we return the variable?
if (bodyBlock.Statements[2] is not ReturnStatementSyntax returnStatement)
if (!isVoidMethod)
{
context.ReportDiagnostic(Diagnostic.Create(MissingTryCatchRule, bodyBlock.GetLocation()));
return;
}
if (bodyBlock.Statements[2] is not ReturnStatementSyntax returnStatement)
{
context.ReportDiagnostic(Diagnostic.Create(MissingTryCatchRule, bodyBlock.GetLocation()));
return;
}

// should be returning the variable
if (returnStatement.Expression is not IdentifierNameSyntax identifierName)
{
context.ReportDiagnostic(Diagnostic.Create(MissingTryCatchRule, bodyBlock.GetLocation()));
return;
}
// should be returning the variable
if (returnStatement.Expression is not IdentifierNameSyntax identifierName)
{
context.ReportDiagnostic(Diagnostic.Create(MissingTryCatchRule, bodyBlock.GetLocation()));
return;
}

if (!localDeclaration.Declaration.Variables.Any()
|| localDeclaration.Declaration.Variables[0] is not { } variable
|| variable.Identifier.ToString() != identifierName.Identifier.ToString())
{
// not returning the right thing
context.ReportDiagnostic(Diagnostic.Create(MissingTryCatchRule, bodyBlock.GetLocation()));
LocalDeclarationStatementSyntax localDeclaration = (LocalDeclarationStatementSyntax)bodyBlock.Statements[0];
if (!localDeclaration.Declaration.Variables.Any()
|| localDeclaration.Declaration.Variables[0] is not { } variable
|| variable.Identifier.ToString() != identifierName.Identifier.ToString())
{
// not returning the right thing
context.ReportDiagnostic(Diagnostic.Create(MissingTryCatchRule, bodyBlock.GetLocation()));
}
}
}
}
2 changes: 1 addition & 1 deletion tracer/src/Datadog.Trace.Tools.Analyzers/AspectAnalyzers/ReplaceAspectCodeFixProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ private async Task<Document> AddTryCatch(Document document, MethodDeclarationSyn

var catchDeclaration = SyntaxFactory.CatchDeclaration(SyntaxFactory.IdentifierName("Exception"), SyntaxFactory.Identifier("ex"));
var logExpression = SyntaxFactory.ExpressionStatement(
SyntaxFactory.ParseExpression($$"""Log.Error(ex, $"Error invoking {nameof({{typeName}})}.{nameof({{methodName}})}")"""));
SyntaxFactory.ParseExpression($$"""IastModule.Log.Error(ex, $"Error invoking {nameof({{typeName}})}.{nameof({{methodName}})}")"""));

var catchSyntax = SyntaxFactory.CatchClause()
.WithDeclaration(catchDeclaration)
Expand Down
6 changes: 2 additions & 4 deletions ...net461/Datadog.Trace.SourceGenerators/AspectsDefinitionsGenerator/AspectsDefinitions.g.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -422,17 +422,15 @@ internal static partial class AspectDefinitions
" [AspectMethodReplace(\"System.String::Concat(System.Object,System.Object,System.Object,System.Object)\",\"\",[0],[False],[None],Default,[])] Concat(System.Object,System.Object,System.Object,System.Object)",
" [AspectMethodReplace(\"System.String::Concat(System.String[])\",\"\",[0],[False],[None],Default,[])] Concat(System.String[])",
" [AspectMethodReplace(\"System.String::Concat(System.Object[])\",\"\",[0],[False],[None],Default,[])] Concat(System.Object[])",
" [AspectMethodReplace(\"System.String::Concat(System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] Concat(System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Concat(System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] Concat2(System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Concat(System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] Concat(System.Collections.Generic.IEnumerable)",
" [AspectMethodReplace(\"System.String::Substring(System.Int32)\",\"\",[0],[False],[StringLiteral_0],Default,[])] Substring(System.String,System.Int32)",
" [AspectMethodReplace(\"System.String::Substring(System.Int32,System.Int32)\",\"\",[0],[False],[StringLiteral_0],Default,[])] Substring(System.String,System.Int32,System.Int32)",
" [AspectMethodReplace(\"System.String::ToCharArray()\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToCharArray(System.String)",
" [AspectMethodReplace(\"System.String::ToCharArray(System.Int32,System.Int32)\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToCharArray(System.String,System.Int32,System.Int32)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.String[],System.Int32,System.Int32)\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.String[],System.Int32,System.Int32)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Object[])\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.Object[])",
" [AspectMethodReplace(\"System.String::Join(System.String,System.String[])\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.String[])",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] JoinString(System.String,System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] JoinString(System.String,System.Collections.Generic.IEnumerable)",
" [AspectMethodReplace(\"System.String::ToUpper()\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToUpper(System.String)",
" [AspectMethodReplace(\"System.String::ToUpper(System.Globalization.CultureInfo)\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToUpper(System.String,System.Globalization.CultureInfo)",
" [AspectMethodReplace(\"System.String::ToUpperInvariant()\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToUpperInvariant(System.String)",
Expand Down
9 changes: 2 additions & 7 deletions ...net6.0/Datadog.Trace.SourceGenerators/AspectsDefinitionsGenerator/AspectsDefinitions.g.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -460,8 +460,7 @@ internal static partial class AspectDefinitions
" [AspectMethodReplace(\"System.String::Concat(System.String,System.String,System.String,System.String)\",\"\",[0],[False],[StringLiterals],Default,[])] Concat(System.String,System.String,System.String,System.String)",
" [AspectMethodReplace(\"System.String::Concat(System.String[])\",\"\",[0],[False],[None],Default,[])] Concat(System.String[])",
" [AspectMethodReplace(\"System.String::Concat(System.Object[])\",\"\",[0],[False],[None],Default,[])] Concat(System.Object[])",
" [AspectMethodReplace(\"System.String::Concat(System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] Concat(System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Concat(System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] Concat2(System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Concat(System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] Concat(System.Collections.Generic.IEnumerable)",
" [AspectMethodReplace(\"System.String::Substring(System.Int32)\",\"\",[0],[False],[StringLiteral_0],Default,[])] Substring(System.String,System.Int32)",
" [AspectMethodReplace(\"System.String::Substring(System.Int32,System.Int32)\",\"\",[0],[False],[StringLiteral_0],Default,[])] Substring(System.String,System.Int32,System.Int32)",
" [AspectMethodReplace(\"System.String::ToCharArray()\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToCharArray(System.String)",
Expand All @@ -470,11 +469,9 @@ internal static partial class AspectDefinitions
" [AspectMethodReplace(\"System.String::Join(System.Char,System.String[])\",\"\",[0],[False],[None],Default,[])] Join(System.Char,System.String[])",
" [AspectMethodReplace(\"System.String::Join(System.Char,System.Object[])\",\"\",[0],[False],[None],Default,[])] Join(System.Char,System.Object[])",
" [AspectMethodReplace(\"System.String::Join(System.Char,System.String[],System.Int32,System.Int32)\",\"\",[0],[False],[None],Default,[])] Join(System.Char,System.String[],System.Int32,System.Int32)",
" [AspectMethodReplace(\"System.String::Join(System.Char,System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] Join(System.Char,System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Object[])\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.Object[])",
" [AspectMethodReplace(\"System.String::Join(System.String,System.String[])\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.String[])",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] JoinString(System.String,System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] JoinString(System.String,System.Collections.Generic.IEnumerable)",
" [AspectMethodReplace(\"System.String::ToUpper()\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToUpper(System.String)",
" [AspectMethodReplace(\"System.String::ToUpper(System.Globalization.CultureInfo)\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToUpper(System.String,System.Globalization.CultureInfo)",
" [AspectMethodReplace(\"System.String::ToUpperInvariant()\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToUpperInvariant(System.String)",
Expand Down Expand Up @@ -562,8 +559,6 @@ internal static partial class AspectDefinitions
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.String,System.Object[])\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.String,System.Object[])",
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.Char,System.String[])\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.Char,System.String[])",
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.Char,System.Object[])\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.Char,System.Object[])",
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.Char,System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.Char,System.Object)",
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.String,System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.String,System.Object)",
"[AspectClass(\"mscorlib,netstandard,System.Runtime\",[None],Sink,[ReflectionInjection])] Datadog.Trace.Iast.Aspects.ActivatorAspect",
" [AspectMethodInsertBefore(\"System.Activator::CreateInstance(System.String,System.String)\",\"\",[1,0],[False,False],[None],Default,[])] ReflectionInjectionParam(System.String)",
" [AspectMethodInsertBefore(\"System.Activator::CreateInstance(System.String,System.String,System.Object[])\",\"\",[2,1],[False,False],[None],Default,[])] ReflectionInjectionParam(System.String)",
Expand Down
9 changes: 2 additions & 7 deletions ...app3.1/Datadog.Trace.SourceGenerators/AspectsDefinitionsGenerator/AspectsDefinitions.g.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -447,8 +447,7 @@ internal static partial class AspectDefinitions
" [AspectMethodReplace(\"System.String::Concat(System.String,System.String,System.String,System.String)\",\"\",[0],[False],[StringLiterals],Default,[])] Concat(System.String,System.String,System.String,System.String)",
" [AspectMethodReplace(\"System.String::Concat(System.String[])\",\"\",[0],[False],[None],Default,[])] Concat(System.String[])",
" [AspectMethodReplace(\"System.String::Concat(System.Object[])\",\"\",[0],[False],[None],Default,[])] Concat(System.Object[])",
" [AspectMethodReplace(\"System.String::Concat(System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] Concat(System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Concat(System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] Concat2(System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Concat(System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] Concat(System.Collections.Generic.IEnumerable)",
" [AspectMethodReplace(\"System.String::Substring(System.Int32)\",\"\",[0],[False],[StringLiteral_0],Default,[])] Substring(System.String,System.Int32)",
" [AspectMethodReplace(\"System.String::Substring(System.Int32,System.Int32)\",\"\",[0],[False],[StringLiteral_0],Default,[])] Substring(System.String,System.Int32,System.Int32)",
" [AspectMethodReplace(\"System.String::ToCharArray()\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToCharArray(System.String)",
Expand All @@ -457,11 +456,9 @@ internal static partial class AspectDefinitions
" [AspectMethodReplace(\"System.String::Join(System.Char,System.String[])\",\"\",[0],[False],[None],Default,[])] Join(System.Char,System.String[])",
" [AspectMethodReplace(\"System.String::Join(System.Char,System.Object[])\",\"\",[0],[False],[None],Default,[])] Join(System.Char,System.Object[])",
" [AspectMethodReplace(\"System.String::Join(System.Char,System.String[],System.Int32,System.Int32)\",\"\",[0],[False],[None],Default,[])] Join(System.Char,System.String[],System.Int32,System.Int32)",
" [AspectMethodReplace(\"System.String::Join(System.Char,System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] Join(System.Char,System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Object[])\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.Object[])",
" [AspectMethodReplace(\"System.String::Join(System.String,System.String[])\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.String[])",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] JoinString(System.String,System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] JoinString(System.String,System.Collections.Generic.IEnumerable)",
" [AspectMethodReplace(\"System.String::ToUpper()\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToUpper(System.String)",
" [AspectMethodReplace(\"System.String::ToUpper(System.Globalization.CultureInfo)\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToUpper(System.String,System.Globalization.CultureInfo)",
" [AspectMethodReplace(\"System.String::ToUpperInvariant()\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToUpperInvariant(System.String)",
Expand Down Expand Up @@ -549,8 +546,6 @@ internal static partial class AspectDefinitions
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.String,System.Object[])\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.String,System.Object[])",
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.Char,System.String[])\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.Char,System.String[])",
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.Char,System.Object[])\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.Char,System.Object[])",
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.Char,System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.Char,System.Object)",
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.String,System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.String,System.Object)",
"[AspectClass(\"mscorlib,netstandard,System.Runtime\",[None],Sink,[ReflectionInjection])] Datadog.Trace.Iast.Aspects.ActivatorAspect",
" [AspectMethodInsertBefore(\"System.Activator::CreateInstance(System.String,System.String)\",\"\",[1,0],[False,False],[None],Default,[])] ReflectionInjectionParam(System.String)",
" [AspectMethodInsertBefore(\"System.Activator::CreateInstance(System.String,System.String,System.Object[])\",\"\",[2,1],[False,False],[None],Default,[])] ReflectionInjectionParam(System.String)",
Expand Down
Loading

0 comments on commit 8a6b8e2

Please sign in to comment.