unified: Use proper CFG nodes for assignments in data flow

Author: asgerf

unified/ql/lib/codeql/unified/internal/dataflow/DataFlowNode.qll

@@ -2,9 +2,16 @@ private import unified
 private import AllDataFlow
 private import codeql.unified.internal.ExprPositions
 
-private predicate hasPostUpdate(Expr expr) {
+private predicate hasIncomingValueAtCfgNode(Expr expr, ControlFlowNode cfgNode) {
+  exists(AstNode declOrAssignment |
+    hasIncomingValue(expr, declOrAssignment) and
+    cfgNode.injects(declOrAssignment)
+  )
+}
+
+private predicate hasPostUpdate(Expr expr, ControlFlowNode cfgNode) {
   exists(MemberAccessExpr member |
-    (hasIncomingValue(member, _) or hasPostUpdate(member)) and
+    (hasIncomingValueAtCfgNode(member, cfgNode) or hasPostUpdate(member, cfgNode)) and
     expr = member.getBase()
   )
 }
@@ -16,28 +23,27 @@ predicate performsVariableAccess(
   Expr expr, LocalVariable var, VariableRefKind kind, ControlFlowNode cfgNode
 ) {
   exists(LocalVariableAccess access | var = access.getLocalVariable() and expr = access |
-    hasResultValue(access) and kind.isRead() and cfgNode.isAfter(expr)
+    hasResultValue(access) and kind.isRead() and cfgNode.asExpr() = expr
     or
-    hasIncomingValue(access, _) and kind.isWrite() and cfgNode.asExpr() = expr // TODO: use more precise CFG node
+    hasIncomingValueAtCfgNode(access, cfgNode) and kind.isWrite()
     or
-    hasPostUpdate(access) and kind.isPostUpdate() and cfgNode.asExpr() = expr // TODO: use more precise CFG node
+    hasPostUpdate(access, cfgNode) and kind.isPostUpdate()
   )
   or
   exists(UnqualifiedMemberAccess access |
     access.isInstanceAccess() and var = access.getImplicitQualifierVariable() and expr = access
   |
     kind.isRead() and cfgNode.isBefore(access)
     or
-    (hasIncomingValue(access, _) or hasPostUpdate(access)) and
-    kind.isPostUpdate() and
-    cfgNode.asExpr() = access // TODO: use more precise CFG node
+    (hasIncomingValueAtCfgNode(access, cfgNode) or hasPostUpdate(access, cfgNode)) and
+    kind.isPostUpdate()
   )
 }
 
 newtype TDataFlowNode =
   TValueNode(Expr expr) { hasResultValue(expr) or hasIncomingValue(expr, _) } or
   TStrictlyIncomingValue(Expr expr) { hasResultValue(expr) and hasIncomingValue(expr, _) } or
-  TExprPostUpdateNode(Expr expr) { hasPostUpdate(expr) } or
+  TExprPostUpdateNode(Expr expr) { hasPostUpdate(expr, _) } or
   TLocalVariableRefNode(Expr expr, LocalVariable var, VariableRefKind kind) {
     performsVariableAccess(expr, var, kind, _)
   } or

unified/ql/test/library-tests/dataflow/test.expected

@@ -95,7 +95,6 @@ edges
 | test.swift:120:12:120:12 | b | test.swift:126:10:126:10 | b | provenance |  |
 | test.swift:120:17:120:21 | tuple [0] | test.swift:120:9:120:13 | TupleExpr [0] | provenance |  |
 | test.swift:120:17:120:21 | tuple [1] | test.swift:120:9:120:13 | TupleExpr [1] | provenance |  |
-| test.swift:131:5:131:5 | a | test.swift:131:14:131:14 | a | provenance |  |
 | test.swift:131:5:131:5 | a | test.swift:132:10:132:10 | a | provenance |  |
 | test.swift:131:19:131:33 | CallExpr | test.swift:131:5:131:5 | a | provenance |  |
 nodes
@@ -224,7 +223,6 @@ nodes
 | test.swift:125:10:125:10 | a | semmle.label | a |
 | test.swift:126:10:126:10 | b | semmle.label | b |
 | test.swift:131:5:131:5 | a | semmle.label | a |
-| test.swift:131:14:131:14 | a | semmle.label | a |
 | test.swift:131:19:131:33 | CallExpr | semmle.label | CallExpr |
 | test.swift:132:10:132:10 | a | semmle.label | a |
 subpaths
@@ -261,5 +259,4 @@ testFailures
 | test.swift:112:10:112:16 | MemberAccessExpr | test.swift:107:19:107:33 | CallExpr | test.swift:112:10:112:16 | MemberAccessExpr | $@ | test.swift:107:19:107:33 | CallExpr | CallExpr |
 | test.swift:125:10:125:10 | a | test.swift:117:18:117:33 | CallExpr | test.swift:125:10:125:10 | a | $@ | test.swift:117:18:117:33 | CallExpr | CallExpr |
 | test.swift:126:10:126:10 | b | test.swift:117:35:117:49 | CallExpr | test.swift:126:10:126:10 | b | $@ | test.swift:117:35:117:49 | CallExpr | CallExpr |
-| test.swift:131:14:131:14 | a | test.swift:131:19:131:33 | CallExpr | test.swift:131:14:131:14 | a | $@ | test.swift:131:19:131:33 | CallExpr | CallExpr |
 | test.swift:132:10:132:10 | a | test.swift:131:19:131:33 | CallExpr | test.swift:132:10:132:10 | a | $@ | test.swift:131:19:131:33 | CallExpr | CallExpr |

unified/ql/test/library-tests/dataflow/test.swift

@@ -128,7 +128,7 @@ func t13() {
 
 func t14() {
     var a = "safe";
-    a = sink(a) + source("t14.1"); // $ SPURIOUS: hasTaintFlow=t14.1
+    a = sink(a) + source("t14.1");
     sink(a); // $ hasTaintFlow=t14.1
 }