Fix S2583/2589 FP: Return inside lock, using and finally causes FP af…

…ter the block (#8761)
SonarSource · Feb 16, 2024 · afd7543 · afd7543
1 parent 4644b28
commit afd7543
Show file tree

Hide file tree

Showing 6 changed files with 626 additions and 496 deletions.
diff --git a/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/ExplodedNode.cs b/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/ExplodedNode.cs
@@ -58,18 +58,37 @@ public int AddVisit()
     }
 
     public override int GetHashCode() =>
-        HashCode.Combine(programPointHash, State);
+        HashCode.Combine(programPointHash, State, FinallyPoint?.BlockIndex);
 
     public override bool Equals(object obj) =>
         Equals(obj as ExplodedNode);
 
     public bool Equals(ExplodedNode other) =>
         other is not null
         && other.programPointHash == programPointHash
-        && other.State.Equals(State);
+        && other.State.Equals(State)
+        && HasSameFinallyPointChain(other.FinallyPoint);
 
     public override string ToString() =>
         Operation.Instance is { } operation
             ? $"Block #{Block.Ordinal}, Operation #{index}, {operation.Serialize()}{Environment.NewLine}{State}"
             : $"Block #{Block.Ordinal}, Branching{Environment.NewLine}{State}";
+
+    private bool HasSameFinallyPointChain(FinallyPoint other)
+    {
+        var current = FinallyPoint;
+        while (current is not null && other is not null)
+        {
+            if (current.BlockIndex == other.BlockIndex && current.BranchDestination == other.BranchDestination)
+            {
+                current = current.Previous;
+                other = other.Previous;
+            }
+            else
+            {
+                return false;
+            }
+        }
+        return current is null && other is null;
+    }
 }
diff --git a/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/FinallyPoint.cs b/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/FinallyPoint.cs
@@ -29,6 +29,7 @@ public sealed class FinallyPoint
 
     public bool IsFinallyBlock => finallyIndex < branch.FinallyRegions.Length;
     public int BlockIndex => IsFinallyBlock ? branch.FinallyRegions[finallyIndex].FirstBlockOrdinal : branch.Destination.Ordinal;
+    public int BranchDestination => branch.Destination.Ordinal;
     public FinallyPoint Previous { get; }
 
     public FinallyPoint(FinallyPoint previous, ControlFlowBranch branch, int finallyIndex = 0)

diff --git a/analyzers/tests/SonarAnalyzer.Test/SymbolicExecution/Roslyn/ExplodedNodeTest.cs b/analyzers/tests/SonarAnalyzer.Test/SymbolicExecution/Roslyn/ExplodedNodeTest.cs
@@ -18,9 +18,9 @@
  * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 
+using System.Xml.Linq;
 using SonarAnalyzer.Extensions;
 using SonarAnalyzer.SymbolicExecution.Roslyn;
-using SonarAnalyzer.Test.Helpers;
 
 namespace SonarAnalyzer.Test.SymbolicExecution.Roslyn;
 
@@ -113,19 +113,101 @@ public void Equals_ReturnsTrueForEquivalent()
         basic.Equals((ExplodedNode)null).Should().BeFalse();    // Explicit cast to ensure correct overload
     }
 
