Skip to content

Commit

Permalink
Fix S2583 FP: nested try catch blocks (#8484)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Tim-Pohlmann
Tim-Pohlmann committed Dec 20, 2023
1 parent 98ea39c commit a314cfc
Show file tree
Hide file tree
Showing 6 changed files with 155 additions and 68 deletions.
31 changes: 20 additions & 11 deletions analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/RoslynSymbolicExecution.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,27 +230,38 @@ private IEnumerable<ExplodedNode> ProcessOperation(ExplodedNode node)
}
}

private IEnumerable<ExplodedNode> ExceptionSuccessors(ExplodedNode node, ExceptionState exception, ControlFlowRegion tryRegion)
private IEnumerable<ExplodedNode> ExceptionSuccessors(ExplodedNode node, ExceptionState exception, ControlFlowRegion nearestTryRegion)
{
var state = node.State.ResetOperations(); // We're jumping out of the outer statement => We need to reset operation states.
state = node.Block.EnclosingRegion(ControlFlowRegionKind.Catch) is not null
? state.PushException(exception) // If we're nested inside catch, we need to preserve the exception chain
: state.SetException(exception); // Otherwise we track only the current exception to avoid explosion of states after each statement
while (tryRegion is not null)

foreach (var handler in ReachableHandlers(nearestTryRegion))
{
var reachableHandlers = tryRegion.EnclosingRegion.NestedRegions.Where(x => x.Kind != ControlFlowRegionKind.Try && IsReachable(x, state.Exception)).ToArray();
foreach (var handler in reachableHandlers) // CatchRegion, FinallyRegion or FilterAndHandlerRegion
{
yield return new(cfg.Blocks[handler.FirstBlockOrdinal], state, null);
}
if (reachableHandlers.Length != 0)
yield return new(cfg.Blocks[handler.FirstBlockOrdinal], state, null);
if (IsExhaustive(handler))
{
yield break;
}
tryRegion = tryRegion.EnclosingRegion.EnclosingRegionOrSelf(ControlFlowRegionKind.Try); // Inner catch for specific exception doesn't match. Go to outer one.
}

yield return new(cfg.ExitBlock, node.State.SetException(exception), null); // catch without finally or uncaught exception type

IEnumerable<ControlFlowRegion> ReachableHandlers(ControlFlowRegion tryRegion) =>
tryRegion is null
? Enumerable.Empty<ControlFlowRegion>()
: tryRegion.EnclosingRegion.NestedRegions.Where(x => x.Kind != ControlFlowRegionKind.Try && IsReachable(x, exception))
.Concat(ReachableHandlers(tryRegion.EnclosingRegion.EnclosingRegionOrSelf(ControlFlowRegionKind.Try))); // Use also outer candidates for nested try/catch.

bool IsExhaustive(ControlFlowRegion handler) =>
handler.Kind switch
{
ControlFlowRegionKind.Finally => true,
ControlFlowRegionKind.FilterAndHandler => false,
_ => exception.Type.DerivesFrom(handler.ExceptionType)
|| handler.ExceptionType.IsAny(KnownType.System_Exception, KnownType.System_Object) // relevant for UnkonwnException: 'catch (Exception) { ... }' and 'catch { ... }'
};
}

private static ExceptionState ThrownException(ExplodedNode node, ControlFlowBranchSemantics semantics) =>
Expand Down Expand Up @@ -297,7 +308,5 @@ private static ProgramState InitLocals(ControlFlowBranch branch, ProgramState st
private static bool IsReachable(ControlFlowRegion region, ExceptionState thrown) =>
region.Kind == ControlFlowRegionKind.Finally
|| thrown == ExceptionState.UnknownException
|| region.ExceptionType.Is(KnownType.System_Object) // catch when (condition)
|| region.ExceptionType.Is(KnownType.System_Exception)
|| thrown.Type.DerivesFrom(region.ExceptionType);
}
50 changes: 43 additions & 7 deletions ...ests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Throw.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,14 +191,12 @@ public void Throw_TryCatch_ThrowInTry_MultipleCatchBlocks()
"InTry",
"InFirstCatch", // With Exception thrown by Tag("InTry")
"InSecondCatch", // With Exception thrown by Tag("InTry")
"InThirdCatch", // With Exception thrown by Tag("InTry")
"InSecondCatch", // With Exception thrown by throw new System.Exception()
"InThirdCatch", // With Exception thrown by throw new System.Exception()
"AfterCatch");

