Go: improve path summary by changing post update nodes

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()

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 = getAnUpdatedInstruction() }
+
+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 getAccessPathPredecessor(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 = getAccessPathPredecessor*(getADirectlyWrittenInstruction())
+}
+
+private IR::Instruction getAnUpdatedInstruction() {
+  result = IR::evalExprInstruction(getAnUpdatedExpr()) or
+  result instanceof IR::EvalImplicitDerefInstruction or
+  result = getAWrittenInstruction()
+}
+
+private Expr getAnUpdatedExpr() {
+  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 = getAnUpdatedInstruction() }
+  }
+
   /**
    * 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)
+    }
   }
 
   /**

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