+    [TestMethod]
+    public void Equals_FinallyPoints_Single()
+    {
+        var cfg = TestHelper.CompileCfgBodyCS("""
+            try
+            {
+                if (condition)
+                {
+                    return; // Branch goes to exit 1->5, skipping AfterFinally
+                }
+                if (condition)
+                {
+                    return; // Branch goes to exit 2->5, skipping AfterFinally
+                }
+                // Branch goes AfterFinally 2->4
+            }
+            finally
+            {
+                System.Console.WriteLine("Finally");
+            }
+            System.Console.WriteLine("AfterFinally");
+            """, "bool condition");
+        var block = cfg.Blocks[0];
+        var from1to5 = cfg.Blocks[1].Successors.Single(x => x.Destination.Ordinal == 5);
+        var from2to5 = cfg.Blocks[2].Successors.Single(x => x.Destination.Ordinal == 5);
+        var from2to4 = cfg.Blocks[2].Successors.Single(x => x.Destination.Ordinal == 4);
+        var node = new ExplodedNode(block, ProgramState.Empty, new FinallyPoint(null, from1to5));
+        var noFinally = new ExplodedNode(block, ProgramState.Empty, null);
+
+        node.Equals(new ExplodedNode(block, ProgramState.Empty, new FinallyPoint(null, from2to5))).Should().BeTrue("We consider only same destination block");
+        node.Equals(new ExplodedNode(block, ProgramState.Empty, new FinallyPoint(null, from2to4))).Should().BeFalse("It has different destination block");
+        node.Equals(new ExplodedNode(block, ProgramState.Empty, new FinallyPoint(null, from1to5, 1))).Should().BeFalse("It has different BlockIndex");
+        node.Equals(noFinally).Should().BeFalse();
+        noFinally.Equals(node).Should().BeFalse();
+    }
+
+    [TestMethod]
+    public void Equals_FinallyPoints_WithPrevious()
+    {
+        var cfg = TestHelper.CompileCfgBodyCS("""
+            try
+            {
+                if (condition)
+                {
+                    return; // Branch goes to exit, skipping AfterFinally
+                }
+                if (condition)
+                {
+                    return; // Branch goes to exit, skipping AfterFinally
+                }
+                // Branch goes AfterFinally
+            }
+            finally
+            {
+                System.Console.WriteLine("Finally");
+            }
+            System.Console.WriteLine("AfterFinally");
+            """, "bool condition");
+        var block = cfg.Blocks[0];
+        var outerSame = cfg.Blocks[0].Successors.Single();
+        var from1to5 = cfg.Blocks[1].Successors.Single(x => x.Destination.Ordinal == 5);
+        var from2to5 = cfg.Blocks[2].Successors.Single(x => x.Destination.Ordinal == 5);
+        var from2to4 = cfg.Blocks[2].Successors.Single(x => x.Destination.Ordinal == 4);
+        var node = new ExplodedNode(block, ProgramState.Empty, new FinallyPoint(new FinallyPoint(null, from1to5), outerSame));
+        var noFinally = new ExplodedNode(block, ProgramState.Empty, Wrap(null));
+
+        node.Equals(new ExplodedNode(block, ProgramState.Empty, Wrap(new FinallyPoint(null, from2to5)))).Should().BeTrue("We consider only same destination block");
+        node.Equals(new ExplodedNode(block, ProgramState.Empty, Wrap(new FinallyPoint(null, from2to4)))).Should().BeFalse("It has different destination block");
+        node.Equals(new ExplodedNode(block, ProgramState.Empty, Wrap(new FinallyPoint(null, from1to5, 1)))).Should().BeFalse("It has different BlockIndex");
+        node.Equals(noFinally).Should().BeFalse();
+        noFinally.Equals(node).Should().BeFalse();
+
+        FinallyPoint Wrap(FinallyPoint inner) =>
+            new(inner, outerSame);
+    }
+
     [TestMethod]
     public void GetHashCode_ReturnsSameForEquivalent()
     {
-        var block = TestHelper.CompileCfgBodyCS("var a = true;").Blocks[1];
-        var basic = new ExplodedNode(block, ProgramState.Empty, null);
-        var same = new ExplodedNode(block, ProgramState.Empty, null);
+        var cfg = TestHelper.CompileCfgBodyCS("var a = true;");
+        var block = cfg.Blocks[1];
+        var finallyPoint = new FinallyPoint(null, block.Successors.Single());
+        var basic = new ExplodedNode(block, ProgramState.Empty, finallyPoint);
+        var same = new ExplodedNode(block, ProgramState.Empty, finallyPoint);
         var differentLocation = basic.CreateNext(ProgramState.Empty);
-        var differentState = new ExplodedNode(block, ProgramState.Empty.SetOperationValue(block.Operations[0], SymbolicValue.Empty), null);
+        var differentState = new ExplodedNode(block, ProgramState.Empty.SetOperationValue(block.Operations[0], SymbolicValue.Empty), finallyPoint);
+        var differentNoFinallyPoint = new ExplodedNode(block, ProgramState.Empty, null);
+        var differentFinallyPointBlockIndex = new ExplodedNode(block, ProgramState.Empty, new(null, cfg.Blocks[0].Successors.Single()));
 
         basic.GetHashCode().Should().Be(basic.GetHashCode(), "value should be deterministic");
         basic.GetHashCode().Should().Be(same.GetHashCode());
         basic.GetHashCode().Should().NotBe(differentLocation.GetHashCode());
         basic.GetHashCode().Should().NotBe(differentState.GetHashCode());
+        basic.GetHashCode().Should().NotBe(differentNoFinallyPoint.GetHashCode());
+        basic.GetHashCode().Should().NotBe(differentFinallyPointBlockIndex.GetHashCode());
     }
 
     [TestMethod]
