From ecef0fdc68b0446fcb4b3d1ac84b3421b9d6452f Mon Sep 17 00:00:00 2001 From: Zsolt Kolbay <121798625+zsolt-kolbay-sonarsource@users.noreply.github.com> Date: Mon, 27 Mar 2023 12:31:37 +0200 Subject: [PATCH] S3900: Basic rule implementation (#6969) --- ...icMethodArgumentsShouldBeCheckedForNull.cs | 39 ++++++++++- .../Roslyn/Extensions/IOperationExtensions.cs | 3 + ...rgumentsShouldBeCheckedForNull.CSharp11.cs | 17 +++-- ...ArgumentsShouldBeCheckedForNull.CSharp8.cs | 38 +++++----- ...ArgumentsShouldBeCheckedForNull.CSharp9.cs | 24 +++---- ...icMethodArgumentsShouldBeCheckedForNull.cs | 70 +++++++++---------- 6 files changed, 118 insertions(+), 73 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs b/analyzers/src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs index c618337cc7e..af666a2a958 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs @@ -18,16 +18,51 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ +using SonarAnalyzer.SymbolicExecution.Constraints; + namespace SonarAnalyzer.SymbolicExecution.Roslyn.RuleChecks.CSharp; public class PublicMethodArgumentsShouldBeCheckedForNull : SymbolicRuleCheck { private const string DiagnosticId = "S3900"; - private const string MessageFormat = "Refactor this method to add validation of parameter '{0}' before using it."; + private const string MessageFormat = "{0}"; internal static readonly DiagnosticDescriptor S3900 = DescriptorFactory.Create(DiagnosticId, MessageFormat); protected override DiagnosticDescriptor Rule => S3900; - public override bool ShouldExecute() => false; + public override bool ShouldExecute() => + Node is BaseMethodDeclarationSyntax or AccessorDeclarationSyntax; + + protected override ProgramState PreProcessSimple(SymbolicContext context) + { + var operation = context.Operation.Instance; + if (operation.Kind == OperationKindEx.ParameterReference + && operation.ToParameterReference().Parameter is var parameter + && !parameter.Type.IsValueType + && IsParameterDereferenced(context.Operation) + && NullableStateIsNotKnownForParameter(parameter) + && !parameter.HasAttribute(KnownType.Microsoft_AspNetCore_Mvc_FromServicesAttribute)) + { + var message = SemanticModel.GetDeclaredSymbol(Node).IsConstructor() + ? "Refactor this constructor to avoid using members of parameter '{0}' because it could be null." + : "Refactor this method to add validation of parameter '{0}' before using it."; + ReportIssue(operation, string.Format(message, operation.Syntax), context); + } + + return context.State; + + bool NullableStateIsNotKnownForParameter(IParameterSymbol symbol) => + context.State[symbol] is null || !context.State[symbol].HasConstraint(); + } + + private static bool IsParameterDereferenced(IOperationWrapperSonar operation) => + operation.Parent != null + && operation.Parent.IsAnyKind( + OperationKindEx.Invocation, + OperationKindEx.FieldReference, + OperationKindEx.PropertyReference, + OperationKindEx.EventReference, + OperationKindEx.Await, + OperationKindEx.ArrayElementReference); } diff --git a/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/Extensions/IOperationExtensions.cs b/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/Extensions/IOperationExtensions.cs index 320025fc952..d4ef7dd7d5b 100644 --- a/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/Extensions/IOperationExtensions.cs +++ b/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/Extensions/IOperationExtensions.cs @@ -69,6 +69,9 @@ internal static IFieldReferenceOperationWrapper ToFieldReference(this IOperation internal static IPropertyReferenceOperationWrapper ToPropertyReference(this IOperation operation) => IPropertyReferenceOperationWrapper.FromOperation(operation); + internal static IParameterReferenceOperationWrapper ToParameterReference(this IOperation operation) => + IParameterReferenceOperationWrapper.FromOperation(operation); + internal static IEventReferenceOperationWrapper ToEventReference(this IOperation operation) => IEventReferenceOperationWrapper.FromOperation(operation); diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp11.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp11.cs index ae2906ff552..c2a73fa26fc 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp11.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp11.cs @@ -5,15 +5,22 @@ public void NotCompliantCases(object[] o) { - o[1].ToString(); // FIXME Non-compliant {{Refactor this method to add validation of parameter 'o' before using it.}} + o[1].ToString(); // Noncompliant {{Refactor this method to add validation of parameter 'o' before using it.}} } - public void Compliant(object[] o) + public void ListPattern1(object[] o) { if (o is [not null, not null]) { - o.ToString(); // Compliant - o[1].ToString(); // Compliant + o.ToString(); // Noncompliant - FP + } + } + + public void ListPattern2(object[] o) + { + if (o is [not null, not null]) + { + o[1].ToString(); // Noncompliant - FP } } } @@ -22,6 +29,6 @@ public interface ISomeInterface { public static virtual void NotCompliantCases(object o) { - o.ToString(); // FIXME Non-compliant {{Refactor this method to add validation of parameter 'o' before using it.}} + o.ToString(); // Noncompliant {{Refactor this method to add validation of parameter 'o' before using it.}} } } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp8.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp8.cs index 6e97a7a762d..33651b4f160 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp8.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp8.cs @@ -9,7 +9,7 @@ public void NullCoalescenceAssignment_NotNull(string s1) public void NullCoalescenceAssignment_Null(string s1) { s1 ??= null; - s1.ToString(); // FN + s1.ToString(); // Covered by S2259 } } @@ -20,7 +20,7 @@ public interface IWithDefaultMembers void Reset(string s) { - s.ToString(); // FIXME Non-compliant + s.ToString(); // Noncompliant } } @@ -30,12 +30,12 @@ public void Method(object arg) { string LocalFunction(object o) { - return o.ToString(); // FN: local functions are not supported by the CFG + return o.ToString(); // Compliant - local methods are not accessible from other assemblies } static string LocalStaticFunction(object o) { - return o.ToString(); // FN: local functions are not supported by the CFG + return o.ToString(); } } } @@ -66,7 +66,7 @@ public void OnlyDiscardBranch_Noncompliant(string s, bool b) { var result = b switch { - _ => s.ToString() // FIXME Non-compliant + _ => s.ToString() // Noncompliant }; } @@ -75,7 +75,7 @@ public void MultipleBranches_Noncompliant(string s, int val) var result = val switch { 1 => "a", - 2 => s.ToString(), // FIXME Non-compliant + 2 => s.ToString(), // Noncompliant _ => "b" }; } @@ -87,7 +87,7 @@ public void Nested_Noncompliant(string s, int val, bool condition) 1 => "a", 2 => condition switch { - _ => s.ToString() // FIXME Non-compliant + _ => s.ToString() // Noncompliant }, _ => "b" }; @@ -97,8 +97,8 @@ public void MultipleBranches_HandleNull(string s, int val) { var result = s switch { - null => s.ToString(), // FIXME Non-compliant - _ => s.ToString() // Compliant as null was already handled + null => s.ToString(), // Covered by S2259 + _ => s.ToString() }; } @@ -116,7 +116,7 @@ public string MultipleBranches_PropertyPattern(Address address, string s) { return address switch { - { State: "WA" } addr => s.ToString(), // FIXME Non-compliant + { State: "WA" } addr => s.ToString(), // Noncompliant _ => string.Empty }; } @@ -125,7 +125,7 @@ public string MultipleBranches_PropertyPattern_FP(string s) { return s switch { - { Length: 5 } => s.ToString(), // FIXME Non-compliant - FP we know that the length is 5 so the string cannot be null + { Length: 5 } => s.ToString(), // Compliant - we know that the length is 5 so the string cannot be null _ => string.Empty }; } @@ -134,7 +134,7 @@ public string MultipleBranches_RecursivePattern(Person person, string s) { return person switch { - { Address: { State: "WA" } } pers => s.ToString(), // FIXME Non-compliant + { Address: { State: "WA" } } pers => s.ToString(), // Noncompliant _ => string.Empty }; } @@ -143,7 +143,7 @@ public string MultipleBranches_TuplePattern(Address address, string s) { return address switch { - var (name, state) => s.ToString(), // FN + var (name, state) => s.ToString(), // Noncompliant _ => string.Empty }; } @@ -152,8 +152,8 @@ public string MultipleBranches_WhenClause(Address address, string s) { return address switch { - Address addr when addr.Name.Length > 0 => s.ToString(), // FIXME Non-compliant - Address addr when addr.Name.Length == s.Length => string.Empty, // FIXME Non-compliant + Address addr when addr.Name.Length > 0 => s.ToString(), // Noncompliant + Address addr when addr.Name.Length == s.Length => string.Empty, // Noncompliant _ => string.Empty }; } @@ -162,7 +162,7 @@ public string MultipleBranches_VarDeclaration(Address address, string s) { return address switch { - Address addr => s.ToString(), // FIXME Non-compliant + Address addr => s.ToString(), // Noncompliant _ => string.Empty }; } @@ -171,8 +171,8 @@ public string TwoBranches_NoDefault(bool condition, string s) { return condition switch { - true => s.ToString(), // FIXME Non-compliant - false => s.ToString() // FIXME Non-compliant + true => s.ToString(), // Noncompliant + false => s.ToString() // Noncompliant }; } } @@ -187,7 +187,7 @@ public void Test(string s) break; default: - s.ToString(); // Compliant - the null is handled by the case null branch. + s.ToString(); // Noncompliant - FP break; } } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp9.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp9.cs index bd2931c42bc..f49a383b604 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp9.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp9.cs @@ -9,7 +9,7 @@ public class Sample public void TargetTypedNew(object arg) { - arg.ToString(); // FN, can't build CFG for this method + arg.ToString(); // Noncompliant StringBuilder sb = new(); } @@ -23,29 +23,29 @@ public void PatternMatching(object arg) if (arg is int and > 0 and > 1) { - arg.ToString(); // Compliant + arg.ToString(); // Noncompliant - FP } if (arg is int or bool or long) { - arg.ToString(); // Compliant + arg.ToString(); // Noncompliant - FP } if (arg is null) { - arg.ToString(); // FN + arg.ToString(); // Covered by S2259 } if (arg is int or bool or null) { - arg.ToString(); // FN + arg.ToString(); } else if (arg is not not null) { - arg.ToString(); // FN + arg.ToString(); } else if (!(arg is not null)) { - arg.ToString(); // FN + arg.ToString(); } } @@ -63,7 +63,7 @@ public object PropertySet get => null; set { - field = value.ToString(); // FIXME Non-compliant + field = value.ToString(); // Noncompliant } } @@ -72,7 +72,7 @@ public object PropertyInit get => null; init { - field = value.ToString(); // FIXME Non-compliant + field = value.ToString(); // Noncompliant } } } @@ -81,7 +81,7 @@ public record Record { public void Method(object arg) { - arg.ToString(); // FIXME Non-compliant + arg.ToString(); // Noncompliant } } @@ -94,7 +94,7 @@ public partial class Partial { public partial void Method(object arg) { - arg.ToString(); // FIXME Non-compliant + arg.ToString(); // Noncompliant } } @@ -107,7 +107,7 @@ public int GetProductMultipleAttr([FromServices][FromRoute] IService service) => service.GetValue(); // Compliant, it's attributed with FromServices attribute public int GetPrice(IService service) => - service.GetValue(); // FIXME Non-compliant + service.GetValue(); // Noncompliant public interface IService { diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs index 66a5930c04c..52f92e1759a 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs @@ -9,34 +9,33 @@ public class Program public void NotCompliantCases(object o, Exception e) { - o.ToString(); // FIXME Non-compliant {{Refactor this method to add validation of parameter 'o' before using it.}} -// ~~~~~~~~~~ + o.ToString(); // Noncompliant {{Refactor this method to add validation of parameter 'o' before using it.}} +// ^ Bar(o); // Compliant, we care about dereference only - throw e; // FIXME Non-compliant - attempting to throw null as an exception will result in a NullReferenceException -// ~~~~~~~ + throw e; // FN - the SE engine uses the throw statement as branch rather than an operation } public void Bar(object o) { } protected void NotCompliantCases_Protected(object o) { - o.ToString(); // FIXME Non-compliant + o.ToString(); // Noncompliant } private void CompliantCases_Private(object o) { - o.ToString(); // Compliant, not public + o.ToString(); // Noncompliant - FP (method visibility) } protected internal void NotCompliantCases_ProtectedInternal(object o) { - o.ToString(); // FIXME Non-compliant + o.ToString(); // Noncompliant } internal void CompliantCases_Internal(object o) { - o.ToString(); // Compliant, not public + o.ToString(); // Noncompliant - FP (method visibility) } public void CompliantCases(bool b, object o1, object o2, object o3, object o4, Exception e) @@ -47,7 +46,7 @@ public void CompliantCases(bool b, object o1, object o2, object o3, object o4, E } o2 = o2 ?? new object(); - o2.ToString(); // Compliant, we coalesce + o2.ToString(); // Noncompliant - FP if (o3 == null) { @@ -77,20 +76,20 @@ public void MoreCompliantCases(string s1, string s2) { if (string.IsNullOrEmpty(s1)) { - s1.ToString(); // FIXME Non-compliant, could be null + s1.ToString(); // Null check was performed, so this belongs to S2259 } else { - s1.ToString(); // Compliant + s1.ToString(); } if (string.IsNullOrWhiteSpace(s2)) { - s2.ToString(); // FIXME Non-compliant, could be null + s2.ToString(); // Null check was performed, so this belongs to S2259 } else { - s2.ToString(); // Compliant + s2.ToString(); } } @@ -101,26 +100,26 @@ public async void AsyncTest(Task task1, Task task2, Task task3, Task task4) await task1; } - await task2; // FIXME Non-compliant + await task2; // Noncompliant if (task3 != null) { await task3.ConfigureAwait(false); } - await task4.ConfigureAwait(false); // FIXME Non-compliant + await task4.ConfigureAwait(false); // Noncompliant } public Program(int i) { } - public Program(string s) : this(s.Length) { } // FIXME Non-compliant {{Refactor this constructor to avoid using members of parameter 's' because it could be null.}} + public Program(string s) : this(s.Length) { } // Noncompliant {{Refactor this constructor to avoid using members of parameter 's' because it could be null.}} public void NonCompliant1(object o) { var c = o?.ToString()?.IsNormalized(); if (c == null) { - o.GetType().GetMethods(); // FIXME Non-compliant + o.GetType().GetMethods(); // Null check was performed, so this belongs to S2259 } } @@ -195,18 +194,19 @@ public void Foo(params string[] infixes) } else { - Array.Resize(ref infixes, infixes.Length + 1); + RefMethod(ref infixes, infixes.Length); // Noncompliant - FP: infixes is not null at this point } - // more stuff } + private void RefMethod(ref string[] array, int num) { } + public void Method(ref string s, int x) { } public void Method1(string infixes) { if (infixes != null) { - Method(ref infixes, infixes.Length); - var x = infixes.Length; // FIXME Non-compliant when passed by ref can be set to null + Method(ref infixes, infixes.Length); // Noncompliant - FP + var x = infixes.Length; } } @@ -215,15 +215,15 @@ public void Method2(string infixes) { if (infixes == null) { - Method(ref infixes, infixes.Length); // FIXME Non-compliant + Method(ref infixes, infixes.Length); // Noncompliant var x = infixes.Length; } } public void Method3(string infixes) { - Method(ref infixes, infixes.Length); // FIXME Non-compliant - var x = infixes.Length; // FIXME Non-compliant + Method(ref infixes, infixes.Length); // Noncompliant + var x = infixes.Length; } public void Method4(string contentType) @@ -253,7 +253,7 @@ public void Method5(string contentType) contentType = ""; } - var parts = contentType.Split('/', ';'); + var parts = contentType.Split('/', ';'); // Covered by S2259 } public void Method6(string contentType) @@ -268,7 +268,7 @@ public void Method6(string contentType) contentType = null; } - var parts = contentType.Split('/', ';'); // FIXME Non-compliant + var parts = contentType.Split('/', ';'); // Covered by S2259 } } @@ -282,7 +282,7 @@ public string FooWithStringTrim(string name) name = Guid.NewGuid().ToString("N"); } - return name.Trim(); // FIXME Non-compliant FP + return name.Trim(); // Noncompliant - FP } public string FooWithStringJoin(string name) @@ -292,7 +292,7 @@ public string FooWithStringJoin(string name) name = Guid.NewGuid().ToString("N"); } - return string.Join("_", name.Split(System.IO.Path.GetInvalidFileNameChars())); // FIXME Non-compliant FP + return string.Join("_", name.Split(System.IO.Path.GetInvalidFileNameChars())); // Noncompliant - FP } public string FooWithObject(object name) @@ -302,7 +302,7 @@ public string FooWithObject(object name) name = Guid.NewGuid().ToString("N"); } - return name.ToString(); // FIXME Non-compliant FP + return name.ToString(); // Noncompliant - FP } } @@ -317,7 +317,7 @@ public static void BooleanEqualityComparisonIsNullOrEmpty(string argument, bool } if (b) { - int index = argument.LastIndexOf('c'); // FIXME Non-compliant FP + int index = argument.LastIndexOf('c'); } } @@ -329,7 +329,7 @@ public static void BooleanEqualityComparisonWithIsNullOrWhiteSpace(string argume } if (b) { - int index = argument.LastIndexOf('c'); // FIXME Non-compliant FP + int index = argument.LastIndexOf('c'); } } @@ -351,7 +351,7 @@ public class ReproWithIs { public override bool Equals(object obj) { - var equals = (obj is string) && (obj.GetHashCode() == GetHashCode()); // FIXME Non-compliant FP + var equals = (obj is string) && (obj.GetHashCode() == GetHashCode()); if (equals) { // do stuff @@ -370,7 +370,7 @@ public static string SanitiseIMDbId(string imdbId) if (imdbId.StartsWith(" ")) imdbId = string.Concat("tt", imdbId); - if (imdbId.Length != 9) // FIXME Non-compliant - FP + if (imdbId.Length != 9) // Noncompliant - FP imdbId = string.Empty; return imdbId; @@ -383,7 +383,7 @@ public class Repro_3400 public void ReassignedFromMethod(StringBuilder parameter) { parameter = Create(); - parameter.Capacity = 1; // FIXME Non-compliant FP + parameter.Capacity = 1; // Noncompliant - FP } public void ReassignedFromConstructor(StringBuilder parameter) @@ -395,7 +395,7 @@ public void ReassignedFromConstructor(StringBuilder parameter) public void ReassignedFromMethodOut(out StringBuilder parameter) { parameter = Create(); - parameter.Capacity = 1; // FIXME Non-compliant FP + parameter.Capacity = 1; // Noncompliant - FP } public void ReassignedFromConstructorOut(out StringBuilder parameter)