From 14f1ecb456201776fa1de164cbf0737804fd7c6a Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Thu, 10 Jan 2019 15:39:40 +0100 Subject: [PATCH 1/3] C++: Data flow dispatch across link targets --- .../dataflow/internal/DataFlowDispatch.qll | 50 ++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowDispatch.qll b/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowDispatch.qll index 8beb2c5d319a..4ca8aadec4b2 100644 --- a/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowDispatch.qll +++ b/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() } /** From 10ce13d1e9631eb3f712fa1fd6cda45875a04ea7 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Tue, 5 Mar 2019 15:56:53 +0100 Subject: [PATCH 2/3] C++: Tests for cross-target dispatch --- .../dataflow-tests/acrossLinkTargets.cpp | 42 +++++++++++++++++++ .../dataflow/dataflow-tests/test.expected | 1 + .../dataflow-tests/test_diff.expected | 1 + 3 files changed, 44 insertions(+) create mode 100644 cpp/ql/test/library-tests/dataflow/dataflow-tests/acrossLinkTargets.cpp diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/acrossLinkTargets.cpp b/cpp/ql/test/library-tests/dataflow/dataflow-tests/acrossLinkTargets.cpp new file mode 100644 index 000000000000..40b0f63f689b --- /dev/null +++ b/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()); +} diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected index 38cbbc1bc4bc..33c9b946cf36 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected +++ b/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 | diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected index 1eccb0fedf19..2bd9822a9d47 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected @@ -1,3 +1,4 @@ +| acrossLinkTargets.cpp:19:27:19:32 | acrossLinkTargets.cpp:12:8:12:8 | AST only | | test.cpp:66:30:66:36 | test.cpp:71:8:71:9 | AST only | | test.cpp:89:28:89:34 | test.cpp:92:8:92:14 | IR only | | test.cpp:100:13:100:18 | test.cpp:103:10:103:12 | AST only | From 80b076561897fa48650ffbc52c16769a5b1e5776 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Wed, 6 Mar 2019 10:02:42 +0100 Subject: [PATCH 3/3] C++: Make IR DataFlow dispatch use non-IR version This removes code duplication and ensures that the IR version also gets the support for flow across link targets. --- .../ir/dataflow/internal/DataFlowDispatch.qll | 74 +------------------ .../dataflow-tests/test_diff.expected | 1 - .../dataflow/dataflow-tests/test_ir.expected | 2 + 3 files changed, 3 insertions(+), 74 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowDispatch.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowDispatch.qll index cb2339cfdc26..f123dbbef70a 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowDispatch.qll +++ b/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 diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected index 2bd9822a9d47..1eccb0fedf19 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected @@ -1,4 +1,3 @@ -| acrossLinkTargets.cpp:19:27:19:32 | acrossLinkTargets.cpp:12:8:12:8 | AST only | | test.cpp:66:30:66:36 | test.cpp:71:8:71:9 | AST only | | test.cpp:89:28:89:34 | test.cpp:92:8:92:14 | IR only | | test.cpp:100:13:100:18 | test.cpp:103:10:103:12 | AST only | diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_ir.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_ir.expected index 5e3d4970bd7a..f1705da88380 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_ir.expected +++ b/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 |