Merge pull request #4193 from tamasvajk/feature/sign-analysis

C#: Sign analysis

Author: tamasvajk

config/identical-files.json

@@ -50,6 +50,18 @@
     "csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowImplConsistency.qll",
     "python/ql/src/experimental/dataflow/internal/DataFlowImplConsistency.qll"
   ],
+  "SsaReadPosition Java/C#": [
+    "java/ql/src/semmle/code/java/dataflow/internal/rangeanalysis/SsaReadPositionCommon.qll",
+    "csharp/ql/src/semmle/code/csharp/dataflow/internal/rangeanalysis/SsaReadPositionCommon.qll"
+  ],
+  "Sign Java/C#": [
+    "java/ql/src/semmle/code/java/dataflow/internal/rangeanalysis/Sign.qll",
+    "csharp/ql/src/semmle/code/csharp/dataflow/internal/rangeanalysis/Sign.qll"
+  ],
+  "SignAnalysis Java/C#": [
+    "java/ql/src/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisCommon.qll",
+    "csharp/ql/src/semmle/code/csharp/dataflow/internal/rangeanalysis/SignAnalysisCommon.qll"
+  ],
   "C++ SubBasicBlocks": [
     "cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll",
     "cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll"
@@ -87,7 +99,7 @@
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Operand.qll",
     "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Operand.qll",
     "csharp/ql/src/experimental/ir/implementation/raw/Operand.qll",
-    "csharp/ql/src/experimental/ir/implementation/unaliased_ssa/Operand.qll" 
+    "csharp/ql/src/experimental/ir/implementation/unaliased_ssa/Operand.qll"
   ],
   "IR IRType": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/IRType.qll",
@@ -109,11 +121,11 @@
     "cpp/ql/src/semmle/code/cpp/ir/implementation/internal/OperandTag.qll",
     "csharp/ql/src/experimental/ir/implementation/internal/OperandTag.qll"
   ],
-  "IR TInstruction":[
+  "IR TInstruction": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/internal/TInstruction.qll",
     "csharp/ql/src/experimental/ir/implementation/internal/TInstruction.qll"
   ],
-  "IR TIRVariable":[
+  "IR TIRVariable": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/internal/TIRVariable.qll",
     "csharp/ql/src/experimental/ir/implementation/internal/TIRVariable.qll"
   ],
@@ -381,4 +393,4 @@
     "javascript/ql/src/Comments/CommentedOutCodeReferences.qhelp",
     "python/ql/src/Lexical/CommentedOutCodeReferences.qhelp"
   ]
-}
+}

csharp/ql/src/Linq/Helpers.qll

@@ -132,6 +132,17 @@ class AnyCall extends MethodCall {
   }
 }
 
