C#: Strengthen call-back heuristics by considering body-less methods

2026-04-27 17:55:19 +02:00 · 2023-11-17 12:17:21 +01:00
parent e1c326642c
commit ccb9d9b8fa
9 changed files with 46 additions and 38 deletions
--- 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. */
--- 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())
  }
--- 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()
  }

--- 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()
+    )
  }
 }

--- 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 AddEventAccessor then not c.fromSource() else any())
 }

 private class SummarizedCallableWithCallback extends SummarizedCallable {
--- 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() }
--- 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 AddEventAccessor then not target.fromSource() else any()
+      ) and
      e = c.getAnArgument() and
      e.getType() instanceof SystemLinqExpressions::DelegateExtType and
      libraryDelegateCall = true
--- 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 @@ namespace My.Qltest
        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()
--- 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 |