@@ -134,21 +216,24 @@ public void ToString_SerializeOperationAndState()
         var cfg = TestHelper.CompileCfgBodyCS("var a = true;");
         var state = ProgramState.Empty.SetSymbolValue(cfg.Blocks[1].Operations[0].ChildOperations.First().TrackedSymbol(ProgramState.Empty), SymbolicValue.Empty);
 
-        new ExplodedNode(cfg.Blocks[0], ProgramState.Empty, null).ToString().Should().BeIgnoringLineEndings(
-@"Block #0, Branching
-Empty
-");
-
-        new ExplodedNode(cfg.Blocks[1], state, null).ToString().Should().BeIgnoringLineEndings(
-@"Block #1, Operation #0, LocalReferenceOperation: a = true
-Symbols:
-a: No constraints
-");
-        new ExplodedNode(cfg.ExitBlock, state, null).ToString().Should().BeIgnoringLineEndings(
-@"Block #2, Branching
-Symbols:
-a: No constraints
-");
+        new ExplodedNode(cfg.Blocks[0], ProgramState.Empty, null).ToString().Should().BeIgnoringLineEndings("""
+            Block #0, Branching
+            Empty
+
+            """);
+
+        new ExplodedNode(cfg.Blocks[1], state, null).ToString().Should().BeIgnoringLineEndings("""
+            Block #1, Operation #0, LocalReferenceOperation: a = true
+            Symbols:
+            a: No constraints
+
+            """);
+        new ExplodedNode(cfg.ExitBlock, state, null).ToString().Should().BeIgnoringLineEndings("""
+            Block #2, Branching
+            Symbols:
+            a: No constraints
+
+            """);
     }
 
     [TestMethod]

diff --git a/analyzers/tests/SonarAnalyzer.Test/SymbolicExecution/Roslyn/FinallyPointTest.cs b/analyzers/tests/SonarAnalyzer.Test/SymbolicExecution/Roslyn/FinallyPointTest.cs
@@ -36,22 +36,23 @@ public void Constructor_Null_Throws()
     [TestMethod]
     public void CreateNext_ReturnsAllFinally_AndThenDestination()
     {
-        const string code = @"
-try
-{
-    try
-    {
-        true.ToString();
-    }
-    finally
-    {
-        true.ToString();
-    }
-}
-finally
-{
-    true.ToString();
-}";
+        const string code = """
+            try
+            {
+                try
+                {
+                    true.ToString();
+                }
+                finally
+                {
+                    true.ToString();
+                }
+            }
+            finally
+            {
+                true.ToString();
+            }
+            """;
         var cfg = TestHelper.CompileCfgBodyCS(code);
         var sut = new FinallyPoint(null, cfg.Blocks[1].FallThroughSuccessor);
         sut.BlockIndex.Should().Be(2);   // Inner finally