SONARJAVA-3747 FPs in S5852 when repetition overlaps with non-repetit…

…ion part (#3494)
SonarSource · Mar 19, 2021 · 55885f1 · 55885f1
1 parent 3fa749e
commit 55885f1
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 16 deletions.
diff --git a/its/ruling/src/test/resources/regex-examples/java-S5852.json b/its/ruling/src/test/resources/regex-examples/java-S5852.json
@@ -115,8 +115,6 @@
 473,
 475,
 479,
-511,
-529,
 551,
 557,
 717,
@@ -142,7 +140,6 @@
 1559,
 1673,
 1741,
-1801,
 1871,
 1897,
 1899,
@@ -612,7 +609,6 @@
 1509,
 1523,
 1771,
-1795,
 1805,
 1807,
 1817,
@@ -632,7 +628,6 @@
 225,
 227,
 231,
-261,
 277,
 331,
 347,
@@ -643,7 +638,6 @@
 633,
 637,
 645,
-649,
 657,
 659,
 665,
@@ -654,7 +648,6 @@
 697,
 699,
 715,
-729,
 791,
 861,
 869,
@@ -664,9 +657,6 @@
 929,
 933,
 935,
-945,
-947,
-949,
 951,
 953,
 965,

diff --git a/java-checks-test-sources/src/main/java/checks/regex/RedosCheck.java b/java-checks-test-sources/src/main/java/checks/regex/RedosCheck.java
@@ -56,7 +56,9 @@ void alwaysQuadratic(String str) {
     str.matches("x*a*x*"); // Noncompliant {{Make sure the regex used here, which is vulnerable to polynomial runtime due to backtracking, cannot lead to denial of service.}}
     str.matches("x*,a*x*"); // Compliant, can fail between the two quantifiers
     str.matches("x*(xy?)*"); // Noncompliant {{Make sure the regex used here, which is vulnerable to polynomial runtime due to backtracking, cannot lead to denial of service.}}
-    str.matches("(ab)*a(ba)*"); // Noncompliant {{Make sure the regex used here, which is vulnerable to polynomial runtime due to backtracking, cannot lead to denial of service.}}
+    str.matches("(ab)*a(ba)*"); // False Negative :-(
+    str.matches("x*xx*"); // Noncompliant {{Make sure the regex used here, which is vulnerable to polynomial runtime due to backtracking, cannot lead to denial of service.}}
+    str.matches("x*yx*"); // Compliant
     str.matches("x*a*b*c*d*e*f*g*h*i*x*"); // Noncompliant {{Make sure the regex used here, which is vulnerable to polynomial runtime due to backtracking, cannot lead to denial of service.}}
     str.matches("x*a*b*c*d*e*f*g*h*i*j*x*"); // FN because we forget about the first x* when the maximum number of tracked repetitions is exceeded
     str.matches("x*a*b*c*d*e*f*g*h*i*j*x*x*"); // Noncompliant {{Make sure the regex used here, which is vulnerable to polynomial runtime due to backtracking, cannot lead to denial of service.}}
@@ -125,8 +127,8 @@ void compliant(String str) {
     str.matches("(;*,)*");
     str.matches("x*|x*");
     str.matches("a*b*");
-    str.matches("a*a?b*"); // Noncompliant - false positive :-(
-    str.matches("a*(a?b)*"); // Noncompliant - false positive :-(
+    str.matches("a*a?b*");
+    str.matches("a*(a?b)*");
     str.matches("a*(ab)*");
     str.split("x*x*");
     str.matches("(?s)x*.*");

diff --git a/java-checks/src/main/java/org/sonar/java/checks/regex/RedosCheck.java b/java-checks/src/main/java/org/sonar/java/checks/regex/RedosCheck.java
@@ -179,9 +179,12 @@ private void checkForOverlappingRepetitions(RepetitionTree tree) {
       if (tree.getQuantifier().isOpenEnded() && canFail(tree)) {
         for (RepetitionTree repetition : nonPossessiveRepetitions) {
           if (reachabilityChecker.canReach(repetition, tree)) {
-            SubAutomaton auto1 = new SubAutomaton(repetition.getElement(), tree.continuation(), false);
-            SubAutomaton auto2 = new SubAutomaton(repetition.continuation(), tree.continuation(), false);
-            if (intersectionChecker.check(auto1, auto2)) {
+            SubAutomaton repetitionAuto = new SubAutomaton(repetition.getElement(), repetition.continuation(), false);
+            SubAutomaton continuationAuto = new SubAutomaton(repetition.continuation(), tree, false);
+            SubAutomaton treeAuto = new SubAutomaton(tree.getElement(), tree.continuation(), false);
+            if (subAutomatonCanConsume(repetitionAuto, continuationAuto)
+              && automatonIsEmptyOrIntersects(continuationAuto, treeAuto)
+              && intersectionChecker.check(repetitionAuto, treeAuto)) {
               addBacktracking(BacktrackingType.ALWAYS_QUADRATIC);
             }
           }
@@ -193,6 +196,16 @@ private void checkForOverlappingRepetitions(RepetitionTree tree) {
       }
     }
 
+    private boolean subAutomatonCanConsume(SubAutomaton auto1, SubAutomaton auto2) {
+      return canReachWithoutConsumingInputOrGoingThroughBoundaries(auto1.end, auto2.end)
+        || intersectionChecker.check(auto1, auto2);
+    }
+
+    private boolean automatonIsEmptyOrIntersects(SubAutomaton auto1, SubAutomaton auto2) {
+      return canReachWithoutConsumingInputOrGoingThroughBoundaries(auto1.start, auto1.end)
+        || intersectionChecker.check(auto1, auto2);
+    }
+
     private void addIfNonPossessive(RepetitionTree tree) {
       if (!tree.isPossessive()) {
         nonPossessiveRepetitions.add(tree);