C#: Fix false-positives in cs/index-out-of-bounds.

Author: calumgrant

csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql

@@ -14,16 +14,55 @@
 
 import csharp
 import semmle.code.csharp.controlflow.Guards
+import semmle.code.csharp.commons.ComparisonTest
 
-from RelationalOperation cmp, Variable array, Variable index, ElementAccess ea, VariableAccess indexAccess
-// Look for `index <= array.Length` or `array.Length >= index`
-where (cmp instanceof GEExpr or cmp instanceof LEExpr)
-  and cmp.getGreaterOperand() = any(PropertyAccess pa | pa.getQualifier() = array.getAnAccess() and pa.getTarget().hasName("Length"))
-  and cmp.getLesserOperand() = index.getAnAccess()
+/** A comparison of an index variable with the length of an array. */
+class IndexGuard extends Expr {
+  ComparisonTest ct;
+  VariableAccess indexAccess;
+  Variable array;
+  
+  IndexGuard() {
+    ct.getExpr() = this and
+    ct.getFirstArgument() = indexAccess and
+    ct.getSecondArgument() = any(PropertyAccess lengthAccess | 
+      lengthAccess.getQualifier() = array.getAnAccess() and
+      lengthAccess.getTarget().hasName("Length")
+    )
+  }
+
+  /** This comparison applies to array `arr` and index `ind`. */
+  predicate controls(Variable arr, Variable ind) {
+    arr = array and
+    ind.getAnAccess() = indexAccess
+  }
+
+  /** This comparison guards `expr`. */
+  predicate guards(GuardedExpr expr, boolean condition) {
+    expr.isGuardedBy(this, indexAccess, condition)
+  }
+
+  /** This comparison is an incorrect `<=` or equivalent. */
+  predicate isIncorrect() {
+    ct.getComparisonKind().isLessThanEquals()
+  }
+}
+
+from IndexGuard incorrectGuard, Variable array, Variable index, ElementAccess ea, GuardedExpr indexAccess
+where
+  // Look for `index <= array.Length` or `array.Length >= index`
+  incorrectGuard.controls(array, index) and
+  incorrectGuard.isIncorrect() and 
   // Look for `array[index]`
-  and ea.getQualifier() = array.getAnAccess()
-  and ea.getIndex(0) = indexAccess
-  and indexAccess = index.getAnAccess()
+  ea.getQualifier() = array.getAnAccess() and
+  ea.getIndex(0) = indexAccess and
+  indexAccess = index.getAnAccess() and
   // Where the index access is guarded by the comparison
-  and indexAccess.(GuardedExpr).isGuardedBy(cmp, index.getAnAccess(), true)
-select cmp, "Off-by-one index comparison against length leads to possible out of bounds $@.", ea, ea.toString()
+  incorrectGuard.guards(indexAccess, true) and
+  // And there are no other guards
+  not exists(IndexGuard validGuard |
+    not validGuard.isIncorrect() and
+    validGuard.controls(array, index) and
+    validGuard.guards(indexAccess, _)
+  )
+select incorrectGuard, "Off-by-one index comparison against length leads to possible out of bounds $@.", ea, ea.toString()

csharp/ql/test/query-tests/Likely Bugs/Collections/ContainerLengthCmpOffByOne/ContainerLengthCmpOffByOne.cs

@@ -2,7 +2,7 @@
 
 class Test
 {
-    static void Main(string[] args)
+    void Test1(string[] args)
     {
         // BAD: Loop upper bound is off-by-one
         for (int i = 0; i <= args.Length; i++)
@@ -34,11 +34,66 @@ static void Main(string[] args)
         {
             Console.WriteLine(args[j]);
         }
+    }
+
+    void Test2(string[] args)
+    {
+        int j = 0;
 
         // GOOD: Correct terminating value
         if (args.Length > j)
         {
             Console.WriteLine(args[j]);
         }
     }
+
+    void Test3(string[] args)
+    {    
+        // GOOD: Guarded by ternary operator.
+        for (int i = 0; i <= args.Length; i++)
+        {
+            string s = i < args.Length ? args[i] : "";
+        }
+    }
+
+    void Test4(string[] args)
+    {
+        int j = 0;
+
+        // GOOD: Guarded by ternary operator.
+        if( j <= args.Length )
+        {
+            string s = j < args.Length ? args[j] : "";
+        }
+    }
+
+    void Test5(string[] args)
+    {  
+        // GOOD: A valid test of Length.
+        for (int i = 0; i != args.Length; i++)
+        {
+            Console.WriteLine(args[i]);
+        }
+    }
+
+    void Test6(string[] args)
+    {
+        int j = 0;
+
+        // GOOD: There is another correct test.
+        if( j <= args.Length )
+        {
+            if (j == args.Length) return;
+            Console.WriteLine(args[j]);
+        }
+    }
+
+    void Test7(string[] args)
+    {
+        // GOOD: Guarded by ||.
+        for (int i = 0; i <= args.Length; i++)
+        {
+            bool b = i == args.Length || args[i] == "x";
+        }        
+    }
 }