Skip to content

Commit

Permalink
S2222: Branch on createdNew parameter on Mutex constructor (#6994)
Browse files Browse the repository at this point in the history
  • Loading branch information
@martin-strecker-sonarsource @pavel-mikula-sonarsource
martin-strecker-sonarsource authored and pavel-mikula-sonarsource committed Apr 4, 2023
1 parent 06a89e6 commit 03a6186
Show file tree
Hide file tree
Showing 5 changed files with 218 additions and 25 deletions.
33 changes: 33 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 @@ -31,6 +31,7 @@ OperationKindEx.FieldReference when operation.ToFieldReference() is var fieldRef
OperationKindEx.LocalReference => operation.ToLocalReference().Local,
OperationKindEx.ParameterReference => operation.ToParameterReference().Parameter,
OperationKindEx.Argument => operation.ToArgument().Value.TrackedSymbol(),
OperationKindEx.DeclarationExpression => IDeclarationExpressionOperationWrapper.FromOperation(operation).Expression.TrackedSymbol(),
_ => null
};

Expand Down Expand Up @@ -102,6 +103,38 @@ public static IOperation UnwrapConversion(this IOperation operation)
return operation;
}

/// <inheritdoc cref="ArgumentValue(ImmutableArray{IOperation}, string)"/>
public static IOperation ArgumentValue(this IInvocationOperationWrapper invocation, string parameterName) =>
ArgumentValue(invocation.Arguments, parameterName);

/// <inheritdoc cref="ArgumentValue(ImmutableArray{IOperation}, string)"/>
public static IOperation ArgumentValue(this IObjectCreationOperationWrapper objectCreation, string parameterName) =>
ArgumentValue(objectCreation.Arguments, parameterName);

/// <inheritdoc cref="ArgumentValue(ImmutableArray{IOperation}, string)"/>
public static IOperation ArgumentValue(this IPropertyReferenceOperationWrapper propertyReference, string parameterName) =>
ArgumentValue(propertyReference.Arguments, parameterName);

/// <inheritdoc cref="ArgumentValue(ImmutableArray{IOperation}, string)"/>
public static IOperation ArgumentValue(this IRaiseEventOperationWrapper raiseEvent, string parameterName) =>
ArgumentValue(raiseEvent.Arguments, parameterName);

/// <summary>
/// Returns the argument value corresponding to <paramref name="parameterName"/>. For <see langword="params"/> parameter an IArrayCreationOperation is returned.
/// </summary>
private static IOperation ArgumentValue(ImmutableArray<IOperation> arguments, string parameterName)
{
foreach (var operation in arguments)
{
var argument = operation.ToArgument();
if (argument.Parameter.Name == parameterName)
{
return argument.Value;
}
}
return null;
}

private static T? As<T>(this IOperation operation, OperationKind kind, Func<IOperation, T> fromOperation) where T : struct =>
operation.Kind == kind ? fromOperation(operation) : null;
}
Expand Down
37 changes: 21 additions & 16 deletions ...src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/RuleChecks/LocksReleasedAllPathsBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,25 @@ public override ProgramState[] PostProcess(SymbolicContext context)
};
}
}
else if (context.Operation.Instance.AsObjectCreation() is { } objectCreation
&& objectCreation.Type.Is(KnownType.System_Threading_Mutex)
&& context.Operation.Parent.AsAssignment() is { } assignment
&& assignment.Target.TrackedSymbol() is { } symbol
&& objectCreation.ArgumentValue("initiallyOwned") is { } initiallyOwned
&& context.State[initiallyOwned] is { } initiallyOwnedValue
&& initiallyOwnedValue.HasConstraint(BoolConstraint.True))
{
lastSymbolLock[symbol] = objectCreation.ToSonar();
return objectCreation.ArgumentValue("createdNew") is { } createdNew
&& createdNew.TrackedSymbol() is { } trackedCreatedNew
? new[]
{
AddLock(context, objectCreation.WrappedOperation).Preserve(symbol).SetSymbolConstraint(trackedCreatedNew, BoolConstraint.True),
context.SetSymbolConstraint(trackedCreatedNew, BoolConstraint.False),
}
: AddLock(context, objectCreation.WrappedOperation).Preserve(symbol).ToArray();
}

