Merge pull request github#14832 from hvitved/csharp/callback-heuristics

hvitved · web-flow · commit 4c8861a60fc9 · 2023-12-04T12:27:37.000+01:00
C#: Strengthen call-back heuristics by considering body-less methods
diff --git a/csharp/ql/lib/semmle/code/csharp/Callable.qll b/csharp/ql/lib/semmle/code/csharp/Callable.qll
@@ -105,7 +105,10 @@ class Callable extends DotNet::Callable, Parameterizable, ExprOrStmtParent, @cal
    * then both `{ return 0; }` and `{ return 1; }` are statement bodies of
    * `N.C.M()`.
    */
-  final BlockStmt getStatementBody() { result = this.getAChildStmt() }
+  final BlockStmt getStatementBody() {
+    result = getStatementBody(this) and
+    not this.getFile().isStub()
+  }
 
   /**
    * DEPRECATED: Use `getStatementBody` instead.
@@ -143,8 +146,8 @@ class Callable extends DotNet::Callable, Parameterizable, ExprOrStmtParent, @cal
    * then both `0` and `1` are expression bodies of `N.C.M()`.
    */
   final Expr getExpressionBody() {
-    result = this.getAChildExpr() and
-    not result = this.(Constructor).getInitializer()
+    result = getExpressionBody(this) and
+    not this.getFile().isStub()
   }
 
   /** Holds if this callable has an expression body. */
