Merge pull request #568 from calumgrant/cs/index-out-of-bounds

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

Author: hvitved

change-notes/1.20/analysis-csharp.md

@@ -0,0 +1,19 @@
+# Improvements to C# analysis
+
+## General improvements
+
+## New queries
+
+| **Query**                   | **Tags**  | **Purpose**                                                        |
+|-----------------------------|-----------|--------------------------------------------------------------------|
+
+## Changes to existing queries
+
+| *@name of query (Query ID)*| *Impact on results*    | *How/why the query has changed*                                  |
+| Off-by-one comparison against container length (cs/index-out-of-bounds) | Fewer false positives | Results have been removed when there are additional guards on the index. |
+
+## Changes to code extraction
+
+## Changes to QL libraries
+
+## Changes to the autobuilder

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

@@ -14,16 +14,53 @@
 
 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 ComparisonTest {
+  VariableAccess indexAccess;
+  Variable array;
+  
+  IndexGuard() {
+    this.getFirstArgument() = indexAccess and
+    this.getSecondArgument() = any(PropertyAccess lengthAccess | 
+      lengthAccess.getQualifier() = array.getAnAccess() and
+      lengthAccess.getTarget().hasName("Length")
+    )
+  }
+
+  /** Holds if this comparison applies to array `arr` and index `ind`. */
+  predicate controls(Variable arr, Variable ind) {
+    arr = array and
+    ind.getAnAccess() = indexAccess
+  }
+
+  /** Holds if this comparison guards `expr`. */
+  predicate guards(GuardedExpr expr, boolean condition) {
+    expr.isGuardedBy(this.getExpr(), indexAccess, condition)
+  }
+
+  /** Holds if this comparison is an incorrect `<=` or equivalent. */
+  predicate isIncorrect() {
+    this.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";
+        }        
+    }
 }