validator.TagStates("InFirstCatch").Should().HaveCount(1).And.ContainSingle(x => HasUnknownException(x));
ValidateHasOnlyUnknownExceptionAndSystemException(validator, "InSecondCatch");
ValidateHasOnlyUnknownExceptionAndSystemException(validator, "InThirdCatch");
validator.TagStates("InThirdCatch").Should().BeEmpty();
validator.TagStates("AfterCatch").Should().HaveCount(1).And.ContainSingle(x => HasNoException(x));
}

Expand Down Expand Up @@ -535,9 +533,8 @@ public void Throw_TryCatch_Throw_CatchSpecificTypeAndBaseTypeAnd()
"BeforeTry",
"InTry",
"InCatchSpecific",
"InCatchBase", // It would be better not visit this state
"End");
validator.TagStates("InCatchBase").Should().HaveCount(1).And.ContainSingle(x => HasExceptionOfType(x, "FileNotFoundException"));
validator.TagStates("InCatchBase").Should().BeEmpty();
validator.TagStates("InCatchSpecific").Should().HaveCount(1).And.ContainSingle(x => HasExceptionOfType(x, "FileNotFoundException"));
}

Expand Down Expand Up @@ -570,10 +567,49 @@ public void Throw_TryCatch_Throw_CatchBaseTypeAndSpecificType_WithWhenCondition(
"BeforeTry",
"InTry",
"InCatchSpecificNoCondition",
"InCatchBase", // It would be better not visit this state, but it gets tricky with conditions
"InCatchSpecificWithCondition",
"End");
validator.TagStates("InCatchBase").Should().HaveCount(1).And.ContainSingle(x => HasExceptionOfType(x, "FileNotFoundException"));
validator.TagStates("InCatchBase").Should().BeEmpty();
validator.TagStates("InCatchSpecificWithCondition").Should().HaveCount(1).And.ContainSingle(x => HasExceptionOfType(x, "FileNotFoundException"));
validator.TagStates("InCatchSpecificNoCondition").Should().HaveCount(1).And.ContainSingle(x => HasExceptionOfType(x, "FileNotFoundException"));
}

[TestMethod]
public void Throw_TryCatch_Throw_CatchBaseTypeAndSpecificType_WithWhenCondition_Nested()
{
const string code = """
var tag = "BeforeTry";
try
{
try
{
tag = "InTry";
throw new System.IO.FileNotFoundException();
tag = "UnreachableInTry";
}
catch (System.IO.FileNotFoundException) when (condition)
{
tag = "InCatchSpecificWithCondition";
}
}
catch (System.IO.FileNotFoundException)
{
tag = "InCatchSpecificNoCondition";
}
catch (System.IO.IOException)
{
tag = "InCatchBase";
}
tag = "End";
""";
var validator = SETestContext.CreateCS(code, "bool condition").Validator;
validator.ValidateTagOrder(
"BeforeTry",
"InTry",
"InCatchSpecificNoCondition",
"InCatchSpecificWithCondition",
"End");
validator.TagStates("InCatchBase").Should().BeEmpty();
validator.TagStates("InCatchSpecificWithCondition").Should().HaveCount(1).And.ContainSingle(x => HasExceptionOfType(x, "FileNotFoundException"));
validator.TagStates("InCatchSpecificNoCondition").Should().HaveCount(1).And.ContainSingle(x => HasExceptionOfType(x, "FileNotFoundException"));
}
Expand Down
67 changes: 20 additions & 47 deletions ...Analyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.TryCatchFinally.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -784,31 +784,35 @@ public void Catch_NestedInFinally()
[TestMethod]
public void CatchWhen_Simple()
{
const string code = @"
var tag = ""BeforeTry"";
try
{
Tag(""InTry"");
}
catch (Exception ex) when (ex is FormatException)
{
tag = ""InCatch"";
}
finally
{
tag = ""InFinally"";
}
tag = ""End"";";
const string code = """
var tag = "BeforeTry";
try
{
Tag("InTry");
}
catch (Exception ex) when (ex is FormatException)
{
tag = "InCatch";
}
finally
{
tag = "InFinally";
}
tag = "End";
""";
var validator = SETestContext.CreateCS(code).Validator;
validator.ValidateTagOrder(
"BeforeTry",
"InTry",
"InFinally",
"InFinally",
"InCatch",
"End");

validator.TagStates("InCatch").Should().HaveCount(1).And.ContainSingle(x => HasUnknownException(x));
validator.TagStates("InFinally").Should().HaveCount(1).And.ContainSingle(x => HasNoException(x));
validator.TagStates("InFinally").Should().HaveCount(2)
.And.ContainSingle(x => HasNoException(x), "no exception, and exception processed by catch clears exception state")
.And.ContainSingle(x => HasUnknownException(x), "if an exception not matching the filter is thrown, execution goes from 'InTry' to 'InFinally'.");
validator.TagStates("End").Should().HaveCount(1).And.ContainSingle(x => HasNoException(x));
}

