From d2b8d836e92a370e170d901ebc1b91e315718740 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 20 Jul 2023 16:48:36 +0100 Subject: [PATCH 1/4] Avoid using getTarget() as it may not exist Try to also deal with the case that we are calling a function through a variable that it has been assigned to. --- .../go/dataflow/internal/DataFlowDispatch.qll | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/go/ql/lib/semmle/go/dataflow/internal/DataFlowDispatch.qll b/go/ql/lib/semmle/go/dataflow/internal/DataFlowDispatch.qll index ea768f547153..111978c9653e 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/DataFlowDispatch.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/DataFlowDispatch.qll @@ -134,10 +134,6 @@ class ArgumentPosition extends int { pragma[inline] predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { ppos = apos } -private predicate isInterfaceMethod(Method c) { - c.getReceiverBaseType().getUnderlyingType() instanceof InterfaceType -} - /** * Holds if `call` is passing `arg` to param `p` in any circumstance except passing * a receiver parameter to a concrete method. @@ -149,9 +145,8 @@ predicate golangSpecificParamArgFilter( // Interface methods calls may be passed strictly to that exact method's model receiver: arg.getPosition() != -1 or - exists(Function callTarget | callTarget = call.getNode().(DataFlow::CallNode).getTarget() | - not isInterfaceMethod(callTarget) - or - callTarget = p.getCallable().asSummarizedCallable().asFunction() - ) + p instanceof DataFlow::SummarizedParameterNode + or + not call.getNode().(DataFlow::CallNode).getReceiver().getType().getUnderlyingType() instanceof + InterfaceType } From 00d5cb737c232b64fdcb6a6b573c9ebb458df741 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Fri, 28 Jul 2023 17:00:06 +0100 Subject: [PATCH 2/4] Different approach to avoiding getTarget() --- .../go/dataflow/internal/DataFlowDispatch.qll | 19 ++++++++++++++++-- .../go/dataflow/internal/DataFlowNodes.qll | 20 ++++++++++++++----- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/go/ql/lib/semmle/go/dataflow/internal/DataFlowDispatch.qll b/go/ql/lib/semmle/go/dataflow/internal/DataFlowDispatch.qll index 111978c9653e..45dfa1fd6301 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/DataFlowDispatch.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/DataFlowDispatch.qll @@ -134,6 +134,10 @@ class ArgumentPosition extends int { pragma[inline] predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { ppos = apos } +private predicate isInterfaceMethod(Method c) { + c.getReceiverBaseType().getUnderlyingType() instanceof InterfaceType +} + /** * Holds if `call` is passing `arg` to param `p` in any circumstance except passing * a receiver parameter to a concrete method. @@ -147,6 +151,17 @@ predicate golangSpecificParamArgFilter( or p instanceof DataFlow::SummarizedParameterNode or - not call.getNode().(DataFlow::CallNode).getReceiver().getType().getUnderlyingType() instanceof - InterfaceType + not isInterfaceMethod(call.getNode() + .(DataFlow::CallNode) + .getACalleeWithoutVirtualDispatch() + .asFunction()) +} + +predicate foo(DataFlowCall call, DataFlow::Node rec0) { + exists(DataFlow::Node rec | rec = call.getNode().(DataFlow::CallNode).getReceiver() | + rec = rec0 and rec.getType().getUnderlyingType() instanceof InterfaceType + ) and + not exists(Function callTarget | callTarget = call.getNode().(DataFlow::CallNode).getTarget() | + not isInterfaceMethod(callTarget) + ) } diff --git a/go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll b/go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll index 59224024ec34..ef7c97a9009b 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll @@ -489,13 +489,9 @@ module Public { * interface type. */ Callable getACalleeIncludingExternals() { - result.asFunction() = this.getTarget() + result = this.getACalleeWithoutVirtualDispatch() or exists(DataFlow::Node calleeSource | calleeSource = this.getACalleeSource() | - result.asFuncLit() = calleeSource.asExpr() - or - calleeSource = result.asFunction().getARead() - or exists(Method declared, Method actual | calleeSource = declared.getARead() and actual.implements(declared) and @@ -510,6 +506,20 @@ module Public { */ FuncDef getACallee() { result = this.getACalleeIncludingExternals().getFuncDef() } + /** + * As `getACalleeIncludingExternals`, except excluding external functions (those for which + * we lack a definition, such as standard library functions). + */ + Callable getACalleeWithoutVirtualDispatch() { + result.asFunction() = this.getTarget() + or + exists(DataFlow::Node calleeSource | calleeSource = this.getACalleeSource() | + result.asFuncLit() = calleeSource.asExpr() + or + calleeSource = result.asFunction().getARead() + ) + } + /** * Gets the name of the function, method or variable that is being called. * From 0895853a230c09dd894c8513f485555bf608837b Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Fri, 28 Jul 2023 17:09:37 +0100 Subject: [PATCH 3/4] Delete unused testing predicate --- .../lib/semmle/go/dataflow/internal/DataFlowDispatch.qll | 9 --------- 1 file changed, 9 deletions(-) diff --git a/go/ql/lib/semmle/go/dataflow/internal/DataFlowDispatch.qll b/go/ql/lib/semmle/go/dataflow/internal/DataFlowDispatch.qll index 45dfa1fd6301..581f049ab316 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/DataFlowDispatch.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/DataFlowDispatch.qll @@ -156,12 +156,3 @@ predicate golangSpecificParamArgFilter( .getACalleeWithoutVirtualDispatch() .asFunction()) } - -predicate foo(DataFlowCall call, DataFlow::Node rec0) { - exists(DataFlow::Node rec | rec = call.getNode().(DataFlow::CallNode).getReceiver() | - rec = rec0 and rec.getType().getUnderlyingType() instanceof InterfaceType - ) and - not exists(Function callTarget | callTarget = call.getNode().(DataFlow::CallNode).getTarget() | - not isInterfaceMethod(callTarget) - ) -} From dbc6868bc13093a4ad9328cce38a9d04ff3ed6f7 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com> Date: Tue, 1 Aug 2023 12:23:49 +0100 Subject: [PATCH 4/4] Update go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll Co-authored-by: Chris Smowton --- go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll b/go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll index ef7c97a9009b..5a51f16b83ad 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll @@ -507,8 +507,7 @@ module Public { FuncDef getACallee() { result = this.getACalleeIncludingExternals().getFuncDef() } /** - * As `getACalleeIncludingExternals`, except excluding external functions (those for which - * we lack a definition, such as standard library functions). + * Gets the definition of a possible target of this call, excluding targets reachable via virtual dispatch. */ Callable getACalleeWithoutVirtualDispatch() { result.asFunction() = this.getTarget()