Merge pull request #9999 from github/smowton/feature/golang-channel-flow

Go: implement conservative cross-thread dataflow

Author: smowton

go/ql/lib/change-notes/2022-08-12-cross-thread-flow.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* Fixed data-flow to captured variable references.
+* We now assume that if a channel-typed field is only referred to twice in the user codebase, once in a send operation and once in a receive, then data flows from the send to the receive statement. This enables finding some cross-goroutine flow.

go/ql/lib/semmle/go/controlflow/ControlFlowGraph.qll

@@ -166,11 +166,21 @@ module ControlFlow {
       )
     }
 
+    /**
+     * Holds if this node writes `rhs` to `channel`.
+     */
+    predicate writesToChannel(DataFlow::ExprNode channel, DataFlow::ExprNode rhs) {
+      exists(SendStmt send |
+        send.getChannel() = channel.asExpr() and
+        send.getValue() = rhs.asExpr()
+      )
+    }
+
     /**
      * Holds if this node sets any field or element of `base` to `rhs`.
      */
     predicate writesComponent(DataFlow::Node base, DataFlow::Node rhs) {
-      writesElement(base, _, rhs) or writesField(base, _, rhs)
+      writesElement(base, _, rhs) or writesField(base, _, rhs) or writesToChannel(base, rhs)
     }
   }
 

go/ql/lib/semmle/go/dataflow/internal/ContainerFlow.qll

@@ -24,9 +24,7 @@ predicate containerStoreStep(Node node1, Node node2, Content c) {
   )
   or
   c instanceof CollectionContent and
-  exists(SendStmt send |
-    send.getChannel() = node2.(ExprNode).asExpr() and send.getValue() = node1.(ExprNode).asExpr()
-  )
+  exists(Write w | w.writesToChannel(node2, node1))
   or
   c instanceof MapKeyContent and
   node2.getType() instanceof MapType and

go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll

@@ -634,9 +634,7 @@ module Public {
     predicate isReceiverOf(MethodDecl m) { parm.isReceiverOf(m) }
   }
 
-  private Node getADirectlyWrittenNode() {
-    exists(Write w | w.writesField(result, _, _) or w.writesElement(result, _, _))
-  }
+  private Node getADirectlyWrittenNode() { exists(Write w | w.writesComponent(result, _)) }
 
   private DataFlow::Node getAccessPathPredecessor(DataFlow::Node node) {
     result = node.(PointerDereferenceNode).getOperand()

go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll

@@ -70,10 +70,7 @@ predicate basicLocalFlowStep(Node nodeFrom, Node nodeTo) {
   )
   or
   // SSA -> SSA
-  exists(SsaDefinition pred, SsaDefinition succ |
-    succ.(SsaVariableCapture).getSourceVariable() = pred.(SsaExplicitDefinition).getSourceVariable() or
-    succ.(SsaPseudoDefinition).getAnInput() = pred
-  |
+  exists(SsaDefinition pred, SsaPseudoDefinition succ | succ.getAnInput() = pred |
     nodeFrom = ssaNode(pred) and
     nodeTo = ssaNode(succ)
   )
@@ -90,6 +87,12 @@ predicate basicLocalFlowStep(Node nodeFrom, Node nodeTo) {
     any(GlobalFunctionNode fn | fn.getFunction() = nodeTo.asExpr().(FunctionName).getTarget())
 }
 
+pragma[noinline]
+private Field getASparselyUsedChannelTypedField() {
+  result.getType() instanceof ChanType and
+  count(result.getARead()) = 2
+}
+
 /**
  * Holds if data can flow from `node1` to `node2` in a way that loses the
  * calling context. For example, this would happen with flow through a
@@ -102,6 +105,27 @@ predicate jumpStep(Node n1, Node n2) {
     w.writes(v, n1) and
     n2 = v.getARead()
   )
+  or
+  exists(SsaDefinition pred, SsaDefinition succ |
+    succ.(SsaVariableCapture).getSourceVariable() = pred.(SsaExplicitDefinition).getSourceVariable()
+  |
+    n1 = ssaNode(pred) and
+    n2 = ssaNode(succ)
+  )
+  or
+  // If a channel-typed field is referenced exactly once in the context of
+  // a send statement and once in a receive expression, assume the two are linked.
+  exists(
+    Field f, DataFlow::ReadNode recvRead, DataFlow::ReadNode sendRead, RecvExpr re, SendStmt ss
+  |
+    f = getASparselyUsedChannelTypedField() and
+    recvRead = f.getARead() and
+    sendRead = f.getARead() and
+    recvRead.asExpr() = re.getOperand() and
+    sendRead.asExpr() = ss.getChannel() and
+    n1.(DataFlow::PostUpdateNode).getPreUpdateNode() = sendRead and
+    n2 = recvRead
+  )
 }
 
 /**

go/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalFlowStep.expected

@@ -60,10 +60,8 @@
 | main.go:11:14:11:14 | z | main.go:11:9:11:15 | type conversion |
 | main.go:14:6:14:10 | function test2 | main.go:34:8:34:12 | test2 |
 | main.go:14:6:14:10 | function test2 | main.go:34:19:34:23 | test2 |
-| main.go:15:2:15:4 | definition of acc | main.go:16:9:16:9 | capture variable acc |
 | main.go:15:9:15:9 | 0 | main.go:15:2:15:4 | definition of acc |
 | main.go:16:9:16:9 | capture variable acc | main.go:17:3:17:5 | acc |
-| main.go:17:3:17:7 | definition of acc | main.go:16:9:16:9 | capture variable acc |
 | main.go:17:3:17:7 | definition of acc | main.go:18:10:18:12 | acc |
 | main.go:17:3:17:7 | rhs of increment statement | main.go:17:3:17:7 | definition of acc |
 | main.go:22:12:22:12 | argument corresponding to b | main.go:22:12:22:12 | definition of b |