Address PR comments

2026-04-28 02:05:14 +02:00 · 2019-09-26 16:43:27 +01:00
parent d6e4a2afef
commit a7a5eaa23f
7 changed files with 116 additions and 106 deletions
--- a/csharp/ql/src/semmle/code/csharp/ir/rangeanalysis/RangeAnalysis.qll
+++ b/csharp/ql/src/semmle/code/csharp/ir/rangeanalysis/RangeAnalysis.qll
@@ -233,19 +233,25 @@ private class SafeCastInstruction extends ConvertInstruction {
 * Holds if `typ` is a small integral type with the given lower and upper bounds.
 */
 private predicate typeBound(IntegralType typ, int lowerbound, int upperbound) {
-  typ instanceof SignedIntegralType and typ.getSize() = 1 and lowerbound = -128 and upperbound = 127
+  typ instanceof SignedIntegralType and
+  typ.getSize() = 1 and
+  lowerbound = typ.minValue() and
+  upperbound = typ.maxValue()
  or
-  typ instanceof UnsignedIntegralType and typ.getSize() = 1 and lowerbound = 0 and upperbound = 255
+  typ instanceof UnsignedIntegralType and
+  typ.getSize() = 1 and
+  lowerbound = typ.minValue() and
+  upperbound = typ.maxValue()
  or
  typ instanceof SignedIntegralType and
  typ.getSize() = 2 and
-  lowerbound = -32768 and
-  upperbound = 32767
+  lowerbound = typ.minValue() and
+  upperbound = typ.maxValue()
  or
  typ instanceof UnsignedIntegralType and
  typ.getSize() = 2 and
-  lowerbound = 0 and
-  upperbound = 65535
+  lowerbound = typ.minValue() and
+  upperbound = typ.maxValue()
 }

 /**
--- a/csharp/ql/src/semmle/code/csharp/ir/rangeanalysis/RangeUtils.qll
+++ b/csharp/ql/src/semmle/code/csharp/ir/rangeanalysis/RangeUtils.qll
@@ -40,10 +40,20 @@ IntValue getConstantValue(Instruction instr) {
  )
 }

+/**
+ * Gets the dimension of the array (either the declared size, or the
+ * size of the initializer); if no size is declared and no initializer used,
+ * the predicate does not hold.
+ */
 IntValue getArrayDim(Variable arr) {
  exists(ArrayCreation ac |
    arr.getInitializer() = ac and
-    result = ac.getInitializer().getNumberOfElements()
+    if exists(ac.getLengthArgument(0))
+    then result = ac.getLengthArgument(0).getValue().toInt()
+    else
+      if exists(ac.getInitializer())
+      then result = ac.getInitializer().getNumberOfElements()
+      else none()
  )
 }

--- a/csharp/ql/test/library-tests/ir/offbyone/OffByOneRA.expected
+++ b/csharp/ql/test/library-tests/ir/offbyone/OffByOneRA.expected
@@ -1,7 +1,5 @@
-| test.cs:10:17:10:21 | test1 | test.cs:17:41:17:51 | access to array element | This array access is ok, the index is at most the lenth of the array  + -1 |
-| test.cs:21:17:21:21 | test2 | test.cs:33:41:33:51 | access to array element | This array access might be out of bounds, as the index might be equal to the array length |
-| test.cs:37:17:37:21 | test3 | test.cs:50:41:50:51 | access to array element | This array access is ok, the index is at most the lenth of the array  + -1 |
-| test.cs:54:17:54:21 | test4 | test.cs:64:41:64:51 | access to array element | This array access might be out of bounds, as the index might be equal to the array length |
-| test.cs:68:17:68:21 | test5 | test.cs:74:22:74:27 | access to indexer | This array access might be out of bounds, as the index might be equal to the array length |
-| test.cs:78:17:78:21 | test6 | test.cs:87:41:87:55 | access to array element | This array access might be out of bounds, as the index might be equal to the array length |
-| test.cs:91:17:91:21 | test7 | test.cs:101:41:101:50 | access to array element | This array access might be out of bounds, as the index might be equal to the array length + 1 |
+| test.cs:21:17:21:21 | Test2 | test.cs:34:41:34:51 | access to array element | This array access might be out of bounds, as the index might be equal to the array length |
+| test.cs:56:17:56:21 | Test4 | test.cs:67:41:67:51 | access to array element | This array access might be out of bounds, as the index might be equal to the array length |
+| test.cs:71:17:71:21 | Test5 | test.cs:77:22:77:27 | access to indexer | This array access might be out of bounds, as the index might be equal to the array length |
+| test.cs:81:17:81:21 | Test6 | test.cs:90:41:90:55 | access to array element | This array access might be out of bounds, as the index might be equal to the array length |
+| test.cs:94:17:94:21 | Test7 | test.cs:104:41:104:50 | access to array element | This array access might be out of bounds, as the index might be equal to the array length + 1 |
--- a/csharp/ql/test/library-tests/ir/offbyone/OffByOneRA.ql
+++ b/csharp/ql/test/library-tests/ir/offbyone/OffByOneRA.ql
@@ -1,6 +1,7 @@
 import csharp
 import semmle.code.csharp.ir.IR
 import semmle.code.csharp.ir.rangeanalysis.RangeAnalysis
+import semmle.code.csharp.ir.rangeanalysis.RangeUtils

 /**
 * Holds if the index expression of `aa` is less than or equal to the array length plus `k`.
@@ -27,19 +28,8 @@ predicate boundedArrayAccess(ElementAccess aa, int k) {
            .getTarget()
    )
    or
-    exists(ArrayCreation ac |
-      ac.getParent().(LocalVariableDeclExpr).getVariable() = aa
-            .getQualifier()
-            .(VariableAccess)
-            .getTarget() and
-      b instanceof ZeroBound and
-      if exists(ac.getLengthArgument(0))
-      then k = delta - ac.getLengthArgument(0).getValue().toInt()
-      else
-        if exists(ac.getInitializer())
-        then k = delta - ac.getInitializer().getNumberOfElements()
-        else none()
-    )
+    b instanceof ZeroBound and
+    k = delta - getArrayDim(aa.getQualifier().(VariableAccess).getTarget())
  )
 }

@@ -54,12 +44,8 @@ predicate bestArrayAccessBound(ElementAccess aa, int k) {
 from ElementAccess aa, int k, string msg, string add
 where
  bestArrayAccessBound(aa, k) and
+  k >= 0 and
  (if k = 0 then add = "" else add = " + " + k) and
-  (
-    if k >= 0
-    then
-      msg = "This array access might be out of bounds, as the index might be equal to the array length"
-          + add
-    else msg = "This array access is ok, the index is at most the lenth of the array " + add
-  )
+  msg = "This array access might be out of bounds, as the index might be equal to the array length" +
+      add
 select aa.getEnclosingCallable(), aa, msg
--- a/csharp/ql/test/library-tests/ir/offbyone/test.cs
+++ b/csharp/ql/test/library-tests/ir/offbyone/test.cs
@@ -3,22 +3,23 @@ class ContainerLengthOffByOne
    public int[] arr;
    public string str;

-    public static void fun(int elem)
+    public static void Fun(int elem)
    {
    }

-    public void test1() 
+    public void Test1() 
    {
        int len1 = this.arr.Length;
        int len2 = len1 + 1;
        // OK
        for(int i = 0; i < len2 - 1; i++) 
        {
-            ContainerLengthOffByOne.fun(this.arr[i]);
+            ContainerLengthOffByOne.Fun(this.arr[i]);
        }
    }

-    public void test2() {
+    public void Test2() 
+    {
        int len1 = this.arr.Length;

        int len2;
@@ -30,11 +31,12 @@ class ContainerLengthOffByOne
        // exceeds the size of the array.
        for(int i = 0; i < len2; i++) 
        {
-            ContainerLengthOffByOne.fun(this.arr[i]);
+            ContainerLengthOffByOne.Fun(this.arr[i]);
        }
    }

-    public void test3() {
+    public void Test3() 
+    {
        int len1 = this.arr.Length;
        int len2 = len1 - 1;

@@ -47,11 +49,12 @@ class ContainerLengthOffByOne
        // we don't get an off by one error.
        for(int i = 0; i < len3; i++) 
        {
-            ContainerLengthOffByOne.fun(this.arr[i]);
+            ContainerLengthOffByOne.Fun(this.arr[i]);
        }
    }   

-    public void test4() {
+    public void Test4() 
+    {
        int len1 = this.arr.Length;

        int len2 = len1 + 1;
@@ -61,11 +64,11 @@ class ContainerLengthOffByOne
        // Not OK, len5 is off by one.
        for(int i = 0; i < len5; i++) 
        {
-            ContainerLengthOffByOne.fun(this.arr[i]);
+            ContainerLengthOffByOne.Fun(this.arr[i]);
        }
    }

-    public void test5()
+    public void Test5()
    {
        int len = this.str.Length;
        // Not OK; test for indexers
@@ -75,20 +78,20 @@ class ContainerLengthOffByOne
        }
    }

-    public void test6()
+    public void Test6()
    {
        int len = this.arr.Length - 2;
        int len1 = len + 3;
        int len2 = len1 - 1;
-        // Not OK, of by one
+        // Not OK, off by one
        // The test shows that more complex expressions are treated correctly
        for (int i = 0; i < len2; i++)
        {
-            ContainerLengthOffByOne.fun(this.arr[i + 1]);
+            ContainerLengthOffByOne.Fun(this.arr[i + 1]);
        }
    }

-    public void test7()
+    public void Test7()
    {
        int[] arrInit = { 1, 2, 3 };
        int len = (arrInit.Length * 2 + 2) / 2 * 2;
@@ -98,7 +101,7 @@ class ContainerLengthOffByOne
        // are used in bounds
        for (int i = 0; i < len1; i++) 
        {
-            ContainerLengthOffByOne.fun(arrInit[i]);    
+            ContainerLengthOffByOne.Fun(arrInit[i]);    
        }
    }
 }
--- a/csharp/ql/test/library-tests/ir/rangeanalysis/RangeAnalysis.expected
+++ b/csharp/ql/test/library-tests/ir/rangeanalysis/RangeAnalysis.expected
@@ -1,18 +1,18 @@
-| test.cs:19:12:19:12 | Store: access to parameter x | test.cs:15:24:15:24 | InitializeParameter: x | 0 | false | NoReason | file://:0:0:0:0 | :0:0:0:0 |
-| test.cs:19:12:19:12 | Store: access to parameter x | test.cs:15:31:15:31 | InitializeParameter: y | 0 | false | CompareLT: ... < ... | test.cs:16:9:16:13 | test.cs:16:9:16:13 |
-| test.cs:19:12:19:12 | Store: access to parameter x | test.cs:15:31:15:31 | InitializeParameter: y | 0 | false | NoReason | file://:0:0:0:0 | :0:0:0:0 |
-| test.cs:29:12:29:12 | Store: access to parameter x | test.cs:23:24:23:24 | InitializeParameter: x | -2 | false | NoReason | file://:0:0:0:0 | :0:0:0:0 |
-| test.cs:29:12:29:12 | Store: access to parameter x | test.cs:23:31:23:31 | InitializeParameter: y | -2 | false | CompareLT: ... < ... | test.cs:24:9:24:13 | test.cs:24:9:24:13 |
-| test.cs:36:12:36:12 | Load: access to local variable i | file://:0:0:0:0 | 0 | 0 | false | NoReason | file://:0:0:0:0 | :0:0:0:0 |
-| test.cs:36:12:36:12 | Load: access to local variable i | test.cs:33:25:33:25 | InitializeParameter: x | -1 | true | CompareLT: ... < ... | test.cs:35:20:35:24 | test.cs:35:20:35:24 |
-| test.cs:39:12:39:12 | Load: access to local variable i | file://:0:0:0:0 | 0 | 1 | false | CompareGT: ... > ... | test.cs:38:20:38:24 | test.cs:38:20:38:24 |
-| test.cs:39:12:39:12 | Load: access to local variable i | test.cs:33:25:33:25 | InitializeParameter: x | 0 | true | NoReason | file://:0:0:0:0 | :0:0:0:0 |
-| test.cs:39:12:39:12 | Load: access to local variable i | test.cs:35:20:35:20 | Phi: access to local variable i | 0 | true | CompareLT: ... < ... | test.cs:35:20:35:24 | test.cs:35:20:35:24 |
-| test.cs:42:12:42:12 | Load: access to local variable i | file://:0:0:0:0 | 0 | 0 | false | NoReason | file://:0:0:0:0 | :0:0:0:0 |
-| test.cs:42:12:42:12 | Load: access to local variable i | test.cs:33:25:33:25 | InitializeParameter: x | 1 | true | CompareLT: ... < ... | test.cs:41:20:41:28 | test.cs:41:20:41:28 |
-| test.cs:42:12:42:12 | Load: access to local variable i | test.cs:35:20:35:20 | Phi: access to local variable i | 1 | true | CompareLT: ... < ... | test.cs:41:20:41:28 | test.cs:41:20:41:28 |
-| test.cs:42:12:42:12 | Load: access to local variable i | test.cs:38:20:38:20 | Phi: access to local variable i | 0 | false | CompareGT: ... > ... | test.cs:38:20:38:24 | test.cs:38:20:38:24 |
-| test.cs:49:13:49:17 | Load: access to parameter begin | test.cs:47:33:47:37 | InitializeParameter: begin | 0 | false | NoReason | file://:0:0:0:0 | :0:0:0:0 |
-| test.cs:58:14:58:14 | Load: access to parameter x | test.cs:55:32:55:32 | InitializeParameter: y | -1 | true | CompareLT: ... < ... | test.cs:57:11:57:15 | test.cs:57:11:57:15 |
-| test.cs:58:14:58:14 | Load: access to parameter x | test.cs:55:39:55:39 | InitializeParameter: z | -2 | true | CompareLT: ... < ... | test.cs:57:11:57:15 | test.cs:57:11:57:15 |
-| test.cs:63:14:63:14 | Load: access to parameter x | test.cs:55:32:55:32 | InitializeParameter: y | -1 | true | CompareLT: ... < ... | test.cs:61:9:61:13 | test.cs:61:9:61:13 |
+| test.cs:22:12:22:12 | Store: access to parameter x | test.cs:16:24:16:24 | InitializeParameter: x | 0 | false | NoReason | file://:0:0:0:0 | :0:0:0:0 |
+| test.cs:22:12:22:12 | Store: access to parameter x | test.cs:16:31:16:31 | InitializeParameter: y | 0 | false | CompareLT: ... < ... | test.cs:18:9:18:13 | test.cs:18:9:18:13 |
+| test.cs:22:12:22:12 | Store: access to parameter x | test.cs:16:31:16:31 | InitializeParameter: y | 0 | false | NoReason | file://:0:0:0:0 | :0:0:0:0 |
+| test.cs:36:12:36:12 | Store: access to parameter x | test.cs:26:24:26:24 | InitializeParameter: x | -2 | false | NoReason | file://:0:0:0:0 | :0:0:0:0 |
+| test.cs:36:12:36:12 | Store: access to parameter x | test.cs:26:31:26:31 | InitializeParameter: y | -2 | false | CompareLT: ... < ... | test.cs:28:9:28:13 | test.cs:28:9:28:13 |
+| test.cs:45:12:45:12 | Load: access to local variable i | file://:0:0:0:0 | 0 | 0 | false | NoReason | file://:0:0:0:0 | :0:0:0:0 |
+| test.cs:45:12:45:12 | Load: access to local variable i | test.cs:40:25:40:25 | InitializeParameter: x | -1 | true | CompareLT: ... < ... | test.cs:43:20:43:24 | test.cs:43:20:43:24 |
+| test.cs:49:12:49:12 | Load: access to local variable i | file://:0:0:0:0 | 0 | 1 | false | CompareGT: ... > ... | test.cs:47:20:47:24 | test.cs:47:20:47:24 |
+| test.cs:49:12:49:12 | Load: access to local variable i | test.cs:40:25:40:25 | InitializeParameter: x | 0 | true | NoReason | file://:0:0:0:0 | :0:0:0:0 |
+| test.cs:49:12:49:12 | Load: access to local variable i | test.cs:43:20:43:20 | Phi: access to local variable i | 0 | true | CompareLT: ... < ... | test.cs:43:20:43:24 | test.cs:43:20:43:24 |
+| test.cs:53:12:53:12 | Load: access to local variable i | file://:0:0:0:0 | 0 | 0 | false | NoReason | file://:0:0:0:0 | :0:0:0:0 |
+| test.cs:53:12:53:12 | Load: access to local variable i | test.cs:40:25:40:25 | InitializeParameter: x | 1 | true | CompareLT: ... < ... | test.cs:51:20:51:28 | test.cs:51:20:51:28 |
+| test.cs:53:12:53:12 | Load: access to local variable i | test.cs:43:20:43:20 | Phi: access to local variable i | 1 | true | CompareLT: ... < ... | test.cs:51:20:51:28 | test.cs:51:20:51:28 |
+| test.cs:53:12:53:12 | Load: access to local variable i | test.cs:47:20:47:20 | Phi: access to local variable i | 0 | false | CompareGT: ... > ... | test.cs:47:20:47:24 | test.cs:47:20:47:24 |
+| test.cs:62:13:62:17 | Load: access to parameter begin | test.cs:58:33:58:37 | InitializeParameter: begin | 0 | false | NoReason | file://:0:0:0:0 | :0:0:0:0 |
+| test.cs:74:14:74:14 | Load: access to parameter x | test.cs:68:32:68:32 | InitializeParameter: y | -1 | true | CompareLT: ... < ... | test.cs:72:11:72:15 | test.cs:72:11:72:15 |
+| test.cs:74:14:74:14 | Load: access to parameter x | test.cs:68:39:68:39 | InitializeParameter: z | -2 | true | CompareLT: ... < ... | test.cs:72:11:72:15 | test.cs:72:11:72:15 |
+| test.cs:81:14:81:14 | Load: access to parameter x | test.cs:68:32:68:32 | InitializeParameter: y | -1 | true | CompareLT: ... < ... | test.cs:77:9:77:13 | test.cs:77:9:77:13 |
--- a/csharp/ql/test/library-tests/ir/rangeanalysis/test.cs
+++ b/csharp/ql/test/library-tests/ir/rangeanalysis/test.cs
@@ -1,78 +1,85 @@
 class RangeAnalysis {
-  static void sink(int val) {
-  
+  static void Sink(int val) 
+  {
  }
  
-  static unsafe void sinkp(int* p) {
-  
+  static unsafe void Sinkp(int* p) 
+  {
  }
  
-  static int source() {
+  static int Source() 
+  {
    return 0;
  }
  
  // Guards, inference, critical edges
-  static int test1(int x, int y) {
-    if (x < y) {
+  static int Test1(int x, int y) 
+  {
+    if (x < y) 
+    {
      x = y;
    }
    return x;
  }
  
  // Bounds mergers at phi nodes
-  static int test2(int x, int y) {
-    if (x < y) {
+  static int Test2(int x, int y) 
+  {
+    if (x < y) 
+    {
      x = y;
-    } else {
-      x = x-2;
+    } 
+    else 
+    {
+      x = x - 2;
    }
    return x;
  }
  
  // for loops
-  static void test3(int x) {
+  static void Test3(int x) 
+  {
    int y = x;
-    for(int i = 0; i < x; i++) {
-      sink(i);
+    for(int i = 0; i < x; i++) 
+    {
+      Sink(i);
    }
-    for(int i = y; i > 0; i--) {
-      sink(i);
+    for(int i = y; i > 0; i--) 
+    {
+      Sink(i);
    }
-    for(int i = 0; i < y + 2; i++) {
-      sink(i);
+    for(int i = 0; i < y + 2; i++) 
+    {
+      Sink(i);
    }
  }
  
  // pointer bounds
-  unsafe static void test4(int *begin, int *end) {
-    while (begin < end) {
-      sinkp(begin);
+  unsafe static void Test4(int *begin, int *end) 
+  {
+    while (begin < end) 
+    {
+      Sinkp(begin);
      begin++;
    }
  }
  
  // bound propagation through conditionals
-  static void test5(int x, int y, int z) {
-    if (y < z) {
-      if (x < y) {
-        sink(x);
+  static void Test5(int x, int y, int z) 
+  {
+    if (y < z) 
+    {
+      if (x < y) 
+      {
+        Sink(x);
      }
    }
-    if (x < y) {
-      if (y < z) {
-        sink(x); // x < z is not inferred here
+    if (x < y) 
+    {
+      if (y < z) 
+      {
+        Sink(x); // x < z is not inferred here
      }
    }
  }
-
-    unsafe static void addone(int[] arr) 
-    {
-        int length = arr.Length;
-        fixed (int* b = arr) 
-        {
-            int *p = b;
-            for(int i = 0; i <= length; i++)
-                *p++ += 1;
-        }
-    }  
 }