Merge pull request #579 from hvitved/csharp/guards-loop

calumgrant · web-flow · commit ca72c8ebfe66 · 2018-11-30T10:27:21.000Z
C#: Fix bug in guards library when the guarded expression is in a loop
diff --git a/csharp/ql/src/semmle/code/csharp/commons/Assertions.qll b/csharp/ql/src/semmle/code/csharp/commons/Assertions.qll
@@ -55,7 +55,8 @@ class Assertion extends MethodCall {
       bb = this.getAControlFlowNode().getBasicBlock() |
       result = bb.getASuccessor*()
     ) and
-    result.getASuccessor() = jb
+    result.getASuccessor() = jb and
+    not jb.dominates(result)
   }
 
   pragma[nomagic]
@@ -64,6 +65,8 @@ class Assertion extends MethodCall {
     forall(BasicBlock pred |
       pred = jb.getAPredecessor() |
       pred = this.getAPossiblyDominatedPredecessor(jb)
+      or
+      jb.dominates(pred)
     )
   }
 
diff --git a/csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowElement.qll b/csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowElement.qll
@@ -106,7 +106,8 @@ class ControlFlowElement extends ExprOrStmtParent, @control_flow_element {
       this.immediatelyControls(mid, s) |
       result = mid.getASuccessor*()
     ) and
-    result.getASuccessor() = controlled
+    result.getASuccessor() = controlled and
+    not controlled.dominates(result)
   }
 
   pragma[nomagic]
@@ -115,6 +116,8 @@ class ControlFlowElement extends ExprOrStmtParent, @control_flow_element {
     forall(BasicBlock pred |
       pred = controlled.getAPredecessor() |
       pred = this.getAPossiblyControlledPredecessor(controlled, s)
+      or
+      controlled.dominates(pred)
     )
   }
 
diff --git a/csharp/ql/test/library-tests/controlflow/guards/BooleanGuardedExpr.expected b/csharp/ql/test/library-tests/controlflow/guards/BooleanGuardedExpr.expected
@@ -62,6 +62,8 @@
 | Guards.cs:194:31:194:31 | access to parameter s | Guards.cs:193:14:193:25 | call to method NullTest3 | Guards.cs:193:24:193:24 | access to parameter s | false |
 | Guards.cs:196:31:196:31 | access to parameter s | Guards.cs:195:13:195:27 | call to method NotNullTest4 | Guards.cs:195:26:195:26 | access to parameter s | true |
 | Guards.cs:198:31:198:31 | access to parameter s | Guards.cs:197:14:197:29 | call to method NullTestWrong | Guards.cs:197:28:197:28 | access to parameter s | false |
+| Guards.cs:205:13:205:13 | access to parameter o | Guards.cs:203:13:203:21 | ... != ... | Guards.cs:203:13:203:13 | access to parameter o | true |
+| Guards.cs:208:17:208:17 | access to parameter o | Guards.cs:203:13:203:21 | ... != ... | Guards.cs:203:13:203:13 | access to parameter o | true |
 | Splitting.cs:13:17:13:17 | access to parameter o | Splitting.cs:12:17:12:25 | ... != ... | Splitting.cs:12:17:12:17 | access to parameter o | true |
 | Splitting.cs:23:24:23:24 | access to parameter o | Splitting.cs:22:17:22:25 | ... != ... | Splitting.cs:22:17:22:17 | access to parameter o | true |
 | Splitting.cs:25:13:25:13 | access to parameter o | Splitting.cs:22:17:22:25 | ... != ... | Splitting.cs:22:17:22:17 | access to parameter o | false |
diff --git a/csharp/ql/test/library-tests/controlflow/guards/GuardedExpr.expected b/csharp/ql/test/library-tests/controlflow/guards/GuardedExpr.expected
@@ -155,6 +155,10 @@
 | Guards.cs:196:31:196:31 | access to parameter s | Guards.cs:195:13:195:27 | call to method NotNullTest4 | Guards.cs:195:26:195:26 | access to parameter s | true |
 | Guards.cs:196:31:196:31 | access to parameter s | Guards.cs:195:26:195:26 | access to parameter s | Guards.cs:195:26:195:26 | access to parameter s | non-null |
 | Guards.cs:198:31:198:31 | access to parameter s | Guards.cs:197:14:197:29 | call to method NullTestWrong | Guards.cs:197:28:197:28 | access to parameter s | false |
+| Guards.cs:205:13:205:13 | access to parameter o | Guards.cs:203:13:203:13 | access to parameter o | Guards.cs:203:13:203:13 | access to parameter o | non-null |
+| Guards.cs:205:13:205:13 | access to parameter o | Guards.cs:203:13:203:21 | ... != ... | Guards.cs:203:13:203:13 | access to parameter o | true |
+| Guards.cs:208:17:208:17 | access to parameter o | Guards.cs:203:13:203:13 | access to parameter o | Guards.cs:203:13:203:13 | access to parameter o | non-null |
+| Guards.cs:208:17:208:17 | access to parameter o | Guards.cs:203:13:203:21 | ... != ... | Guards.cs:203:13:203:13 | access to parameter o | true |
 | Splitting.cs:13:17:13:17 | access to parameter o | Splitting.cs:12:17:12:17 | access to parameter o | Splitting.cs:12:17:12:17 | access to parameter o | non-null |
 | Splitting.cs:13:17:13:17 | access to parameter o | Splitting.cs:12:17:12:25 | ... != ... | Splitting.cs:12:17:12:17 | access to parameter o | true |
 | Splitting.cs:23:24:23:24 | access to parameter o | Splitting.cs:22:17:22:17 | access to parameter o | Splitting.cs:22:17:22:17 | access to parameter o | non-null |
