Merge pull request #21348 from hvitved/csharp/remove-tcs

C#: Remove some unbounded TC computations

Author: hvitved

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

@@ -142,6 +142,7 @@ private module GuardsInput implements
     }
   }
 
+  pragma[nomagic]
   predicate equalityTest(Expr eqtest, Expr left, Expr right, boolean polarity) {
     exists(ComparisonTest ct |
       ct.getExpr() = eqtest and
@@ -410,6 +411,22 @@ private predicate typePattern(PatternMatch pm, TypePatternExpr tpe, Type t) {
   t = pm.getExpr().getType()
 }
 
+pragma[nomagic]
+private predicate dereferenceableExpr(Expr e, boolean isNullableType) {
+  exists(Type t | t = e.getType() |
+    t instanceof NullableType and
+    isNullableType = true
+    or
+    t instanceof RefType and
+    isNullableType = false
+  )
+  or
+  exists(Expr parent |
+    dereferenceableExpr(parent, isNullableType) and
+    e = getNullEquivParent(parent)
+  )
+}
+
 /**
  * An expression that evaluates to a value that can be dereferenced. That is,
  * an expression that may evaluate to `null`.
@@ -418,21 +435,12 @@ class DereferenceableExpr extends Expr {
   private boolean isNullableType;
 
   DereferenceableExpr() {
-    exists(Expr e, Type t |
-      // There is currently a bug in the extractor: the type of `x?.Length` is
-      // incorrectly `int`, while it should have been `int?`. We apply
-      // `getNullEquivParent()` as a workaround
-      this = getNullEquivParent*(e) and
-      t = e.getType() and
-      not this instanceof SwitchCaseExpr and
-      not this instanceof PatternExpr
-    |
-      t instanceof NullableType and
-      isNullableType = true
-      or
-      t instanceof RefType and
-      isNullableType = false
-    )
+    // There is currently a bug in the extractor: the type of `x?.Length` is
+    // incorrectly `int`, while it should have been `int?`. We apply
+    // `getNullEquivParent()` as a workaround
+    dereferenceableExpr(this, isNullableType) and
+    not this instanceof SwitchCaseExpr and
+    not this instanceof PatternExpr
   }
 
   /** Holds if this expression has a nullable type `T?`. */

csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll

@@ -94,9 +94,19 @@ private Element getAChild(Element p) {
   result = p.(AssignOperation).getExpandedAssignment()
 }
 
+pragma[nomagic]
+private predicate astNode(Element e) {
+  e = any(@top_level_exprorstmt_parent p | not p instanceof Attribute)
+  or
+  exists(Element parent |
+    astNode(parent) and
+    e = getAChild(parent)
+  )
+}
+
 /** An AST node. */
 class AstNode extends Element, TAstNode {
-  AstNode() { this = getAChild*(any(@top_level_exprorstmt_parent p | not p instanceof Attribute)) }
+  AstNode() { astNode(this) }
 
   int getId() { idOf(this, result) }
 }

csharp/ql/lib/semmle/code/csharp/dataflow/internal/ControlFlowReachability.qll

@@ -15,16 +15,47 @@ private class ControlFlowScope extends ControlFlowElement {
   predicate isNonExact() { exactScope = false }
 }
 
-private ControlFlowElement getANonExactScopeChild(ControlFlowScope scope) {
-  scope.isNonExact() and
-  result = scope
+private newtype TControlFlowElementOrBasicBlock =
+  TControlFlowElement(ControlFlowElement cfe) or
+  TBasicBlock(ControlFlow::BasicBlock bb)
+
+class ControlFlowElementOrBasicBlock extends TControlFlowElementOrBasicBlock {
+  ControlFlowElement asControlFlowElement() { this = TControlFlowElement(result) }
+
+  ControlFlow::BasicBlock asBasicBlock() { this = TBasicBlock(result) }
+
+  string toString() {
+    result = this.asControlFlowElement().toString()
+    or
+    result = this.asBasicBlock().toString()
+  }
+
+  Location getLocation() {
+    result = this.asControlFlowElement().getLocation()
+    or
+    result = this.asBasicBlock().getLocation()
+  }
+}
+
+private predicate isBasicBlock(ControlFlowElementOrBasicBlock c) { c instanceof TBasicBlock }
+
+private predicate isNonExactScope(ControlFlowElementOrBasicBlock c) {
+  c.asControlFlowElement().(ControlFlowScope).isNonExact()
+}
+
+private predicate step(ControlFlowElementOrBasicBlock pred, ControlFlowElementOrBasicBlock succ) {
+  pred.asBasicBlock().getANode().getAstNode() = succ.asControlFlowElement()
   or
-  result = getANonExactScopeChild(scope).getAChild()
+  pred.asControlFlowElement() = succ.asControlFlowElement().getAChild()
 }
 
+private predicate basicBlockInNonExactScope(
+  ControlFlowElementOrBasicBlock bb, ControlFlowElementOrBasicBlock scope
+) = doublyBoundedFastTC(step/2, isBasicBlock/1, isNonExactScope/1)(bb, scope)
+
 pragma[noinline]
 private ControlFlow::BasicBlock getABasicBlockInScope(ControlFlowScope scope, boolean exactScope) {
-  result.getANode().getAstNode() = getANonExactScopeChild(scope) and
+  basicBlockInNonExactScope(TBasicBlock(result), TControlFlowElement(scope)) and
   exactScope = false
   or
   scope.isExact() and

csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll

@@ -8,6 +8,7 @@ private import semmle.code.csharp.dataflow.FlowSummary as FlowSummary
 private import semmle.code.csharp.dataflow.internal.ExternalFlow
 private import semmle.code.csharp.commons.Collections
 private import semmle.code.csharp.Conversion
+private import semmle.code.csharp.exprs.internal.Expr
 private import semmle.code.csharp.dataflow.internal.SsaImpl as SsaImpl
 private import semmle.code.csharp.ExprOrStmtParent
 private import semmle.code.csharp.Unification
@@ -2377,6 +2378,16 @@ predicate storeStep(Node node1, ContentSet c, Node node2) {
   storeStepDelegateCall(node1, c, node2)
 }
 
+pragma[nomagic]
+private predicate isAssignExprLValueDescendant(Expr e) {
+  e = any(AssignExpr ae).getLValue()
+  or
+  exists(Expr parent |
+    isAssignExprLValueDescendant(parent) and
+    e = parent.getAChildExpr()
+  )
+}
+
 private class ReadStepConfiguration extends ControlFlowReachabilityConfiguration {
   ReadStepConfiguration() { this = "ReadStepConfiguration" }
 
@@ -2432,7 +2443,7 @@ private class ReadStepConfiguration extends ControlFlowReachabilityConfiguration
     scope =
       any(AssignExpr ae |
         ae = defTo.(AssignableDefinitions::TupleAssignmentDefinition).getAssignment() and
-        e = ae.getLValue().getAChildExpr*().(TupleExpr) and
+        isAssignExprLValueDescendant(e.(TupleExpr)) and
         exactScope = false and
         isSuccessor = true
       )
@@ -2488,7 +2499,7 @@ private predicate readContentStep(Node node1, Content c, Node node2) {
       )
       or
       // item = variable in node1 = (..., variable, ...) in a case/is var (..., ...)
-      te = any(PatternExpr pe).getAChildExpr*() and
+      isPatternExprDescendant(te) and
       exists(AssignableDefinitions::LocalVariableDefinition lvd |
         node2.(AssignableDefinitionNode).getDefinition() = lvd and
         lvd.getDeclaration() = item and

csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/ControlFlowReachability.qll

@@ -15,16 +15,47 @@ private class ControlFlowScope extends ControlFlowElement {
   predicate isNonExact() { exactScope = false }
 }
 
-private ControlFlowElement getANonExactScopeChild(ControlFlowScope scope) {
-  scope.isNonExact() and
-  result = scope
+private newtype TControlFlowElementOrBasicBlock =
+  TControlFlowElement(ControlFlowElement cfe) or
+  TBasicBlock(ControlFlow::BasicBlock bb)
+
+class ControlFlowElementOrBasicBlock extends TControlFlowElementOrBasicBlock {
+  ControlFlowElement asControlFlowElement() { this = TControlFlowElement(result) }
+
+  ControlFlow::BasicBlock asBasicBlock() { this = TBasicBlock(result) }
+
+  string toString() {
+    result = this.asControlFlowElement().toString()
+    or
+    result = this.asBasicBlock().toString()
+  }
+
+  Location getLocation() {
+    result = this.asControlFlowElement().getLocation()
+    or
+    result = this.asBasicBlock().getLocation()
+  }
+}
+
+private predicate isBasicBlock(ControlFlowElementOrBasicBlock c) { c instanceof TBasicBlock }
+
+private predicate isNonExactScope(ControlFlowElementOrBasicBlock c) {
+  c.asControlFlowElement().(ControlFlowScope).isNonExact()
+}
+
+private predicate step(ControlFlowElementOrBasicBlock pred, ControlFlowElementOrBasicBlock succ) {
+  pred.asBasicBlock().getANode().getAstNode() = succ.asControlFlowElement()
   or
-  result = getANonExactScopeChild(scope).getAChild()
+  pred.asControlFlowElement() = succ.asControlFlowElement().getAChild()
 }
 
+private predicate basicBlockInNonExactScope(
+  ControlFlowElementOrBasicBlock bb, ControlFlowElementOrBasicBlock scope
+) = doublyBoundedFastTC(step/2, isBasicBlock/1, isNonExactScope/1)(bb, scope)
+
 pragma[noinline]
 private ControlFlow::BasicBlock getABasicBlockInScope(ControlFlowScope scope, boolean exactScope) {
-  result.getANode().getAstNode() = getANonExactScopeChild(scope) and
+  basicBlockInNonExactScope(TBasicBlock(result), TControlFlowElement(scope)) and
   exactScope = false
   or
   scope.isExact() and

csharp/ql/lib/semmle/code/csharp/exprs/Expr.qll

@@ -21,6 +21,7 @@ import semmle.code.csharp.Type
 private import semmle.code.csharp.ExprOrStmtParent
 private import semmle.code.csharp.frameworks.System
 private import semmle.code.csharp.TypeRef
+private import internal.Expr
 
 /**
  * An expression. Either an access (`Access`), a call (`Call`), an object or
@@ -64,14 +65,24 @@ class Expr extends ControlFlowElement, @expr {
   /** Gets the enclosing callable of this expression, if any. */
   override Callable getEnclosingCallable() { enclosingCallable(this, result) }
 
+  pragma[nomagic]
+  private predicate isExpandedAssignmentRValueDescendant() {
+    this =
+      any(AssignOperation op).getExpandedAssignment().getRValue().getChildExpr(0).getAChildExpr()
+    or
+    exists(Expr parent |
+      parent.isExpandedAssignmentRValueDescendant() and
+      this = parent.getAChildExpr()
+    )
+  }
+
   /**
    * Holds if this expression is generated by the compiler and does not appear
    * explicitly in the source code.
    */
   final predicate isImplicit() {
     compiler_generated(this) or
-    this =
-      any(AssignOperation op).getExpandedAssignment().getRValue().getChildExpr(0).getAChildExpr+()
+    this.isExpandedAssignmentRValueDescendant()
   }
 
   /**
@@ -1133,7 +1144,7 @@ class TupleExpr extends Expr, @tuple_expr {
   /** Holds if this expression is a tuple construction. */
   predicate isConstruction() {
     not this = getAnAssignOrForeachChild() and
-    not this = any(PatternExpr pe).getAChildExpr*()
+    not isPatternExprDescendant(this)
   }
 
   override string getAPrimaryQlClass() { result = "TupleExpr" }

csharp/ql/lib/semmle/code/csharp/exprs/internal/Expr.qll

@@ -0,0 +1,11 @@
+private import csharp
+
+pragma[nomagic]
+predicate isPatternExprDescendant(Expr e) {
+  e instanceof PatternExpr
+  or
+  exists(Expr parent |
+    isPatternExprDescendant(parent) and
+    e = parent.getAChildExpr()
+  )
+}

csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql

@@ -15,23 +15,6 @@
 
 import csharp
 
-/** An expression containing a qualified member access, a method call, or an array access. */
-class DangerousExpression extends Expr {
-  DangerousExpression() {
-    exists(Expr e | this = e.getParent*() |
-      exists(Expr q | q = e.(MemberAccess).getQualifier() |
-        not q instanceof ThisAccess and
-        not q instanceof BaseAccess
-      )
-      or
-      e instanceof MethodCall
-      or
-      e instanceof ArrayAccess
-    ) and
-    not exists(Expr e | this = e.getParent*() | e.(Call).getTarget().getAParameter().isOutOrRef())
-  }
-}
-
 /** A use of `&` or `|` on operands of type boolean. */
 class NonShortCircuit extends BinaryBitwiseOperation {
   NonShortCircuit() {
@@ -42,10 +25,40 @@ class NonShortCircuit extends BinaryBitwiseOperation {
     ) and
     not exists(AssignBitwiseOperation abo | abo.getExpandedAssignment().getRValue() = this) and
     this.getLeftOperand().getType() instanceof BoolType and
-    this.getRightOperand().getType() instanceof BoolType and
-    this.getRightOperand() instanceof DangerousExpression
+    this.getRightOperand().getType() instanceof BoolType
+  }
+
+  pragma[nomagic]
+  private predicate hasRightOperandDescendant(Expr e) {
+    e = this.getRightOperand()
+    or
+    exists(Expr parent |
+      this.hasRightOperandDescendant(parent) and
+      e.getParent() = parent
+    )
+  }
+
+  /**
+   * Holds if this non-short-circuit expression contains a qualified member access,
+   * a method call, or an array access inside the right operand.
+   */
+  predicate isDangerous() {
+    exists(Expr e | this.hasRightOperandDescendant(e) |
+      exists(Expr q | q = e.(MemberAccess).getQualifier() |
+        not q instanceof ThisAccess and
+        not q instanceof BaseAccess
+      )
+      or
+      e instanceof MethodCall
+      or
+      e instanceof ArrayAccess
+    ) and
+    not exists(Expr e | this.hasRightOperandDescendant(e) |
+      e.(Call).getTarget().getAParameter().isOutOrRef()
+    )
   }
 }
 
 from NonShortCircuit e
+where e.isDangerous()
 select e, "Potentially dangerous use of non-short circuit logic."