Expand Down Expand Up @@ -861,37 +865,6 @@ public void CatchWhen_Multiple()
validator.TagStates("End").Should().HaveCount(1).And.ContainSingle(x => HasNoException(x));
}

[TestMethod]
public void CatchWhen_Finally()
{
const string code = @"
var tag = ""BeforeTry"";
try
{
Tag(""InTry"");
}
catch (Exception ex) when (ex is ArgumentNullException)
{
tag = ""InCatch"";
}
finally
{
tag = ""InFinally"";
}
tag = ""End"";";
var validator = SETestContext.CreateCS(code).Validator;
validator.ValidateTagOrder(
"BeforeTry",
"InTry",
"InFinally",
"InCatch",
"End");

validator.TagStates("InCatch").Should().HaveCount(1).And.ContainSingle(x => HasUnknownException(x));
validator.TagStates("InFinally").Should().HaveCount(1).And.ContainSingle(x => HasNoException(x));
validator.TagStates("End").Should().HaveCount(1).And.ContainSingle(x => HasNoException(x));
}

[TestMethod]
public void Finally_NestedLambda_ShouldNotFail()
{
Expand Down
29 changes: 29 additions & 0 deletions ...SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/ConditionEvaluatesToConstant.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3662,3 +3662,32 @@ public string WithFloat()
return "c";
}
}

// https://github.com/SonarSource/sonar-dotnet/issues/8484
public class Repro_8484
{
void TestMethod()
{
Exception failure = null;

try
{
try
{
var x = new object();
}
catch (InvalidOperationException)
{
}
}
catch (NotSupportedException exception)
{
failure = exception;
}

if (failure != null) // Compliant
{
Console.WriteLine("Found failures.");
}
}
}
33 changes: 31 additions & 2 deletions ...zer.UnitTest/TestCases/SymbolicExecution/Roslyn/LocksReleasedAllPaths.Monitor.TryCatch.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public void Method4(bool condition)

public void Method5(string arg)
{
Monitor.Enter(obj); // Compliant
Monitor.Enter(obj); // Noncompliant
try
{
Console.WriteLine(arg.Length);
Expand All @@ -65,12 +65,27 @@ public void Method5(string arg)
Monitor.Exit(obj);
}

public void CatchWhen(bool condition)
{
Monitor.Enter(obj); // Noncompliant
try
{
Console.WriteLine();
Monitor.Exit(obj);
}
catch when (condition)
{
Monitor.Exit(obj);
throw;
}
}

public void Method6(string arg)
{
Monitor.Enter(obj); // Noncompliant
try
{
Console.WriteLine(arg.Length);
Console.WriteLine(arg.Length); // throws NRE and UnknownException
}
catch (NullReferenceException nre) when (nre.Message.Contains("Dummy string"))
{
Expand All @@ -79,6 +94,20 @@ public void Method6(string arg)
}
}

public void CatchWhen_SpecificException(string arg)
{
Monitor.Enter(obj); // Noncompliant
try
{
throw new NotImplementedException();
}
catch (NotImplementedException nre) when (nre.Message.Contains("Dummy string"))
{
Monitor.Exit(obj);
throw;
}
}

public void Method7(string arg)
{
Monitor.Enter(obj); // Noncompliant
Expand Down
13 changes: 12 additions & 1 deletion ...zer.UnitTest/TestCases/SymbolicExecution/Roslyn/LocksReleasedAllPaths.Monitor.TryCatch.vb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ Namespace Monitor_TryCatch
End Sub

Public Sub Method5(Arg As String)
Monitor.Enter(Obj) ' Compliant
Monitor.Enter(Obj) ' Noncompliant
Try
Console.WriteLine(Arg.Length)
Catch nre As NullReferenceException
Expand All @@ -45,6 +45,17 @@ Namespace Monitor_TryCatch
Monitor.Exit(Obj)
End Sub

Public Sub CatchWhen(condition As Boolean)
Monitor.Enter(Obj) ' Noncompliant
Try
Console.WriteLine()
Monitor.Exit(Obj)
Catch When (condition)
Monitor.Exit(Obj)
Throw
End Try
End Sub

Public Sub Method6(Arg As String)
Monitor.Enter(Obj) ' Noncompliant
Try
Expand Down

0 comments on commit a314cfc

Please sign in to comment.