Skip to content

Commit

Permalink
S3900: Basic rule implementation (#6969)
Browse files Browse the repository at this point in the history
  • Loading branch information
@zsolt-kolbay-sonarsource
zsolt-kolbay-sonarsource authored Mar 27, 2023
1 parent 4f63200 commit 7e9dccf
Show file tree
Hide file tree
Showing 6 changed files with 118 additions and 73 deletions.
39 changes: 37 additions & 2 deletions ...arAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ObjectConstraint>();
}

private static bool IsParameterDereferenced(IOperationWrapperSonar operation) =>
operation.Parent != null
&& operation.Parent.IsAnyKind(
OperationKindEx.Invocation,
OperationKindEx.FieldReference,
OperationKindEx.PropertyReference,
OperationKindEx.EventReference,
OperationKindEx.Await,
OperationKindEx.ArrayElementReference);
}
3 changes: 3 additions & 0 deletions ...zers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/Extensions/IOperationExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
17 changes: 12 additions & 5 deletions ...estCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp11.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
}
Expand All @@ -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.}}
}
}
38 changes: 19 additions & 19 deletions ...TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp8.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand All @@ -20,7 +20,7 @@ public interface IWithDefaultMembers

void Reset(string s)
{
s.ToString(); // FIXME Non-compliant
s.ToString(); // Noncompliant
}
}

Expand All @@ -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();
}
}
}
Expand Down Expand Up @@ -66,7 +66,7 @@ public void OnlyDiscardBranch_Noncompliant(string s, bool b)
{
var result = b switch
{
_ => s.ToString() // FIXME Non-compliant
_ => s.ToString() // Noncompliant
};
}

Expand All @@ -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"
};
}
Expand All @@ -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"
};
Expand All @@ -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()
};
}

Expand All @@ -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
};
}
Expand All @@ -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
};
}
Expand All @@ -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
};
}
Expand All @@ -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
};
}
Expand All @@ -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
};
}
Expand All @@ -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
};
}
Expand All @@ -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
};
}
}
Expand All @@ -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;
}
}
Expand Down
24 changes: 12 additions & 12 deletions ...TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp9.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand All @@ -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();
}
}

Expand All @@ -63,7 +63,7 @@ public object PropertySet
get => null;
set
{
field = value.ToString(); // FIXME Non-compliant
field = value.ToString(); // Noncompliant
}
}

Expand All @@ -72,7 +72,7 @@ public object PropertyInit
get => null;
init
{
field = value.ToString(); // FIXME Non-compliant
field = value.ToString(); // Noncompliant
}
}
}
Expand All @@ -81,7 +81,7 @@ public record Record
{
public void Method(object arg)
{
arg.ToString(); // FIXME Non-compliant
arg.ToString(); // Noncompliant
}
}

Expand All @@ -94,7 +94,7 @@ public partial class Partial
{
public partial void Method(object arg)
{
arg.ToString(); // FIXME Non-compliant
arg.ToString(); // Noncompliant
}
}

Expand All @@ -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
{
Expand Down
Loading

0 comments on commit 7e9dccf

Please sign in to comment.