From 084fa80a5720ad2bf1c56c9e33208347fa8e64cd Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Fri, 20 Mar 2020 09:16:56 +0000 Subject: [PATCH 1/5] Refine virtual call targets by local reasoning where possible. --- .../go/dataflow/internal/DataFlowDispatch.qll | 67 ++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowDispatch.qll b/ql/src/semmle/go/dataflow/internal/DataFlowDispatch.qll index 0300d8bb76f..8e55a70e2b7 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowDispatch.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowDispatch.qll @@ -1,11 +1,76 @@ private import go private import DataFlowPrivate +/** + * Holds if `call` is an interface call to method `m`, meaning that its receiver `recv` has an + * interface type. + */ +private predicate isInterfaceCallReceiver(DataFlow::CallNode call, DataFlow::Node recv, string m) { + call.getReceiver() = recv and + recv.getType().getUnderlyingType() instanceof InterfaceType and + m = call.getCalleeName() +} + +/** Holds if `nd` may flow into the receiver value of `call`, which is an interface value. */ +private DataFlow::Node getInterfaceCallReceiverSource(DataFlow::CallNode call) { + isInterfaceCallReceiver(call, result.getASuccessor*(), _) +} + +/** Gets the type of `nd`, which must be a valid type and not an interface type. */ +private Type getConcreteType(DataFlow::Node nd) { + result = nd.getType() and + not result.getUnderlyingType() instanceof InterfaceType and + not result instanceof InvalidType +} + +/** + * Holds if all concrete (that is, non-interface) types of `nd` concrete types can be determined by + * local reasoning. + * + * `nd` is restricted to nodes that flow into the receiver value of an interface call, since that is + * all we are ultimately interested in. + */ +private predicate isConcreteValue(DataFlow::Node nd) { + nd = getInterfaceCallReceiverSource(_) and + ( + exists(getConcreteType(nd)) + or + forex(DataFlow::Node pred | pred = nd.getAPredecessor() | isConcreteValue(pred)) + ) +} + +/** + * Holds if `call` is an interface call to method `m` with receiver `recv`, where the concrete + * types of `recv` can be established by local reasoning. + */ +private predicate isConcreteInterfaceCall(DataFlow::Node call, DataFlow::Node recv, string m) { + isInterfaceCallReceiver(call, recv, m) and isConcreteValue(recv) +} + +/** + * Gets a function that might be called by `call`, where the receiver of `call` has interface type, + * but its concrete types can be determined by local reasoning. + */ +private FuncDecl getConcreteTarget(DataFlow::CallNode call) { + exists(DataFlow::Node recv, string m | + isConcreteInterfaceCall(call, recv, m) | + exists(Type concreteReceiverType, DeclaredFunction concreteTarget | + concreteReceiverType = getConcreteType(getInterfaceCallReceiverSource(call)) and + concreteTarget = concreteReceiverType.getMethod(m) and + result = concreteTarget.getFuncDecl() + ) + ) +} + /** * Gets a function that might be called by `call`. */ DataFlowCallable viableCallable(CallExpr ma) { - result = DataFlow::exprNode(ma).(DataFlow::CallNode).getACallee() + exists(DataFlow::CallNode call | call.asExpr() = ma | + if isConcreteInterfaceCall(call, _, _) + then result = getConcreteTarget(call) + else result = call.getACallee() + ) } /** From 752ee3909a52c7699a2cc2a26aabf70184bc486e Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Fri, 20 Mar 2020 09:34:30 +0000 Subject: [PATCH 2/5] Add queries to inspect and measure dispatch differences. --- .../dataflow/internal/CompareDispatch.qhelp | 1 + .../go/dataflow/internal/CompareDispatch.ql | 20 +++++++++++++++++++ .../go/dataflow/internal/DispatchStats.ql | 18 +++++++++++++++++ 3 files changed, 39 insertions(+) create mode 100644 ql/src/semmle/go/dataflow/internal/CompareDispatch.qhelp create mode 100644 ql/src/semmle/go/dataflow/internal/CompareDispatch.ql create mode 100644 ql/src/semmle/go/dataflow/internal/DispatchStats.ql diff --git a/ql/src/semmle/go/dataflow/internal/CompareDispatch.qhelp b/ql/src/semmle/go/dataflow/internal/CompareDispatch.qhelp new file mode 100644 index 00000000000..2fd9f957002 --- /dev/null +++ b/ql/src/semmle/go/dataflow/internal/CompareDispatch.qhelp @@ -0,0 +1 @@ +TBD \ No newline at end of file diff --git a/ql/src/semmle/go/dataflow/internal/CompareDispatch.ql b/ql/src/semmle/go/dataflow/internal/CompareDispatch.ql new file mode 100644 index 00000000000..8cccbce8d4e --- /dev/null +++ b/ql/src/semmle/go/dataflow/internal/CompareDispatch.ql @@ -0,0 +1,20 @@ +/** + * @kind problem + * @problem.severity warning + * @precision very-high + * @id go/compare-dispatch + */ + +import go +import DataFlowDispatch + +FuncDef viableCallableOld(CallExpr c) { + exists(DataFlow::CallNode call | call.asExpr() = c | result = call.getACallee()) +} + +from CallExpr c, FuncDef fn, string msg +where + fn = viableCallableOld(c) and not fn = viableCallable(c) and msg = "Missing" + or + not fn = viableCallableOld(c) and fn = viableCallable(c) and msg = "New" +select c, msg + " $@.", fn, "callee" diff --git a/ql/src/semmle/go/dataflow/internal/DispatchStats.ql b/ql/src/semmle/go/dataflow/internal/DispatchStats.ql new file mode 100644 index 00000000000..74eabe44bb5 --- /dev/null +++ b/ql/src/semmle/go/dataflow/internal/DispatchStats.ql @@ -0,0 +1,18 @@ +/** + * @kind table + * @id go/dispatch-stats + */ + +import go +import DataFlowDispatch + +FuncDef viableCallableOld(CallExpr c) { + exists(DataFlow::CallNode call | call.asExpr() = c | result = call.getACallee()) +} + +from CallExpr c, int old, int new +where + old = count(viableCallableOld(c)) and + new = count(viableCallable(c)) and + old != new +select c, old + " -> " + new From 6edbe74c09839762bdfdc5257b20d955f89c0947 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 25 Mar 2020 10:43:05 +0000 Subject: [PATCH 3/5] Revert "Add queries to inspect and measure dispatch differences." This reverts commit 752ee3909a52c7699a2cc2a26aabf70184bc486e. --- .../dataflow/internal/CompareDispatch.qhelp | 1 - .../go/dataflow/internal/CompareDispatch.ql | 20 ------------------- .../go/dataflow/internal/DispatchStats.ql | 18 ----------------- 3 files changed, 39 deletions(-) delete mode 100644 ql/src/semmle/go/dataflow/internal/CompareDispatch.qhelp delete mode 100644 ql/src/semmle/go/dataflow/internal/CompareDispatch.ql delete mode 100644 ql/src/semmle/go/dataflow/internal/DispatchStats.ql diff --git a/ql/src/semmle/go/dataflow/internal/CompareDispatch.qhelp b/ql/src/semmle/go/dataflow/internal/CompareDispatch.qhelp deleted file mode 100644 index 2fd9f957002..00000000000 --- a/ql/src/semmle/go/dataflow/internal/CompareDispatch.qhelp +++ /dev/null @@ -1 +0,0 @@ -TBD \ No newline at end of file diff --git a/ql/src/semmle/go/dataflow/internal/CompareDispatch.ql b/ql/src/semmle/go/dataflow/internal/CompareDispatch.ql deleted file mode 100644 index 8cccbce8d4e..00000000000 --- a/ql/src/semmle/go/dataflow/internal/CompareDispatch.ql +++ /dev/null @@ -1,20 +0,0 @@ -/** - * @kind problem - * @problem.severity warning - * @precision very-high - * @id go/compare-dispatch - */ - -import go -import DataFlowDispatch - -FuncDef viableCallableOld(CallExpr c) { - exists(DataFlow::CallNode call | call.asExpr() = c | result = call.getACallee()) -} - -from CallExpr c, FuncDef fn, string msg -where - fn = viableCallableOld(c) and not fn = viableCallable(c) and msg = "Missing" - or - not fn = viableCallableOld(c) and fn = viableCallable(c) and msg = "New" -select c, msg + " $@.", fn, "callee" diff --git a/ql/src/semmle/go/dataflow/internal/DispatchStats.ql b/ql/src/semmle/go/dataflow/internal/DispatchStats.ql deleted file mode 100644 index 74eabe44bb5..00000000000 --- a/ql/src/semmle/go/dataflow/internal/DispatchStats.ql +++ /dev/null @@ -1,18 +0,0 @@ -/** - * @kind table - * @id go/dispatch-stats - */ - -import go -import DataFlowDispatch - -FuncDef viableCallableOld(CallExpr c) { - exists(DataFlow::CallNode call | call.asExpr() = c | result = call.getACallee()) -} - -from CallExpr c, int old, int new -where - old = count(viableCallableOld(c)) and - new = count(viableCallable(c)) and - old != new -select c, old + " -> " + new From e6bdc1809bed23dad3cadc8860d0e5f4943cbdf6 Mon Sep 17 00:00:00 2001 From: Max Schaefer <54907921+max-schaefer@users.noreply.github.com> Date: Wed, 25 Mar 2020 15:04:48 +0000 Subject: [PATCH 4/5] Update ql/src/semmle/go/dataflow/internal/DataFlowDispatch.qll Co-Authored-By: Sauyon Lee --- ql/src/semmle/go/dataflow/internal/DataFlowDispatch.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowDispatch.qll b/ql/src/semmle/go/dataflow/internal/DataFlowDispatch.qll index 8e55a70e2b7..6d1f644ee98 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowDispatch.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowDispatch.qll @@ -11,7 +11,7 @@ private predicate isInterfaceCallReceiver(DataFlow::CallNode call, DataFlow::Nod m = call.getCalleeName() } -/** Holds if `nd` may flow into the receiver value of `call`, which is an interface value. */ +/** Gets a data-flow node that may flow into the receiver value of `call`, which is an interface value. */ private DataFlow::Node getInterfaceCallReceiverSource(DataFlow::CallNode call) { isInterfaceCallReceiver(call, result.getASuccessor*(), _) } From 46a1a4e010b7f90b0e53456f02c4f6d0554fffb4 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 25 Mar 2020 20:34:34 +0000 Subject: [PATCH 5/5] Add a test. --- .../go/dataflow/CallGraph/getACallee.expected | 3 +++ .../semmle/go/dataflow/CallGraph/main.go | 20 +++++++++++++- .../dataflow/CallGraph/viableCallee.expected | 2 ++ .../go/dataflow/CallGraph/viableCallee.ql | 26 +++++++++++++++++++ 4 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 ql/test/library-tests/semmle/go/dataflow/CallGraph/viableCallee.expected create mode 100644 ql/test/library-tests/semmle/go/dataflow/CallGraph/viableCallee.ql diff --git a/ql/test/library-tests/semmle/go/dataflow/CallGraph/getACallee.expected b/ql/test/library-tests/semmle/go/dataflow/CallGraph/getACallee.expected index 381d576a20b..d097ea413bf 100644 --- a/ql/test/library-tests/semmle/go/dataflow/CallGraph/getACallee.expected +++ b/ql/test/library-tests/semmle/go/dataflow/CallGraph/getACallee.expected @@ -1,2 +1,5 @@ missingCallee spuriousCallee +| main.go:44:3:44:7 | call to m | main.go:17:1:17:17 | function declaration | +| main.go:44:3:44:7 | call to m | main.go:21:1:21:20 | function declaration | +| main.go:56:2:56:6 | call to m | main.go:21:1:21:20 | function declaration | diff --git a/ql/test/library-tests/semmle/go/dataflow/CallGraph/main.go b/ql/test/library-tests/semmle/go/dataflow/CallGraph/main.go index 1a10877b80a..3047f79e9d6 100644 --- a/ql/test/library-tests/semmle/go/dataflow/CallGraph/main.go +++ b/ql/test/library-tests/semmle/go/dataflow/CallGraph/main.go @@ -35,5 +35,23 @@ func test(x *s1, y s2, z s3, b mybool, i I) { } func test2(v interface{}) { - v.(interface{ m(int) }).m(0) // callee: s3.m + v.(interface{ m(int) }).m(0) // callee: s3.m +} + +func test3(v I) { + if v == nil { + v = s1{} + v.m() // callee: s1.m + } + v.m() // callee: s1.m callee: s2.m callee: mybool.m +} + +func test4(b bool) { + var v I + if b { + v = s1{} + } else { + v = &s2{} + } + v.m() // callee: s1.m callee: s2.m } diff --git a/ql/test/library-tests/semmle/go/dataflow/CallGraph/viableCallee.expected b/ql/test/library-tests/semmle/go/dataflow/CallGraph/viableCallee.expected new file mode 100644 index 00000000000..381d576a20b --- /dev/null +++ b/ql/test/library-tests/semmle/go/dataflow/CallGraph/viableCallee.expected @@ -0,0 +1,2 @@ +missingCallee +spuriousCallee diff --git a/ql/test/library-tests/semmle/go/dataflow/CallGraph/viableCallee.ql b/ql/test/library-tests/semmle/go/dataflow/CallGraph/viableCallee.ql new file mode 100644 index 00000000000..132cbf36e4e --- /dev/null +++ b/ql/test/library-tests/semmle/go/dataflow/CallGraph/viableCallee.ql @@ -0,0 +1,26 @@ +import go +import semmle.go.dataflow.internal.DataFlowDispatch + +/** + * Gets a string `val` such that there is a comment on the same line as `l` + * that contains the substring `key: val`. + */ +string metadata(Locatable l, string key) { + exists(string f, int line, Comment c, string kv | + l.hasLocationInfo(f, line, _, _, _) and + c.hasLocationInfo(f, line, _, _, _) and + kv = c.getText().regexpFind("\\b(\\w+: \\S+)", _, _) and + key = kv.regexpCapture("(\\w+): (\\S+)", 1) and + result = kv.regexpCapture("(\\w+): (\\S+)", 2) + ) +} + +query predicate missingCallee(DataFlow::CallNode call, FuncDef callee) { + metadata(call.asExpr(), "callee") = metadata(callee, "name") and + not viableCallable(call.asExpr()) = callee +} + +query predicate spuriousCallee(DataFlow::CallNode call, FuncDef callee) { + viableCallable(call.asExpr()) = callee and + not metadata(call.asExpr(), "callee") = metadata(callee, "name") +}