diff --git a/csharp/ql/test/library-tests/controlflow/guards/Guards.cs b/csharp/ql/test/library-tests/controlflow/guards/Guards.cs
@@ -197,4 +197,16 @@ void M16(string s)
         if (!NullTestWrong(s))
             Console.WriteLine(s); // not null guarded
     }
+
+    void M17(object o, string[] args)
+    {
+        if (o != null)
+        {
+            o.ToString(); // null guarded
+            foreach (var arg in args)
+            {
+                o.ToString(); // null guarded
+            }
+        }
+    }
 }
diff --git a/csharp/ql/test/library-tests/controlflow/guards/Implications.expected b/csharp/ql/test/library-tests/controlflow/guards/Implications.expected
@@ -200,6 +200,8 @@
 | Guards.cs:195:13:195:27 | call to method NotNullTest4 | true | Guards.cs:195:26:195:26 | access to parameter s | non-null |
 | Guards.cs:197:13:197:29 | !... | false | Guards.cs:197:14:197:29 | call to method NullTestWrong | true |
 | Guards.cs:197:13:197:29 | !... | true | Guards.cs:197:14:197:29 | call to method NullTestWrong | false |
+| Guards.cs:203:13:203:21 | ... != ... | false | Guards.cs:203:13:203:13 | access to parameter o | null |
+| Guards.cs:203:13:203:21 | ... != ... | true | Guards.cs:203:13:203:13 | access to parameter o | non-null |
 | Splitting.cs:12:17:12:25 | ... != ... | false | Splitting.cs:12:17:12:17 | access to parameter o | null |
 | Splitting.cs:12:17:12:25 | ... != ... | true | Splitting.cs:12:17:12:17 | access to parameter o | non-null |
 | Splitting.cs:22:17:22:25 | ... != ... | false | Splitting.cs:22:17:22:17 | access to parameter o | null |
diff --git a/csharp/ql/test/library-tests/controlflow/guards/NullGuardedExpr.expected b/csharp/ql/test/library-tests/controlflow/guards/NullGuardedExpr.expected
@@ -39,6 +39,8 @@
 | Guards.cs:192:31:192:31 | access to parameter s |
 | Guards.cs:194:31:194:31 | access to parameter s |
 | Guards.cs:196:31:196:31 | access to parameter s |
+| Guards.cs:205:13:205:13 | access to parameter o |
+| Guards.cs:208:17:208:17 | access to parameter o |
 | Splitting.cs:13:17:13:17 | access to parameter o |
 | Splitting.cs:23:24:23:24 | access to parameter o |
 | Splitting.cs:35:13:35:13 | access to parameter o |

Original file line number	Diff line number	Diff line change
`@@ -55,7 +55,8 @@ class Assertion extends MethodCall {`
`55`	`55`	`bb = this.getAControlFlowNode().getBasicBlock() \|`
`56`	`56`	`result = bb.getASuccessor*()`
`57`	`57`	`) and`
`58`		`- result.getASuccessor() = jb`
	`58`	`+ result.getASuccessor() = jb and`
	`59`	`+ not jb.dominates(result)`
`59`	`60`	`}`
`60`	`61`
`61`	`62`	`pragma[nomagic]`
`@@ -64,6 +65,8 @@ class Assertion extends MethodCall {`
`64`	`65`	`forall(BasicBlock pred \|`
`65`	`66`	`pred = jb.getAPredecessor() \|`
`66`	`67`	`pred = this.getAPossiblyDominatedPredecessor(jb)`
	`68`	`+ or`
	`69`	`+ jb.dominates(pred)`
`67`	`70`	`)`
`68`	`71`	`}`
`69`	`72`
Original file line number	Diff line number	Diff line change
`@@ -106,7 +106,8 @@ class ControlFlowElement extends ExprOrStmtParent, @control_flow_element {`
`106`	`106`	`this.immediatelyControls(mid, s) \|`
`107`	`107`	`result = mid.getASuccessor*()`
`108`	`108`	`) and`
`109`		`- result.getASuccessor() = controlled`
	`109`	`+ result.getASuccessor() = controlled and`
	`110`	`+ not controlled.dominates(result)`
`110`	`111`	`}`
`111`	`112`
`112`	`113`	`pragma[nomagic]`
`@@ -115,6 +116,8 @@ class ControlFlowElement extends ExprOrStmtParent, @control_flow_element {`
`115`	`116`	`forall(BasicBlock pred \|`
`116`	`117`	`pred = controlled.getAPredecessor() \|`
`117`	`118`	`pred = this.getAPossiblyControlledPredecessor(controlled, s)`
	`119`	`+ or`
	`120`	`+ controlled.dominates(pred)`
`118`	`121`	`)`
`119`	`122`	`}`
`120`	`123`