return base.PostProcess(context);
}

Expand Down Expand Up @@ -131,21 +150,7 @@ ProgramState ProcessCondition(ISymbol lockSymbol)

protected override ProgramState PostProcessSimple(SymbolicContext context)
{
if (context.Operation.Instance.AsObjectCreation() is { } objectCreation)
{
if (objectCreation.Type.Is(KnownType.System_Threading_Mutex)
&& context.Operation.Parent.AsAssignment() is { } assignment
&& objectCreation.Arguments.Length > 0
&& objectCreation.Arguments.First().ToArgument().Value is { } firstArgument
&& context.State[firstArgument] is { } firstArgumentValue
&& firstArgumentValue.HasConstraint(BoolConstraint.True)
&& assignment.Target.TrackedSymbol() is { } symbol)
{
lastSymbolLock[symbol] = objectCreation.ToSonar();
return AddLock(context, objectCreation.WrappedOperation).Preserve(symbol);
}
}
else if (context.Operation.Instance.AsInvocation() is { } invocation)
if (context.Operation.Instance.AsInvocation() is { } invocation)
{
// ToDo: we ignore the number of parameters for now.
if (invocation.TargetMethod.IsAny(KnownType.System_Threading_Monitor, "Enter", "TryEnter"))
Expand Down Expand Up @@ -191,7 +196,7 @@ private ProgramState AddLock(SymbolicContext context, ISymbol symbol)
}

lastSymbolLock[symbol] = context.Operation;
return context.SetSymbolConstraint(symbol, LockConstraint.Held).Preserve(symbol);
return context.SetSymbolConstraint(symbol, LockConstraint.Held).SetSymbolConstraint(symbol, ObjectConstraint.NotNull).Preserve(symbol);
}

private ProgramState RemoveLock(SymbolicContext context, ISymbol symbol)
Expand Down
130 changes: 130 additions & 0 deletions analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/IOperationExtensionsTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Operations;
using SonarAnalyzer.SymbolicExecution.Roslyn;
using StyleCop.Analyzers.Lightup;
Expand Down Expand Up @@ -59,11 +60,140 @@ public void TrackedSymbol_FieldReference_IsFieldSymbol(string assignment)
assignmentTarget.TrackedSymbol().Should().Be(fieldReferenceSymbol);
}

[DataTestMethod]
[DataRow(@"(int i, int j) = (1, 1)")]
[DataRow(@"(var i, var j) = (1, 1)")]
[DataRow(@"int.TryParse(string.Empty, out int value)")]
[DataRow(@"int.TryParse(string.Empty, out var value)")]
public void TrackedSymbol_DeclarationExpression(string assignment)
{
var code = $"public class C {{ void Method() {{ {assignment}; }} }}";
var graph = TestHelper.CompileCfgCS(code);
var allDeclarations = graph.Blocks[1].Operations.SelectMany(x => x.DescendantsAndSelf()).Where(x => x.Kind == OperationKindEx.DeclarationExpression).Select(IDeclarationExpressionOperationWrapper.FromOperation).ToArray();
allDeclarations.Should().NotBeEmpty();
allDeclarations.Should().AllSatisfy(x =>
x.WrappedOperation.TrackedSymbol().Should().NotBeNull().And.BeAssignableTo<ISymbol>()
.Which.GetSymbolType().Should().NotBeNull().And.BeAssignableTo<ITypeSymbol>()
.Which.SpecialType.Should().Be(SpecialType.System_Int32));
}

[TestMethod]
public void TrackedSymbol_DeclarationExpression_Tuple()
{
var code = $"public class C {{ void Method() {{ var (i, j) = (1, 1); }} }}";
var graph = TestHelper.CompileCfgCS(code);
var allDeclarations = graph.Blocks[1].Operations.SelectMany(x => x.DescendantsAndSelf()).Where(x => x.Kind == OperationKindEx.DeclarationExpression).Select(IDeclarationExpressionOperationWrapper.FromOperation).ToArray();
var declaration = allDeclarations.Should().ContainSingle().Which.WrappedOperation;
declaration.Kind.Should().Be(OperationKindEx.DeclarationExpression);
var declarationExpression = IDeclarationExpressionOperationWrapper.FromOperation(declaration).Expression;
declarationExpression.Kind.Should().Be(OperationKindEx.Tuple);
declaration.TrackedSymbol().Should().BeNull();
}