diff --git a/csharp/ql/lib/semmle/code/csharp/ExprOrStmtParent.qll b/csharp/ql/lib/semmle/code/csharp/ExprOrStmtParent.qll
@@ -53,6 +53,20 @@ class TopLevelExprParent extends Element, @top_level_expr_parent {
 
 private predicate hasNoSourceLocation(Element e) { not e.getALocation() instanceof SourceLocation }
 
+/** INTERNAL: Do not use. */
+Expr getExpressionBody(Callable c) {
+  result = c.getAChildExpr() and
+  not result = c.(Constructor).getInitializer()
+}
+
+/** INTERNAL: Do not use. */
+BlockStmt getStatementBody(Callable c) { result = c.getAChildStmt() }
+
+private ControlFlowElement getBody(Callable c) {
+  result = getExpressionBody(c) or
+  result = getStatementBody(c)
+}
+
 cached
 private module Cached {
   cached
@@ -161,20 +175,20 @@ private module Cached {
 
   private predicate parent(ControlFlowElement child, ExprOrStmtParent parent) {
     child = getAChild(parent) and
-    not child = any(Callable c).getBody()
+    not child = getBody(_)
   }
 
   /** Holds if the enclosing body of `cfe` is `body`. */
   cached
   predicate enclosingBody(ControlFlowElement cfe, ControlFlowElement body) {
-    body = any(Callable c).getBody() and
+    body = getBody(_) and
     parent*(enclosingStart(cfe), body)
   }
 
   /** Holds if the enclosing callable of `cfe` is `c`. */
   cached
   predicate enclosingCallable(ControlFlowElement cfe, Callable c) {
-    enclosingBody(cfe, c.getBody())
+    enclosingBody(cfe, getBody(c))
     or
     parent*(enclosingStart(cfe), c.(Constructor).getInitializer())
   }
diff --git a/csharp/ql/lib/semmle/code/csharp/File.qll b/csharp/ql/lib/semmle/code/csharp/File.qll
@@ -54,14 +54,14 @@ class File extends Container, Impl::File {
 
   /** Holds if this file is a QL test stub file. */
   pragma[noinline]
-  private predicate isStub() {
+  predicate isStub() {
     this.extractedQlTest() and
     this.getAbsolutePath().matches("%resources/stubs/%")
   }
 
   /** Holds if this file contains source code. */
   final predicate fromSource() {
-    this.getExtension() = "cs" and
+    this.getExtension() = ["cs", "cshtml"] and
     not this.isStub()
   }
 
diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll
@@ -13,11 +13,14 @@ private import semmle.code.csharp.commons.Compilation
 /** An element that defines a new CFG scope. */
 class CfgScope extends Element, @top_level_exprorstmt_parent {
   CfgScope() {
-    this instanceof Callable
-    or
-    // For now, static initializer values have their own scope. Eventually, they
-    // should be treated like instance initializers.
-    this.(Assignable).(Modifiable).isStatic()
+    this.getFile().fromSource() and
+    (
+      this instanceof Callable
+      or
+      // For now, static initializer values have their own scope. Eventually, they
+      // should be treated like instance initializers.
+      this.(Assignable).(Modifiable).isStatic()
+    )
   }
 }
 
diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/FlowSummary.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/FlowSummary.qll
@@ -168,7 +168,8 @@ private SummaryComponent delegateSelf() {
 
 private predicate mayInvokeCallback(Callable c, int n) {
   c.getParameter(n).getType() instanceof SystemLinqExpressions::DelegateExtType and
-  not c.fromSource()
+  not c.hasBody() and
+  (if c instanceof Accessor then not c.fromSource() else any())
 }
 
 private class SummarizedCallableWithCallback extends SummarizedCallable {
diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowDispatch.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowDispatch.qll
@@ -81,9 +81,9 @@ newtype TReturnKind =
  */
 class DataFlowSummarizedCallable instanceof FlowSummary::SummarizedCallable {
   DataFlowSummarizedCallable() {
-    not this.fromSource()
+    not this.hasBody()
     or
-    this.fromSource() and not this.applyGeneratedModel()
+    this.hasBody() and not this.applyGeneratedModel()
   }
 
   string toString() { result = super.toString() }
diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll
@@ -310,7 +310,12 @@ private module CallGraph {
       c = any(DelegateCall dc | e = dc.getExpr()) and
       libraryDelegateCall = false
       or
-      c.getTarget().fromLibrary() and
+      exists(Callable target |
+        target = c.getTarget() and
+        not target.hasBody()
+      |
+        if target instanceof Accessor then not target.fromSource() else any()
+      ) and
       e = c.getAnArgument() and
       e.getType() instanceof SystemLinqExpressions::DelegateExtType and
       libraryDelegateCall = true
diff --git a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.cs b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.cs
@@ -185,16 +185,16 @@ public class G
         void M1()
         {
             var o = new object();
-            Sink(GeneratedFlow(o));
+            Sink(GeneratedFlow(o)); // no flow because the modelled method exists in source code
         }
 
         void M2()
         {
             var o1 = new object();
-            Sink(GeneratedFlowArgs(o1, null));
+            Sink(GeneratedFlowArgs(o1, null)); // no flow because the modelled method exists in source code
 
             var o2 = new object();
-            Sink(GeneratedFlowArgs(null, o2));
+            Sink(GeneratedFlowArgs(null, o2)); // no flow because the modelled method exists in source code
         }
 
         void M3()
diff --git a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.expected b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.expected
@@ -61,12 +61,6 @@ edges
 | ExternalFlow.cs:118:21:118:30 | call to method Reverse : null [element] : Object | ExternalFlow.cs:120:18:120:18 | access to local variable b : null [element] : Object |
 | ExternalFlow.cs:118:29:118:29 | access to local variable a : null [element] : Object | ExternalFlow.cs:118:21:118:30 | call to method Reverse : null [element] : Object |
 | ExternalFlow.cs:120:18:120:18 | access to local variable b : null [element] : Object | ExternalFlow.cs:120:18:120:21 | access to array element |
-| ExternalFlow.cs:187:21:187:32 | object creation of type Object : Object | ExternalFlow.cs:188:32:188:32 | access to local variable o : Object |
-| ExternalFlow.cs:188:32:188:32 | access to local variable o : Object | ExternalFlow.cs:188:18:188:33 | call to method GeneratedFlow |
-| ExternalFlow.cs:193:22:193:33 | object creation of type Object : Object | ExternalFlow.cs:194:36:194:37 | access to local variable o1 : Object |
-| ExternalFlow.cs:194:36:194:37 | access to local variable o1 : Object | ExternalFlow.cs:194:18:194:44 | call to method GeneratedFlowArgs |
-| ExternalFlow.cs:196:22:196:33 | object creation of type Object : Object | ExternalFlow.cs:197:42:197:43 | access to local variable o2 : Object |
-| ExternalFlow.cs:197:42:197:43 | access to local variable o2 : Object | ExternalFlow.cs:197:18:197:44 | call to method GeneratedFlowArgs |
 | ExternalFlow.cs:205:22:205:33 | object creation of type Object : Object | ExternalFlow.cs:206:38:206:39 | access to local variable o2 : Object |
 | ExternalFlow.cs:206:38:206:39 | access to local variable o2 : Object | ExternalFlow.cs:206:18:206:40 | call to method MixedFlowArgs |
 | ExternalFlow.cs:231:21:231:28 | object creation of type HC : HC | ExternalFlow.cs:232:21:232:21 | access to local variable h : HC |
@@ -151,15 +145,6 @@ nodes
 | ExternalFlow.cs:118:29:118:29 | access to local variable a : null [element] : Object | semmle.label | access to local variable a : null [element] : Object |
 | ExternalFlow.cs:120:18:120:18 | access to local variable b : null [element] : Object | semmle.label | access to local variable b : null [element] : Object |
 | ExternalFlow.cs:120:18:120:21 | access to array element | semmle.label | access to array element |
-| ExternalFlow.cs:187:21:187:32 | object creation of type Object : Object | semmle.label | object creation of type Object : Object |
-| ExternalFlow.cs:188:18:188:33 | call to method GeneratedFlow | semmle.label | call to method GeneratedFlow |
-| ExternalFlow.cs:188:32:188:32 | access to local variable o : Object | semmle.label | access to local variable o : Object |
-| ExternalFlow.cs:193:22:193:33 | object creation of type Object : Object | semmle.label | object creation of type Object : Object |
-| ExternalFlow.cs:194:18:194:44 | call to method GeneratedFlowArgs | semmle.label | call to method GeneratedFlowArgs |
-| ExternalFlow.cs:194:36:194:37 | access to local variable o1 : Object | semmle.label | access to local variable o1 : Object |
-| ExternalFlow.cs:196:22:196:33 | object creation of type Object : Object | semmle.label | object creation of type Object : Object |
-| ExternalFlow.cs:197:18:197:44 | call to method GeneratedFlowArgs | semmle.label | call to method GeneratedFlowArgs |
-| ExternalFlow.cs:197:42:197:43 | access to local variable o2 : Object | semmle.label | access to local variable o2 : Object |
 | ExternalFlow.cs:205:22:205:33 | object creation of type Object : Object | semmle.label | object creation of type Object : Object |
 | ExternalFlow.cs:206:18:206:40 | call to method MixedFlowArgs | semmle.label | call to method MixedFlowArgs |
 | ExternalFlow.cs:206:38:206:39 | access to local variable o2 : Object | semmle.label | access to local variable o2 : Object |
@@ -189,8 +174,5 @@ subpaths
 | ExternalFlow.cs:104:18:104:25 | access to field Field | ExternalFlow.cs:98:24:98:35 | object creation of type Object : Object | ExternalFlow.cs:104:18:104:25 | access to field Field | $@ | ExternalFlow.cs:98:24:98:35 | object creation of type Object : Object | object creation of type Object : Object |
 | ExternalFlow.cs:112:18:112:25 | access to property MyProp | ExternalFlow.cs:111:24:111:35 | object creation of type Object : Object | ExternalFlow.cs:112:18:112:25 | access to property MyProp | $@ | ExternalFlow.cs:111:24:111:35 | object creation of type Object : Object | object creation of type Object : Object |
 | ExternalFlow.cs:120:18:120:21 | access to array element | ExternalFlow.cs:117:36:117:47 | object creation of type Object : Object | ExternalFlow.cs:120:18:120:21 | access to array element | $@ | ExternalFlow.cs:117:36:117:47 | object creation of type Object : Object | object creation of type Object : Object |
-| ExternalFlow.cs:188:18:188:33 | call to method GeneratedFlow | ExternalFlow.cs:187:21:187:32 | object creation of type Object : Object | ExternalFlow.cs:188:18:188:33 | call to method GeneratedFlow | $@ | ExternalFlow.cs:187:21:187:32 | object creation of type Object : Object | object creation of type Object : Object |
-| ExternalFlow.cs:194:18:194:44 | call to method GeneratedFlowArgs | ExternalFlow.cs:193:22:193:33 | object creation of type Object : Object | ExternalFlow.cs:194:18:194:44 | call to method GeneratedFlowArgs | $@ | ExternalFlow.cs:193:22:193:33 | object creation of type Object : Object | object creation of type Object : Object |
-| ExternalFlow.cs:197:18:197:44 | call to method GeneratedFlowArgs | ExternalFlow.cs:196:22:196:33 | object creation of type Object : Object | ExternalFlow.cs:197:18:197:44 | call to method GeneratedFlowArgs | $@ | ExternalFlow.cs:196:22:196:33 | object creation of type Object : Object | object creation of type Object : Object |
 | ExternalFlow.cs:206:18:206:40 | call to method MixedFlowArgs | ExternalFlow.cs:205:22:205:33 | object creation of type Object : Object | ExternalFlow.cs:206:18:206:40 | call to method MixedFlowArgs | $@ | ExternalFlow.cs:205:22:205:33 | object creation of type Object : Object | object creation of type Object : Object |
 | ExternalFlow.cs:233:18:233:18 | access to local variable o | ExternalFlow.cs:231:21:231:28 | object creation of type HC : HC | ExternalFlow.cs:233:18:233:18 | access to local variable o | $@ | ExternalFlow.cs:231:21:231:28 | object creation of type HC : HC | object creation of type HC : HC |

Original file line number	Diff line number	Diff line change
`@@ -54,14 +54,14 @@ class File extends Container, Impl::File {`
`54`	`54`
`55`	`55`	`/** Holds if this file is a QL test stub file. */`
`56`	`56`	`pragma[noinline]`
`57`		`- private predicate isStub() {`
	`57`	`+ predicate isStub() {`
`58`	`58`	`this.extractedQlTest() and`
`59`	`59`	`this.getAbsolutePath().matches("%resources/stubs/%")`
`60`	`60`	`}`
`61`	`61`
`62`	`62`	`/** Holds if this file contains source code. */`
`63`	`63`	`final predicate fromSource() {`
`64`		`- this.getExtension() = "cs" and`
	`64`	`+ this.getExtension() = ["cs", "cshtml"] and`
`65`	`65`	`not this.isStub()`
`66`	`66`	`}`
`67`	`67`
Original file line number	Diff line number	Diff line change
`@@ -168,7 +168,8 @@ private SummaryComponent delegateSelf() {`
`168`	`168`
`169`	`169`	`private predicate mayInvokeCallback(Callable c, int n) {`
`170`	`170`	`c.getParameter(n).getType() instanceof SystemLinqExpressions::DelegateExtType and`
`171`		`- not c.fromSource()`
	`171`	`+ not c.hasBody() and`
	`172`	`+ (if c instanceof Accessor then not c.fromSource() else any())`
`172`	`173`	`}`
`173`	`174`
`174`	`175`	`private class SummarizedCallableWithCallback extends SummarizedCallable {`
Original file line number	Diff line number	Diff line change
`@@ -185,16 +185,16 @@ public class G`
`185`	`185`	`void M1()`
`186`	`186`	`{`
`187`	`187`	`var o = new object();`
`188`		`- Sink(GeneratedFlow(o));`
	`188`	`+ Sink(GeneratedFlow(o)); // no flow because the modelled method exists in source code`
`189`	`189`	`}`
`190`	`190`
`191`	`191`	`void M2()`
`192`	`192`	`{`
`193`	`193`	`var o1 = new object();`
`194`		`- Sink(GeneratedFlowArgs(o1, null));`
	`194`	`+ Sink(GeneratedFlowArgs(o1, null)); // no flow because the modelled method exists in source code`
`195`	`195`
`196`	`196`	`var o2 = new object();`
`197`		`- Sink(GeneratedFlowArgs(null, o2));`
	`197`	`+ Sink(GeneratedFlowArgs(null, o2)); // no flow because the modelled method exists in source code`
`198`	`198`	`}`
`199`	`199`
`200`	`200`	`void M3()`