Merge pull request #355 from hvitved/csharp/guards-logic

Approved by calumgrant

Author: semmle-qlci

csharp/ql/src/Performance/UseTryGetValue.ql

@@ -11,7 +11,7 @@
 
 import csharp
 import semmle.code.csharp.commons.StructuralComparison
-import semmle.code.csharp.controlflow.Guards
+import semmle.code.csharp.controlflow.Guards as G
 
 class SameElement extends StructuralComparisonConfiguration
 {
@@ -22,7 +22,7 @@ class SameElement extends StructuralComparisonConfiguration
     exists(MethodCall mc, IndexerRead access |
       mc.getTarget().hasName("ContainsKey")
       and
-      access.getQualifier().(GuardedExpr).isGuardedBy(mc, mc.getQualifier(), _)
+      access.getQualifier().(G::GuardedExpr).isGuardedBy(mc, mc.getQualifier(), _)
       and
       e1 = mc.getArgument(0)
       and

csharp/ql/src/Security Features/CWE-022/ZipSlip.ql

@@ -16,4 +16,4 @@ import semmle.code.csharp.security.dataflow.ZipSlip::ZipSlip
 
 from TaintTrackingConfiguration zipTaintTracking, DataFlow::Node source, DataFlow::Node sink
 where zipTaintTracking.hasFlow(source, sink)
-select sink, "Unsanitized zip archive $@, which may contain '..', is used in a file system operation.", source, "item path"
+select sink, "Unsanitized zip archive $@, which may contain '..', is used in a file system operation.", source, "item path"

csharp/ql/src/Security Features/CWE-022/ZipSlipBad.cs

@@ -9,4 +9,4 @@ public static void WriteToDirectory(ZipArchiveEntry entry,
         string destFileName = Path.Combine(destDirectory, entry.FullName);
         entry.ExtractToFile(destFileName);
     }
-}
+}

csharp/ql/src/Stubs/Stubs.qll

@@ -1,9 +1,9 @@
-/** 
+/**
  * Generates C# stubs for use in test code.
  *
  * Extend the abstract class `GeneratedDeclaration` with the declarations that should be generated.
  * This will generate stubs for all the required dependencies as well.
- * 
+ *
  * Use
  * ```
  * select generatedCode()
@@ -104,7 +104,7 @@ private abstract class GeneratedType extends ValueOrRefType, GeneratedElement {
       result = this.stubComment() +
       this.stubAttributes() +
       this.stubAbstractModifier() +
-      this.stubStaticModifier() + 
+      this.stubStaticModifier() +
       this.stubAccessibilityModifier() +
       this.stubKeyword() + " " +
       this.getUndecoratedName() +

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

@@ -144,7 +144,7 @@ class ForwarderAssertMethod extends AssertMethod {
   override ExceptionClass getExceptionClass() {
     result = this.getUnderlyingAssertMethod().getExceptionClass()
   }
-  
+
   /** Gets the underlying assertion method that is being forwarded to. */
   AssertMethod getUnderlyingAssertMethod() { result = a.getAssertMethod() }
 }

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