[TestMethod]
public void TrackedSymbol_SimpleAssignment_IsNull()
{
var simpleAssignment = TestHelper.CompileCfgBodyCS("var a = true; bool b; b = a;").Blocks[1].Operations[0];
simpleAssignment.TrackedSymbol().Should().BeNull();
}

[DataTestMethod]
[DataRow("""1, "Test" """, "intParam", 1)]
[DataRow("""1, "Test" """, "stringParam", "Test")]
[DataRow("""1, "Test" """, "optionalBoolParam", true)]
[DataRow("""1, "Test" """, "optionalIntParam", 1)]
[DataRow("""1, "Test", true """, "optionalBoolParam", true)]
[DataRow("""stringParam: "Test", intParam: 1 """, "stringParam", "Test")]
[DataRow("""stringParam: "Test", intParam: 1, optionalIntParam: 2 """, "optionalIntParam", 2)]
public void ArgumentValue_ObjectCreation(string objectCreationArguments, string parameterName, object expected)
{
var testClass = $$"""
class Test
{
Test(int intParam, string stringParam, bool optionalBoolParam = true, int optionalIntParam = 1) { }
static void Create() =>
new Test({{objectCreationArguments}});
}
""";
var (tree, model) = TestHelper.CompileCS(testClass);
var objectCreation = tree.GetRoot().DescendantNodesAndSelf().OfType<ObjectCreationExpressionSyntax>().Single();
var operation = IObjectCreationOperationWrapper.FromOperation(model.GetOperation(objectCreation));
operation.ArgumentValue(parameterName).Should().NotBeNull().And.BeAssignableTo<IOperation>().Which.ConstantValue.Value.Should().Be(expected);
}

[DataTestMethod]
[DataRow(""" """)]
[DataRow(""" "param1", "param2" """, "param1", "param2")]
[DataRow(""" null, null """, null, null)]
[DataRow(""" new[] {"param1", "param2"} """, "param1", "param2")]
public void ArgumentValue_ObjectCreation_Params(string arguments, params string[] expected)
{
var testClass = $$"""
class Test
{
Test(params string[] stringParams) { }
static void Create() =>
new Test({{arguments}});
}
""";
var (tree, model) = TestHelper.CompileCS(testClass);
var objectCreation = tree.GetRoot().DescendantNodesAndSelf().OfType<ObjectCreationExpressionSyntax>().Single();
var operation = IObjectCreationOperationWrapper.FromOperation(model.GetOperation(objectCreation));
var argument = operation.ArgumentValue("stringParams").Should().NotBeNull().And.BeAssignableTo<IOperation>().Subject;
var argumentArray = IArrayCreationOperationWrapper.FromOperation(argument);
var result = argumentArray.Initializer.ElementValues.Select(x => x.ConstantValue.Value).ToArray();
result.Should().BeEquivalentTo(expected);
}

[TestMethod]
public void ArgumentValue_Invocation()
{
const string testClass = """
class Test
{
static void M(string stringParam) => M("param");
}
""";
var (tree, model) = TestHelper.CompileCS(testClass);
var invocation = tree.GetRoot().DescendantNodesAndSelf().OfType<InvocationExpressionSyntax>().Single();
var operation = IInvocationOperationWrapper.FromOperation(model.GetOperation(invocation));
operation.ArgumentValue("stringParam").Should().NotBeNull().And.BeAssignableTo<IOperation>().Which.ConstantValue.Value.Should().Be("param");
}

[TestMethod]
public void ArgumentValue_PropertyReference()
{
const string testClass = """
class Test
{
int this[int index] => this[1];
}
""";
var (tree, model) = TestHelper.CompileCS(testClass);
var elementAccess = tree.GetRoot().DescendantNodesAndSelf().OfType<ElementAccessExpressionSyntax>().Single();
var operation = IPropertyReferenceOperationWrapper.FromOperation(model.GetOperation(elementAccess));
operation.ArgumentValue("index").Should().NotBeNull().And.BeAssignableTo<IOperation>().Which.ConstantValue.Value.Should().Be(1);
}

