Merge pull request #1048 from jbj/dataflow-link-targets

C++: Data flow dispatch across link targets

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowDispatch.qll

@@ -2,11 +2,59 @@ private import cpp
 private import DataFlowPrivate
 
 Function viableImpl(MethodAccess ma) {
-  result = ma.getTarget()
+  result = viableCallable(ma)
 }
 
+/**
+ * Gets a function that might be called by `call`.
+ */
 Function viableCallable(Call call) {
   result = call.getTarget()
+  or
+  // If the target of the call does not have a body in the snapshot, it might
+  // be because the target is just a header declaration, and the real target
+  // will be determined at run time when the caller and callee are linked
+  // together by the operating system's dynamic linker. In case a _unique_
+  // function with the right signature is present in the database, we return
+  // that as a potential callee.
+  exists(string qualifiedName, int nparams |
+    callSignatureWithoutBody(qualifiedName, nparams, call) and
+    functionSignatureWithBody(qualifiedName, nparams, result) and
+    strictcount(Function other | functionSignatureWithBody(qualifiedName, nparams, other)) = 1
+  )
+}
+
+/**
+ * Holds if `f` is a function with a body that has name `qualifiedName` and
+ * `nparams` parameter count. See `functionSignature`.
+ */
+private predicate functionSignatureWithBody(string qualifiedName, int nparams, Function f) {
+  functionSignature(f, qualifiedName, nparams) and
+  exists(f.getBlock())
+}
+
+/**
+ * Holds if the target of `call` is a function _with no definition_ that has
+ * name `qualifiedName` and `nparams` parameter count. See `functionSignature`.
+ */
+pragma[noinline]
+private predicate callSignatureWithoutBody(string qualifiedName, int nparams, Call call) {
+  exists(Function target |
+    target = call.getTarget() and
+    not exists(target.getBlock()) and
+    functionSignature(target, qualifiedName, nparams)
+  )
+}
+
+/**
+ * Holds if `f` has name `qualifiedName` and `nparams` parameter count. This is
+ * an approximation of its signature for the purpose of matching functions that
+ * might be the same across link targets.
+ */
+private predicate functionSignature(Function f, string qualifiedName, int nparams) {
+  qualifiedName = f.getQualifiedName() and
+  nparams = f.getNumberOfParameters() and
+  not f.isStatic()
 }
 
 /**

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowDispatch.qll

@@ -1,73 +1 @@
-private import cpp
-private import DataFlowPrivate
-
-Function viableImpl(MethodAccess ma) {
-  result = ma.getTarget()
-}
-
-Function viableCallable(Call call) {
-  result = call.getTarget()
-}
-
-/**
- * Holds if the call context `ctx` reduces the set of viable dispatch
- * targets of `ma` in `c`.
- */
-predicate reducedViableImplInCallContext(MethodAccess ma, Callable c, Call ctx) {
-  none()
-}
-
-/**
- * Gets a viable dispatch target of `ma` in the context `ctx`. This is
- * restricted to those `ma`s for which a context might make a difference.
- */
-private Method viableImplInCallContext(MethodAccess ma, Call ctx) {
-  // stub implementation
-  result = viableImpl(ma) and
-  viableCallable(ctx) = ma.getEnclosingFunction()
-}
-
-/**
- * Gets a viable dispatch target of `ma` in the context `ctx`. This is
- * restricted to those `ma`s for which the context makes a difference.
- */
-Method prunedViableImplInCallContext(MethodAccess ma, Call ctx) {
-  result = viableImplInCallContext(ma, ctx) and
-  reducedViableImplInCallContext(ma, _, ctx)
-}
-
-/**
- * Holds if data might flow from `ma` to a return statement in some
- * configuration.
- */
-private predicate maybeChainedReturn(MethodAccess ma) {
-  exists(ReturnStmt ret |
-    exists(ret.getExpr()) and
-    ret.getEnclosingFunction() = ma.getEnclosingFunction() and
-    not ma.getParent() instanceof ExprStmt
-  )
-}
-
-/**
- * Holds if flow returning from `m` to `ma` might return further and if
- * this path restricts the set of call sites that can be returned to.
- */
-predicate reducedViableImplInReturn(Method m, MethodAccess ma) {
-  exists(int tgts, int ctxtgts |
-    m = viableImpl(ma) and
-    ctxtgts = count(Call ctx | m = viableImplInCallContext(ma, ctx)) and
-    tgts = strictcount(Call ctx | viableCallable(ctx) = ma.getEnclosingFunction()) and
-    ctxtgts < tgts
-  ) and
-  maybeChainedReturn(ma)
-}
-
-/**
- * Gets a viable dispatch target of `ma` in the context `ctx`. This is
- * restricted to those `ma`s and results for which the return flow from the
- * result to `ma` restricts the possible context `ctx`.
- */
-Method prunedViableImplInCallContextReverse(MethodAccess ma, Call ctx) {
-  result = viableImplInCallContext(ma, ctx) and
-  reducedViableImplInReturn(result, ma)
-}
+import semmle.code.cpp.dataflow.internal.DataFlowDispatch

cpp/ql/test/library-tests/dataflow/dataflow-tests/acrossLinkTargets.cpp

@@ -0,0 +1,42 @@
+int source();
+void sink(int);
+
+// For the purpose of this test, this function acts like it's defined in a
+// different link target such that we get two different functions named
+// `calleeAcrossLinkTargets` and no link from the caller to this one. The test
+// is getting that effect because the library can't distinguish the two
+// overloaded functions that differ only on `int` vs. `long`, so we might lose
+// this result and be forced to write a better test if the function signature
+// detection should improve.
+void calleeAcrossLinkTargets(long x) {
+  sink(x);
+}
+
+void calleeAcrossLinkTargets(int x); // no body
+
+
+void callerAcrossLinkTargets() {
+  calleeAcrossLinkTargets(source());
+}
+
+///////////////////////////////////////////////////////////////////////////////
+
+
+// No flow into this function as its signature is not unique (in the limited
+// model of the library).
+void ambiguousCallee(long x) {
+  sink(x);
+}
+
+// No flow into this function as its signature is not unique (in the limited
+// model of the library).
+void ambiguousCallee(short x) {
+  sink(x);
+}
+
+void ambiguousCallee(int x); // no body
+
+
+void ambiguousCaller() {
+  ambiguousCallee(source());
+}

cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected

@@ -1,3 +1,4 @@
+| acrossLinkTargets.cpp:12:8:12:8 | x | acrossLinkTargets.cpp:19:27:19:32 | call to source |
 | test.cpp:7:8:7:9 | t1 | test.cpp:6:12:6:17 | call to source |
 | test.cpp:9:8:9:9 | t1 | test.cpp:6:12:6:17 | call to source |
 | test.cpp:10:8:10:9 | t2 | test.cpp:6:12:6:17 | call to source |

cpp/ql/test/library-tests/dataflow/dataflow-tests/test_ir.expected

@@ -1,3 +1,5 @@
+| acrossLinkTargets.cpp:12:8:12:8 | Convert: (int)... | acrossLinkTargets.cpp:19:27:19:32 | Call: call to source |
+| acrossLinkTargets.cpp:12:8:12:8 | Load: x | acrossLinkTargets.cpp:19:27:19:32 | Call: call to source |
 | test.cpp:7:8:7:9 | Load: t1 | test.cpp:6:12:6:17 | Call: call to source |
 | test.cpp:9:8:9:9 | Load: t1 | test.cpp:6:12:6:17 | Call: call to source |
 | test.cpp:10:8:10:9 | Load: t2 | test.cpp:6:12:6:17 | Call: call to source |