From b1e254ea5030801f6c286b50f3904925b80ea2a8 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 15 Jun 2023 15:51:30 +0100 Subject: [PATCH 1/2] Change post-update nodes --- .../go/controlflow/ControlFlowGraph.qll | 4 +- .../go/dataflow/internal/DataFlowNodes.qll | 120 ++++++++++++------ .../go/dataflow/internal/DataFlowPrivate.qll | 16 ++- 3 files changed, 96 insertions(+), 44 deletions(-) diff --git a/go/ql/lib/semmle/go/controlflow/ControlFlowGraph.qll b/go/ql/lib/semmle/go/controlflow/ControlFlowGraph.qll index eeaed8ed2f75..3ac83040792b 100644 --- a/go/ql/lib/semmle/go/controlflow/ControlFlowGraph.qll +++ b/go/ql/lib/semmle/go/controlflow/ControlFlowGraph.qll @@ -135,7 +135,7 @@ module ControlFlow { exists(IR::FieldTarget trg | trg = super.getLhs() | ( trg.getBase() = base.asInstruction() or - trg.getBase() = MkImplicitDeref(base.asExpr()) + trg.getBase() = IR::implicitDerefInstruction(base.asExpr()) ) and trg.getField() = f and super.getRhs() = rhs.asInstruction() @@ -155,7 +155,7 @@ module ControlFlow { exists(IR::ElementTarget trg | trg = super.getLhs() | ( trg.getBase() = base.asInstruction() or - trg.getBase() = MkImplicitDeref(base.asExpr()) + trg.getBase() = IR::implicitDerefInstruction(base.asExpr()) ) and trg.getIndex() = index.asInstruction() and super.getRhs() = rhs.asInstruction() diff --git a/go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll b/go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll index 84ed6d5f61c7..e82480e83a7f 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll @@ -11,7 +11,54 @@ private newtype TNode = MkSsaNode(SsaDefinition ssa) or MkGlobalFunctionNode(Function f) or MkImplicitVarargsSlice(CallExpr c) { c.hasImplicitVarargs() } or - MkFlowSummaryNode(FlowSummaryImpl::Private::SummaryNode sn) + MkFlowSummaryNode(FlowSummaryImpl::Private::SummaryNode sn) or + MkPostUpdateNode(IR::Instruction insn) { insn = updatedInstruction() } + +private IR::Instruction getADirectlyWrittenInstruction() { + exists(IR::WriteTarget target, IR::Instruction base | + target = any(IR::WriteInstruction w).getLhs() and + (base = result or base = IR::implicitDerefInstruction(result.(IR::EvalInstruction).getExpr())) + | + target.(IR::FieldTarget).getBase() = base or + target.(IR::ElementTarget).getBase() = base + ) + or + result = IR::evalExprInstruction(any(SendStmt s).getChannel()) +} + +private IR::Instruction getAccessPathPredecessor2(IR::Instruction insn) { + exists(Expr e | result = IR::evalExprInstruction(e) | + e = insn.(IR::EvalInstruction).getExpr().(UnaryExpr).getOperand() + or + e = insn.(IR::EvalInstruction).getExpr().(StarExpr).getBase() + or + e = insn.(IR::EvalImplicitDerefInstruction).getOperand() + ) + or + result = insn.(IR::ComponentReadInstruction).getBase() +} + +private IR::Instruction getAWrittenInstruction() { + result = getAccessPathPredecessor2*(getADirectlyWrittenInstruction()) +} + +private IR::Instruction updatedInstruction() { + result = IR::evalExprInstruction(updatedExpr()) or + result instanceof IR::EvalImplicitDerefInstruction or + result = getAWrittenInstruction() +} + +private Expr updatedExpr() { + result instanceof AddressExpr + or + result = any(AddressExpr e).getOperand() + or + result instanceof StarExpr + or + result instanceof DerefExpr + or + result = any(CallExpr ce).getAnArgument() and mutableType(result.getType()) +} /** Nodes intended for only use inside the data-flow libraries. */ module Private { @@ -706,19 +753,6 @@ module Public { predicate isReceiverOf(MethodDecl m) { parm.isReceiverOf(m) } } - private Node getADirectlyWrittenNode() { - exists(Write w | w.writesComponent(result, _)) or - result = DataFlow::exprNode(any(SendStmt s).getChannel()) - } - - private DataFlow::Node getAccessPathPredecessor(DataFlow::Node node) { - result = node.(PointerDereferenceNode).getOperand() - or - result = node.(ComponentReadNode).getBase() - } - - private Node getAWrittenNode() { result = getAccessPathPredecessor*(getADirectlyWrittenNode()) } - /** * Holds if `tp` is a type that may (directly or indirectly) reference a memory location. * @@ -734,6 +768,10 @@ module Public { ) } + private class UpdateNode extends InstructionNode { + UpdateNode() { insn = updatedInstruction() } + } + /** * A node associated with an object after an operation that might have * changed its state. @@ -753,35 +791,37 @@ module Public { abstract Node getPreUpdateNode(); } - private class DefaultPostUpdateNode extends PostUpdateNode { - Node preupd; + /** A post-update node for an instruction that updates a value. */ + class DefaultPostUpdateNode extends PostUpdateNode { + UpdateNode preupd; - DefaultPostUpdateNode() { - ( - preupd instanceof AddressOperationNode - or - preupd = any(AddressOperationNode addr).getOperand() - or - preupd = any(PointerDereferenceNode deref).getOperand() - or - preupd = getAWrittenNode() - or - ( - preupd instanceof ArgumentNode and not preupd instanceof ImplicitVarargsSlice - or - preupd = any(CallNode c).getAnImplicitVarargsArgument() - ) and - mutableType(preupd.getType()) - ) and - ( - preupd = this.(SsaNode).getAUse() - or - preupd = this and - not basicLocalFlowStep(_, this) - ) - } + DefaultPostUpdateNode() { this = MkPostUpdateNode(preupd.asInstruction()) } override Node getPreUpdateNode() { result = preupd } + + /** Gets the node that data will flow to from this node. */ + Node getSuccessor() { + preupd = result.(SsaNode).getAUse() + or + preupd = result and + not basicLocalFlowStep0(_, preupd) + } + + override ControlFlow::Root getRoot() { result = preupd.asInstruction().getRoot() } + + override Type getType() { result = preupd.asInstruction().getResultType() } + + override string getNodeKind() { result = "post-update node" } + + override string toString() { + result = "post-update node for " + preupd.asInstruction().toString() + } + + override predicate hasLocationInfo( + string filepath, int startline, int startcolumn, int endline, int endcolumn + ) { + preupd.asInstruction().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) + } } /** diff --git a/go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll b/go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll index ee146fc2abad..54a7f0f2fe3d 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll @@ -41,9 +41,10 @@ OutNode getAnOutNode(DataFlowCall call, ReturnKind kind) { /** * Holds if data flows from `nodeFrom` to `nodeTo` in exactly one local - * (intra-procedural) step, not taking function models into account. + * (intra-procedural) step, not taking function models into account, and also + * excluding flow from post-update nodes to their successors. */ -predicate basicLocalFlowStep(Node nodeFrom, Node nodeTo) { +predicate basicLocalFlowStep0(Node nodeFrom, Node nodeTo) { // Instruction -> Instruction exists(Expr pred, Expr succ | succ.(LogicalBinaryExpr).getAnOperand() = pred or @@ -89,6 +90,17 @@ predicate basicLocalFlowStep(Node nodeFrom, Node nodeTo) { any(GlobalFunctionNode fn | fn.getFunction() = nodeTo.asExpr().(FunctionName).getTarget()) } +/** + * Holds if data flows from `nodeFrom` to `nodeTo` in exactly one local + * (intra-procedural) step, not taking function models into account. + */ +predicate basicLocalFlowStep(Node nodeFrom, Node nodeTo) { + basicLocalFlowStep0(nodeFrom, nodeTo) + or + // post-update node -> successor + nodeTo = nodeFrom.(DefaultPostUpdateNode).getSuccessor() +} + pragma[noinline] private Field getASparselyUsedChannelTypedField() { result.getType() instanceof ChanType and From 9e4f4fa2c700f9a87c2b764e73bf03650919c4dc Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Fri, 16 Jun 2023 11:05:20 +0100 Subject: [PATCH 2/2] Improve predicate names --- .../semmle/go/dataflow/internal/DataFlowNodes.qll | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll b/go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll index e82480e83a7f..8bb7aab72f67 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll @@ -12,7 +12,7 @@ private newtype TNode = MkGlobalFunctionNode(Function f) or MkImplicitVarargsSlice(CallExpr c) { c.hasImplicitVarargs() } or MkFlowSummaryNode(FlowSummaryImpl::Private::SummaryNode sn) or - MkPostUpdateNode(IR::Instruction insn) { insn = updatedInstruction() } + MkPostUpdateNode(IR::Instruction insn) { insn = getAnUpdatedInstruction() } private IR::Instruction getADirectlyWrittenInstruction() { exists(IR::WriteTarget target, IR::Instruction base | @@ -26,7 +26,7 @@ private IR::Instruction getADirectlyWrittenInstruction() { result = IR::evalExprInstruction(any(SendStmt s).getChannel()) } -private IR::Instruction getAccessPathPredecessor2(IR::Instruction insn) { +private IR::Instruction getAccessPathPredecessor(IR::Instruction insn) { exists(Expr e | result = IR::evalExprInstruction(e) | e = insn.(IR::EvalInstruction).getExpr().(UnaryExpr).getOperand() or @@ -39,16 +39,16 @@ private IR::Instruction getAccessPathPredecessor2(IR::Instruction insn) { } private IR::Instruction getAWrittenInstruction() { - result = getAccessPathPredecessor2*(getADirectlyWrittenInstruction()) + result = getAccessPathPredecessor*(getADirectlyWrittenInstruction()) } -private IR::Instruction updatedInstruction() { - result = IR::evalExprInstruction(updatedExpr()) or +private IR::Instruction getAnUpdatedInstruction() { + result = IR::evalExprInstruction(getAnUpdatedExpr()) or result instanceof IR::EvalImplicitDerefInstruction or result = getAWrittenInstruction() } -private Expr updatedExpr() { +private Expr getAnUpdatedExpr() { result instanceof AddressExpr or result = any(AddressExpr e).getOperand() @@ -769,7 +769,7 @@ module Public { } private class UpdateNode extends InstructionNode { - UpdateNode() { insn = updatedInstruction() } + UpdateNode() { insn = getAnUpdatedInstruction() } } /**