[TestMethod]
public void ArgumentValue_RaiseEvent()
{
const string testClass = """
Imports System
Public Class C
Event SomeEvent As EventHandler
Public Sub M()
RaiseEvent SomeEvent(Nothing, EventArgs.Empty)
End Sub
End Class
""";
var (tree, model) = TestHelper.CompileVB(testClass);
var raiseEvent = tree.GetRoot().DescendantNodesAndSelf().OfType<Microsoft.CodeAnalysis.VisualBasic.Syntax.RaiseEventStatementSyntax>().Single();
var operation = IRaiseEventOperationWrapper.FromOperation(model.GetOperation(raiseEvent));
operation.ArgumentValue("sender").Should().NotBeNull().And.BeAssignableTo<IOperation>().Which.ConstantValue.Value.Should().BeNull();
operation.ArgumentValue("e").Should().NotBeNull().And.BeAssignableTo<IOperation>().Which.Kind.Should().Be(OperationKindEx.FieldReference);
}
}
}
2 changes: 1 addition & 1 deletion ...Analyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/LocksReleasedAllPaths.Mutex.NetFx.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public static void CompliantFromDocs(string mutexName, MutexSecurity mutexSecuri
// Enter the mutex, and hold it until the program exits.
try
{
m.WaitOne(); // Noncompliant FP
m.WaitOne();
}
catch (UnauthorizedAccessException ex)
{
Expand Down
41 changes: 33 additions & 8 deletions .../SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/LocksReleasedAllPaths.Mutex.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ public void Noncompliant(Foo foo)
var m3 = Mutex.OpenExisting("x");
m3.WaitOne(); // Noncompliant

var m4 = new Mutex();

foo.instanceMutex.WaitOne(); // FN

Foo.staticMutex.WaitOne(); // Noncompliant
Expand All @@ -35,7 +33,6 @@ public void Noncompliant(Foo foo)
m1.ReleaseMutex();
m2.ReleaseMutex();
m3.ReleaseMutex();
m4.ReleaseMutex();
foo.instanceMutex.ReleaseMutex();
Foo.staticMutex.ReleaseMutex();
}
Expand All @@ -47,6 +44,34 @@ public void Noncompliant(Foo foo)
m3.Dispose();
}

public void MutexWasCreatedAndReleased(bool arg)
{
var m = new Mutex(initiallyOwned: true, "bar", out var wasCreated); // Noncompliant
if (wasCreated && arg)
{
m.ReleaseMutex();
}
}

public void MutexWasCreatedAndNotReleased(bool arg)
{
var m = new Mutex(initiallyOwned: true, "bar", out var wasCreated); // Compliant
if (!wasCreated && arg)
{
// wasCreated is false here, indicating that the lock is NOT (yet) held
m.ReleaseMutex(); // This is a release without a previous lock. This is not covered by this rule.
}
}

public void MutexWasCreatedWithoutOwnership(bool arg)
{
var m = new Mutex(initiallyOwned: false, "bar", out var wasCreated); // Compliant. The lock must still be acquired by requesting it.
if (wasCreated && arg)
{
m.ReleaseMutex();
}
}

public void Noncompliant2(Mutex paramMutex, Mutex paramMutex2)
{
// 'true' means it owns the mutex if no exception gets thrown
Expand Down Expand Up @@ -339,11 +364,11 @@ public void MutextAquireByConstructor_MultipleVariableDeclaration()
public void MutextAquireByConstructor_SwitchExpression(int x)
{
var m = x switch
{
1 => new Mutex(),
2 => new Mutex(new bool()),
3 => new Mutex(true, "bar", out var mutextCreated), // FN
};
{
1 => new Mutex(),
2 => new Mutex(new bool()),
3 => new Mutex(true, "bar", out var mutextCreated), // FN
};

if (cond)
{
Expand Down

0 comments on commit 03a6186

Please sign in to comment.