@@ -152,7 +152,7 @@ abstract class StructuralComparisonConfiguration extends string {
  */
 module Internal {
   // Import all uses of the internal library to make sure caching works
-  private import semmle.code.csharp.controlflow.Guards
+  private import semmle.code.csharp.controlflow.Guards as G
 
   /**
    * A configuration for performing structural comparisons of program elements

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

@@ -524,6 +524,7 @@ class ConditionBlock extends BasicBlock {
   //
   // In the former case, `x` and `y` control `A`, in the latter case
   // only `x & y` controls `A` if we do not take sub conditions into account.
+  deprecated
   predicate controlsSubCond(BasicBlock controlled, boolean testIsTrue, Expr cond, boolean condIsTrue) {
     impliesSub(getLastNode().getElement(), cond, testIsTrue, condIsTrue) and
     controls(controlled, any(BooleanSuccessor s | s.getValue() = testIsTrue))
@@ -542,6 +543,7 @@ class ConditionBlock extends BasicBlock {
  * Holds if `e2` is a sub expression of (Boolean) expression `e1`, and
  * if `e1` has value `b1` then `e2` must have value `b2`.
  */
+deprecated
 private predicate impliesSub(Expr e1, Expr e2, boolean b1, boolean b2) {
   if e1 instanceof LogicalNotExpr then (
     impliesSub(e1.(LogicalNotExpr).getOperand(), e2, b1.booleanNot(), b2)

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

@@ -430,12 +430,20 @@ private predicate mustHaveMatchingCompletion(ControlFlowElement cfe) {
   cfe instanceof SpecificCatchClause
 }
 
+/**
+ * Holds if `cfe` is the element inside foreach statement `fs` that has the emptiness
+ * completion.
+ */
+predicate foreachEmptiness(ForeachStmt fs, ControlFlowElement cfe) {
+  cfe = fs // use `foreach` statement itself to represent the emptiness test
+}
+
 /**
  * Holds if a normal completion of `cfe` must be an emptiness completion. Thats is,
  * whether `cfe` determines whether to execute the body of a `foreach` statement.
  */
 private predicate mustHaveEmptinessCompletion(ControlFlowElement cfe) {
-  cfe instanceof ForeachStmt // use `foreach` statement itself to represent the emptiness test
+  foreachEmptiness(_, cfe)
 }
 
 /**

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

@@ -2593,6 +2593,15 @@ module ControlFlow {
           this = TPhiPreSsaDef(_, result)
         }
 
+        LocalScopeVariableRead getARead() {
+          firstReadSameVar(this, result)
+          or
+          exists(LocalScopeVariableRead read |
+            firstReadSameVar(this, read) |
+            adjacentReadPairSameVar+(read, result)
+          )
+        }
+
         Location getLocation() {
           exists(AssignableDefinition def |
             this = TExplicitPreSsaDef(_, _, def, _) |
@@ -2720,21 +2729,29 @@ module ControlFlow {
         ssaRefRank(bb2, i2, v, _) = 1
       }
 
-      predicate firstReadSameVar(Definition def, LocalScopeVariableRead read) {
-        exists(SimpleLocalScopeVariable v, PreBasicBlock b1, int i1, PreBasicBlock b2, int i2 |
-          adjacentVarRefs(v, b1, i1, b2, i2) and
-          defAt(b1, i1, def, v) and
-          readAt(b2, i2, read, v)
-        )
-      }
+      private cached module PreSsaCached {
+        cached
+        predicate firstReadSameVar(Definition def, LocalScopeVariableRead read) {
+          exists(SimpleLocalScopeVariable v, PreBasicBlock b1, int i1, PreBasicBlock b2, int i2 |
+            adjacentVarRefs(v, b1, i1, b2, i2) and
+            defAt(b1, i1, def, v) and
+            readAt(b2, i2, read, v)
+          )
+        }
 
-      predicate adjacentReadPairSameVar(LocalScopeVariableRead read1, LocalScopeVariableRead read2) {
-        exists(SimpleLocalScopeVariable v, PreBasicBlock bb1, int i1, PreBasicBlock bb2, int i2 |
-          adjacentVarRefs(v, bb1, i1, bb2, i2) and
-          readAt(bb1, i1, read1, v) and
-          readAt(bb2, i2, read2, v)
-        )
+        cached
+        predicate adjacentReadPairSameVar(LocalScopeVariableRead read1, LocalScopeVariableRead read2) {
+          exists(SimpleLocalScopeVariable v, PreBasicBlock bb1, int i1, PreBasicBlock bb2, int i2 |
+            adjacentVarRefs(v, bb1, i1, bb2, i2) and
+            readAt(bb1, i1, read1, v) and
+            readAt(bb2, i2, read2, v)
+          )
+        }
+
+        cached
+        predicate forceCachingInSameStage() { any() }
       }
+      import PreSsaCached
     }
 
     /**
@@ -3327,11 +3344,7 @@ module ControlFlow {
            * Holds if condition `cb` is a read of the SSA variable in this split.
            */
           private predicate defCondition(ConditionBlock cb) {
-            exists(LocalScopeVariableRead read1, LocalScopeVariableRead read2 |
-              firstReadSameVar(def, read1) |
-              adjacentReadPairSameVar*(read1, read2) and
-              read2 = cb.getLastElement()
-            )
+            cb.getLastElement() = def.getARead()
           }
 
           /**
@@ -3827,6 +3840,7 @@ module ControlFlow {
       cached
       newtype TPreSsaDef =
         TExplicitPreSsaDef(PreBasicBlocks::PreBasicBlock bb, int i, AssignableDefinition def, LocalScopeVariable v) {
+          PreSsa::forceCachingInSameStage() and
           PreSsa::assignableDefAt(bb, i, def, v)
         }
         or