+/** A LINQ Count(...) call. */
+class CountCall extends MethodCall {
+  CountCall() {
+    exists(Method m |
+      m = getTarget() and
+      isEnumerableType(m.getDeclaringType()) and
+      m.hasName("Count")
+    )
+  }
+}
+
 /** A variable of type IEnumerable<T>, for some T. */
 class IEnumerableSequence extends Variable {
   IEnumerableSequence() { isIEnumerableType(getType()) }

csharp/ql/src/semmle/code/csharp/commons/ComparisonTest.qll

@@ -2,7 +2,7 @@
  * Provides classes for capturing various ways of performing comparison tests.
  */
 
-import csharp
+private import csharp
 private import semmle.code.csharp.frameworks.System
 private import semmle.code.csharp.frameworks.system.Collections
 private import semmle.code.csharp.frameworks.system.collections.Generic

csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll

@@ -31,6 +31,25 @@ class Guard extends Expr {
   predicate controlsNode(ControlFlow::Nodes::ElementNode cfn, AccessOrCallExpr sub, AbstractValue v) {
     isGuardedByNode(cfn, this, sub, v)
   }
+
+  /**
+   * Holds if basic block `bb` is guarded by this expression having value `v`.
+   */
+  predicate controlsBasicBlock(BasicBlock bb, AbstractValue v) {
+    Internal::guardControls(this, bb, v)
+  }
+
+  /**
+   * Holds if this guard is an equality test between `e1` and `e2`. If the test is
+   * negated, that is `!=`, then `polarity` is false, otherwise `polarity` is
+   * true.
+   */
+  predicate isEquality(Expr e1, Expr e2, boolean polarity) {
+    exists(BooleanValue v |
+      this = Internal::getAnEqualityCheck(e1, v, e2) and
+      polarity = v.getValue()
+    )
+  }
 }
 
 /** An abstract value. */
@@ -943,31 +962,28 @@ module Internal {
     e = any(BinaryArithmeticOperation bao | result = bao.getAnOperand())
   }
 
-  /** Holds if basic block `bb` only is reached when guard `g` has abstract value `v`. */
-  private predicate guardControls(Guard g, BasicBlock bb, AbstractValue v) {
-    exists(ControlFlowElement cfe, ConditionalSuccessor s, AbstractValue v0, Guard g0 |
-      cfe.controlsBlock(bb, s)
-    |
-      v0.branch(cfe, s, g0) and
-      impliesSteps(g0, v0, g, v)
+  pragma[noinline]
+  private predicate assertionControlsNodeInSameBasicBlock0(
+    Guard g, AbstractValue v, BasicBlock bb, int i
+  ) {
+    exists(Assertion a, Guard g0, AbstractValue v0 |
+      asserts(a, g0, v0) and
+      impliesSteps(g0, v0, g, v) and
+      bb.getNode(i) = a.getAControlFlowNode()
     )
   }
 
   /**
    * Holds if control flow node `cfn` only is reached when guard `g` evaluates to `v`,
    * because of an assertion.
    */
-  private predicate guardAssertionControlsNode(Guard g, ControlFlow::Node cfn, AbstractValue v) {
-    exists(Assertion a, Guard g0, AbstractValue v0 |
-      asserts(a, g0, v0) and
-      impliesSteps(g0, v0, g, v)
-    |
-      a.strictlyDominates(cfn.getBasicBlock())
-      or
-      exists(BasicBlock bb, int i, int j | bb.getNode(i) = a.getAControlFlowNode() |
-        bb.getNode(j) = cfn and
-        j > i
-      )
+  private predicate assertionControlsNodeInSameBasicBlock(
+    Guard g, ControlFlow::Node cfn, AbstractValue v
+  ) {
+    exists(BasicBlock bb, int i, int j |
+      assertionControlsNodeInSameBasicBlock0(g, v, bb, i) and
+      bb.getNode(j) = cfn and
+      j > i
     )
   }
 
@@ -977,7 +993,7 @@ module Internal {
    */
   private predicate guardAssertionControlsElement(Guard g, ControlFlowElement cfe, AbstractValue v) {
     forex(ControlFlow::Node cfn | cfn = cfe.getAControlFlowNode() |
-      guardAssertionControlsNode(g, cfn, v)
+      assertionControlsNodeInSameBasicBlock(g, cfn, v)
     )
   }
 
@@ -1291,24 +1307,6 @@ module Internal {
       )
     }
 
-    /**
-     * Gets an expression that tests whether expression `e1` is equal to
-     * expression `e2`.
-     *
-     * If the returned expression has abstract value `v`, then expression `e1` is
-     * guaranteed to be equal to `e2`, and if the returned expression has abstract
-     * value `v.getDualValue()`, then this expression is guaranteed to be
-     * non-equal to `e`.
-     *
-     * For example, if the expression `x != ""` evaluates to `false` then the
-     * expression `x` is guaranteed to be equal to `""`.
-     */
-    Expr getAnEqualityCheck(Expr e1, AbstractValue v, Expr e2) {
-      result = getABooleanEqualityCheck(e1, v, e2)
-      or
-      result = getAMatchingEqualityCheck(e1, v, e2)
-    }
-
     private Expr getAnEqualityCheckVal(Expr e, AbstractValue v, AbstractValue vExpr) {
       result = getAnEqualityCheck(e, v, vExpr.getAnExpr())
     }
@@ -1464,6 +1462,29 @@ module Internal {
         not e = any(LocalVariableDeclStmt s).getAVariableDeclExpr()
       }
 
+      /**
+       * Gets an expression that tests whether expression `e1` is equal to
+       * expression `e2`.
+       *
+       * If the returned expression has abstract value `v`, then expression `e1` is
+       * guaranteed to be equal to `e2`, and if the returned expression has abstract
+       * value `v.getDualValue()`, then this expression is guaranteed to be
+       * non-equal to `e`.
+       *
+       * For example, if the expression `x != ""` evaluates to `false` then the
+       * expression `x` is guaranteed to be equal to `""`.
+       */
+      cached
+      Expr getAnEqualityCheck(Expr e1, AbstractValue v, Expr e2) {
+        result = getABooleanEqualityCheck(e1, v, e2)
+        or
+        result = getABooleanEqualityCheck(e2, v, e1)
+        or
+        result = getAMatchingEqualityCheck(e1, v, e2)
+        or
+        result = getAMatchingEqualityCheck(e2, v, e1)
+      }
+
       cached
       predicate isCustomNullCheck(Call call, Expr arg, BooleanValue v, boolean isNull) {
         exists(Callable callable, Parameter p |
@@ -1749,7 +1770,7 @@ module Internal {
       exists(Guard g | e = getAChildExprStar(g) |
         guardControls(g, bb, _)
         or
-        guardAssertionControlsNode(g, bb.getANode(), _)
+        assertionControlsNodeInSameBasicBlock(g, bb.getANode(), _)
       )
     }
   }
@@ -1758,6 +1779,21 @@ module Internal {
   private module Cached {
     private import semmle.code.csharp.Caching
 
+    /** Holds if basic block `bb` only is reached when guard `g` has abstract value `v`. */
+    cached
+    predicate guardControls(Guard g, BasicBlock bb, AbstractValue v) {
+      exists(AbstractValue v0, Guard g0 | impliesSteps(g0, v0, g, v) |
+        exists(ControlFlowElement cfe, ConditionalSuccessor s |
+          v0.branch(cfe, s, g0) and cfe.controlsBlock(bb, s)
+        )
+        or
+        exists(Assertion a |
+          asserts(a, g0, v0) and
+          a.strictlyDominates(bb)
+        )
+      )
+    }
+
     pragma[noinline]
     private predicate isGuardedByNode0(
       ControlFlow::Node cfn, AccessOrCallExpr guarded, Guard g, AccessOrCallExpr sub,
@@ -1813,7 +1849,7 @@ module Internal {
     ) {
       isGuardedByNode0(guarded, _, g, sub, v)
       or
-      guardAssertionControlsNode(g, guarded, v) and
+      assertionControlsNodeInSameBasicBlock(g, guarded, v) and
       exists(ConditionOnExprComparisonConfig c | c.same(sub, guarded.getElement()))
     }
 

csharp/ql/src/semmle/code/csharp/dataflow/Nullness.qll

@@ -21,10 +21,8 @@ import csharp
 private import ControlFlow
 private import internal.CallableReturns
 private import semmle.code.csharp.commons.Assertions
-private import semmle.code.csharp.commons.ComparisonTest
 private import semmle.code.csharp.controlflow.Guards as G
 private import semmle.code.csharp.controlflow.Guards::AbstractValues
-private import semmle.code.csharp.dataflow.SSA
 private import semmle.code.csharp.frameworks.System
 private import semmle.code.csharp.frameworks.Test
 

csharp/ql/src/semmle/code/csharp/dataflow/SSA.qll

@@ -2537,6 +2537,13 @@ module Ssa {
       )
     }
 
+    /** Holds if `inp` is an input to the phi node along the edge originating in `bb`. */
+    predicate hasInputFromBlock(Definition inp, BasicBlock bb) {
+      this.getAnInput() = inp and
+      this.getBasicBlock().getAPredecessor() = bb and
+      inp.isLiveAtEndOfBlock(bb)
+    }
+
     override string toString() {
       result = getToStringPrefix(this) + "SSA phi(" + getSourceVariable() + ")"
     }

csharp/ql/src/semmle/code/csharp/dataflow/SignAnalysis.qll

@@ -0,0 +1,9 @@
+/**
+ * Provides sign analysis to determine whether expression are always positive
+ * or negative.
+ *
+ * The analysis is implemented as an abstract interpretation over the
+ * three-valued domain `{negative, zero, positive}`.
+ */
+
+import semmle.code.csharp.dataflow.internal.rangeanalysis.SignAnalysisCommon

csharp/ql/src/semmle/code/csharp/dataflow/internal/rangeanalysis/ConstantUtils.qll

@@ -0,0 +1,64 @@
+/**
+ * Provides classes and predicates to represent constant integer expressions.
+ */
+
+private import csharp
+private import Ssa
+
+/**
+ * Holds if property `p` matches `property` in `baseClass` or any overrides.
+ */
+predicate propertyOverrides(Property p, string baseClass, string property) {
+  exists(Property p2 |
+    p2.getSourceDeclaration().getDeclaringType().hasQualifiedName(baseClass) and
+    p2.hasName(property)
+  |
+    p.overridesOrImplementsOrEquals(p2)
+  )
+}
+
+/**
+ * Holds if expression `e` is either
+ * - a compile time constant with integer value `val`, or
+ * - a read of a compile time constant with integer value `val`, or
+ * - a read of the `Length` of an array with `val` lengths.
+ */
+private predicate constantIntegerExpr(Expr e, int val) {
+  e.getValue().toInt() = val
+  or
+  exists(ExplicitDefinition v, Expr src |
+    e = v.getARead() and
+    src = v.getADefinition().getSource() and
+    constantIntegerExpr(src, val)
+  )
+  or
+  isArrayLengthAccess(e, val)
+}
+
+private int getArrayLength(ArrayCreation arrCreation, int index) {
+  constantIntegerExpr(arrCreation.getLengthArgument(index), result)
+}
+
+private int getArrayLengthRec(ArrayCreation arrCreation, int index) {
+  index = 0 and result = getArrayLength(arrCreation, 0)
+  or
+  index > 0 and
+  result = getArrayLength(arrCreation, index) * getArrayLengthRec(arrCreation, index - 1)
+}
+
+private predicate isArrayLengthAccess(PropertyAccess pa, int length) {
+  propertyOverrides(pa.getTarget(), "System.Array", "Length") and
+  exists(ExplicitDefinition arr, ArrayCreation arrCreation |
+    getArrayLengthRec(arrCreation, arrCreation.getNumberOfLengthArguments() - 1) = length and
+    arrCreation = arr.getADefinition().getSource() and
+    pa.getQualifier() = arr.getARead()
+  )
+}
+
+/** An expression that always has the same integer value. */
+class ConstantIntegerExpr extends Expr {
+  ConstantIntegerExpr() { constantIntegerExpr(this, _) }
+
+  /** Gets the integer value of this expression. */
+  int getIntValue() { constantIntegerExpr(this, result) }
+}