Merge pull request #15166 from yoff/python/add-scope-entry-definition-nodes

yoff · web-flow · commit 19813c8ba6a8 · 2023-12-20T13:14:11.000+01:00
Python: Add scope entry definition nodes
diff --git a/python/ql/lib/change-notes/2023-12-20-add-scope-entry-definition-nodes.md b/python/ql/lib/change-notes/2023-12-20-add-scope-entry-definition-nodes.md
@@ -0,0 +1,5 @@
+---
+category: fix
+---
+
+- We would previously confuse all captured variables into a single scope entry node. Now they each get their own node so they can be tracked properly.
diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll b/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll
@@ -352,7 +352,10 @@ module LocalFlow {
     //   nodeFrom is `y` on first line
     //   nodeTo is `y` on second line
     exists(EssaDefinition def |
-      nodeFrom.(CfgNode).getNode() = def.(EssaNodeDefinition).getDefiningNode() and
+      nodeFrom.(CfgNode).getNode() = def.(EssaNodeDefinition).getDefiningNode()
+      or
+      nodeFrom.(ScopeEntryDefinitionNode).getDefinition() = def
+    |
       AdjacentUses::firstUse(def, nodeTo.(CfgNode).getNode())
     )
     or
@@ -481,8 +484,7 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
  * or at runtime when callables in the module are called.
  */
 predicate simpleLocalFlowStepForTypetracking(Node nodeFrom, Node nodeTo) {
-  IncludePostUpdateFlow<PhaseDependentFlow<LocalFlow::localFlowStep/2>::step/2>::step(nodeFrom,
-    nodeTo)
+  LocalFlow::localFlowStep(nodeFrom, nodeTo)
 }
 
 private predicate summaryLocalStep(Node nodeFrom, Node nodeTo) {
diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll b/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll
@@ -27,9 +27,12 @@ newtype TNode =
     isExpressionNode(node)
     or
     node.getNode() instanceof Pattern
-    or
-    node = any(ScopeEntryDefinition def | not def.getScope() instanceof Module).getDefiningNode()
   } or
+  /**
+   * A node corresponding to a scope entry definition. That is, the value of a variable
+   * as it enters a scope.
+   */
+  TScopeEntryDefinitionNode(ScopeEntryDefinition def) { not def.getScope() instanceof Module } or
   /**
    * A synthetic node representing the value of an object before a state change.
    *
@@ -257,6 +260,28 @@ class ExprNode extends CfgNode {
 /** Gets a node corresponding to expression `e`. */
 ExprNode exprNode(DataFlowExpr e) { result.getNode().getNode() = e }
 
+/**
+ * A node corresponding to a scope entry definition. That is, the value of a variable
+ * as it enters a scope.
+ */
+class ScopeEntryDefinitionNode extends Node, TScopeEntryDefinitionNode {
+  ScopeEntryDefinition def;
+
+  ScopeEntryDefinitionNode() { this = TScopeEntryDefinitionNode(def) }
+
+  /** Gets the `ScopeEntryDefinition` associated with this node. */
+  ScopeEntryDefinition getDefinition() { result = def }
+
+  /** Gets the source variable represented by this node. */
+  SsaSourceVariable getVariable() { result = def.getSourceVariable() }
+
+  override Location getLocation() { result = def.getLocation() }
+
+  override Scope getScope() { result = def.getScope() }
+
+  override string toString() { result = "Entry definition for " + this.getVariable().toString() }
+}
+
 /**
  * The value of a parameter at function entry, viewed as a node in a data
  * flow graph.
diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/LocalSources.qll b/python/ql/lib/semmle/python/dataflow/new/internal/LocalSources.qll
@@ -71,7 +71,7 @@ class LocalSourceNode extends Node {
     or
     // We include all scope entry definitions, as these act as the local source within the scope they
     // enter.
-    this.asCfgNode() = any(ScopeEntryDefinition def).getDefiningNode()
+    this instanceof ScopeEntryDefinitionNode
     or
     this instanceof ParameterNode
   }
@@ -167,7 +167,7 @@ class LocalSourceNodeNotModuleVariableNode extends LocalSourceNode {
   LocalSourceNodeNotModuleVariableNode() {
     this instanceof ExprNode
     or
-    this.asCfgNode() = any(ScopeEntryDefinition def).getDefiningNode()
+    this instanceof ScopeEntryDefinitionNode
   }
 }
 
diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/TypeTrackingImpl.qll b/python/ql/lib/semmle/python/dataflow/new/internal/TypeTrackingImpl.qll
@@ -251,7 +251,7 @@ module TypeTrackingInput implements Shared::TypeTrackingInput {
       e.getSourceVariable() = var and
       var.hasDefiningNode(def)
     |
-      nodeTo.asCfgNode() = e.getDefiningNode() and
+      nodeTo.(DataFlowPublic::ScopeEntryDefinitionNode).getDefinition() = e and
       nodeFrom.asCfgNode() = def.getValue() and
       var.getScope().getScope*() = nodeFrom.getScope()
     )
diff --git a/python/ql/test/experimental/dataflow/coverage/localFlow.expected b/python/ql/test/experimental/dataflow/coverage/localFlow.expected
@@ -1,12 +1,12 @@
-| test.py:41:1:41:33 | Entry node for Function test_tuple_with_local_flow | test.py:42:10:42:18 | ControlFlowNode for NONSOURCE |
-| test.py:41:1:41:33 | Entry node for Function test_tuple_with_local_flow | test.py:42:21:42:26 | ControlFlowNode for SOURCE |
-| test.py:41:1:41:33 | Entry node for Function test_tuple_with_local_flow | test.py:44:5:44:8 | ControlFlowNode for SINK |
+| test.py:41:1:41:33 | Entry definition for SsaSourceVariable NONSOURCE | test.py:42:10:42:18 | ControlFlowNode for NONSOURCE |
+| test.py:41:1:41:33 | Entry definition for SsaSourceVariable SINK | test.py:44:5:44:8 | ControlFlowNode for SINK |
+| test.py:41:1:41:33 | Entry definition for SsaSourceVariable SOURCE | test.py:42:21:42:26 | ControlFlowNode for SOURCE |
 | test.py:42:5:42:5 | ControlFlowNode for x | test.py:43:9:43:9 | ControlFlowNode for x |
 | test.py:42:10:42:26 | ControlFlowNode for Tuple | test.py:42:5:42:5 | ControlFlowNode for x |
 | test.py:43:5:43:5 | ControlFlowNode for y | test.py:44:10:44:10 | ControlFlowNode for y |
 | test.py:43:9:43:12 | ControlFlowNode for Subscript | test.py:43:5:43:5 | ControlFlowNode for y |
-| test.py:208:1:208:53 | Entry node for Function test_nested_comprehension_deep_with_local_flow | test.py:209:25:209:30 | ControlFlowNode for SOURCE |
-| test.py:208:1:208:53 | Entry node for Function test_nested_comprehension_deep_with_local_flow | test.py:210:5:210:8 | ControlFlowNode for SINK |
+| test.py:208:1:208:53 | Entry definition for SsaSourceVariable SINK | test.py:210:5:210:8 | ControlFlowNode for SINK |
+| test.py:208:1:208:53 | Entry definition for SsaSourceVariable SOURCE | test.py:209:25:209:30 | ControlFlowNode for SOURCE |
 | test.py:209:5:209:5 | ControlFlowNode for x | test.py:210:10:210:10 | ControlFlowNode for x |
 | test.py:209:9:209:68 | ControlFlowNode for .0 | test.py:209:9:209:68 | ControlFlowNode for .0 |
 | test.py:209:9:209:68 | ControlFlowNode for ListComp | test.py:209:5:209:5 | ControlFlowNode for x |
diff --git a/python/ql/test/experimental/dataflow/enclosing-callable/EnclosingCallable.expected b/python/ql/test/experimental/dataflow/enclosing-callable/EnclosingCallable.expected
@@ -18,7 +18,6 @@
 | generator.py:1:1:1:23 | Function generator_func | generator.py:2:12:2:26 | ControlFlowNode for .0 |
 | generator.py:1:1:1:23 | Function generator_func | generator.py:2:12:2:26 | ControlFlowNode for .0 |
 | generator.py:1:1:1:23 | Function generator_func | generator.py:2:12:2:26 | ControlFlowNode for ListComp |
-| generator.py:1:1:1:23 | Function generator_func | generator.py:2:12:2:26 | Entry node for Function listcomp |
 | generator.py:1:1:1:23 | Function generator_func | generator.py:2:13:2:13 | ControlFlowNode for Yield |
 | generator.py:1:1:1:23 | Function generator_func | generator.py:2:13:2:13 | ControlFlowNode for x |
 | generator.py:1:1:1:23 | Function generator_func | generator.py:2:19:2:19 | ControlFlowNode for x |
diff --git a/python/ql/test/experimental/dataflow/typetracking/moduleattr.expected b/python/ql/test/experimental/dataflow/typetracking/moduleattr.expected
@@ -6,6 +6,6 @@ module_attr_tracker
 | import_as_attr.py:1:28:1:35 | ControlFlowNode for attr_ref |
 | import_as_attr.py:3:1:3:1 | ControlFlowNode for x |
 | import_as_attr.py:3:5:3:12 | ControlFlowNode for attr_ref |
-| import_as_attr.py:5:1:5:10 | Entry node for Function fun |
+| import_as_attr.py:5:1:5:10 | Entry definition for SsaSourceVariable attr_ref |
 | import_as_attr.py:6:5:6:5 | ControlFlowNode for y |
 | import_as_attr.py:6:9:6:16 | ControlFlowNode for attr_ref |
diff --git a/python/ql/test/experimental/dataflow/typetracking/tracked.ql b/python/ql/test/experimental/dataflow/typetracking/tracked.ql
@@ -26,7 +26,7 @@ module TrackedTest implements TestSig {
       not e.getLocation().getStartLine() = 0 and
       // We do not wish to annotate scope entry definitions,
       // as they do not appear in the source code.
-      not e.asCfgNode() = any(ScopeEntryDefinition def).getDefiningNode() and
+      not e instanceof DataFlow::ScopeEntryDefinitionNode and
       tag = "tracked" and
       location = e.getLocation() and
       value = t.getAttr() and
diff --git a/python/ql/test/library-tests/ApiGraphs/py3/test_crosstalk.expected b/python/ql/test/library-tests/ApiGraphs/py3/test_crosstalk.expected
@@ -0,0 +1,2 @@
+| test_crosstalk.py:8:16:8:18 | ControlFlowNode for f() | bar |
+| test_crosstalk.py:13:16:13:18 | ControlFlowNode for g() | baz |
diff --git a/python/ql/test/library-tests/ApiGraphs/py3/test_crosstalk.py b/python/ql/test/library-tests/ApiGraphs/py3/test_crosstalk.py
@@ -0,0 +1,13 @@
+
+def outer():
+    from foo import bar, baz
+    
+    def inner_bar():
+        f = bar
+        g = baz
+        return f()
+    
+    def inner_baz():
+        f = bar
+        g = baz
+        return g()
diff --git a/python/ql/test/library-tests/ApiGraphs/py3/test_crosstalk.ql b/python/ql/test/library-tests/ApiGraphs/py3/test_crosstalk.ql
@@ -0,0 +1,8 @@
+import python
+import semmle.python.ApiGraphs
+
+from API::CallNode callNode, string member
+where
+  callNode = API::moduleImport("foo").getMember(member).getACall() and
+  callNode.getLocation().getFile().getBaseName() = "test_crosstalk.py"
+select callNode, member

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +category: fix
 +---
++
 +- We would previously confuse all captured variables into a single scope entry node. Now they each get their own node so they can be tracked properly.
Original file line number	Diff line number	Diff line change
`@@ -71,7 +71,7 @@ class LocalSourceNode extends Node {`
`71`	`71`	`or`
`72`	`72`	`// We include all scope entry definitions, as these act as the local source within the scope they`
`73`	`73`	`// enter.`
`74`		`- this.asCfgNode() = any(ScopeEntryDefinition def).getDefiningNode()`
	`74`	`+ this instanceof ScopeEntryDefinitionNode`
`75`	`75`	`or`
`76`	`76`	`this instanceof ParameterNode`
`77`	`77`	`}`
`@@ -167,7 +167,7 @@ class LocalSourceNodeNotModuleVariableNode extends LocalSourceNode {`
`167`	`167`	`LocalSourceNodeNotModuleVariableNode() {`
`168`	`168`	`this instanceof ExprNode`
`169`	`169`	`or`
`170`		`- this.asCfgNode() = any(ScopeEntryDefinition def).getDefiningNode()`
	`170`	`+ this instanceof ScopeEntryDefinitionNode`
`171`	`171`	`}`
`172`	`172`	`}`
`173`	`173`
Original file line number	Diff line number	Diff line change
`@@ -251,7 +251,7 @@ module TypeTrackingInput implements Shared::TypeTrackingInput {`
`251`	`251`	`e.getSourceVariable() = var and`
`252`	`252`	`var.hasDefiningNode(def)`
`253`	`253`	`\|`
`254`		`- nodeTo.asCfgNode() = e.getDefiningNode() and`
	`254`	`+ nodeTo.(DataFlowPublic::ScopeEntryDefinitionNode).getDefinition() = e and`
`255`	`255`	`nodeFrom.asCfgNode() = def.getValue() and`
`256`	`256`	`var.getScope().getScope*() = nodeFrom.getScope()`
`257`	`257`	`)`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+\| test_crosstalk.py:8:16:8:18 \| ControlFlowNode for f() \| bar \|`
	`2`	`+\| test_crosstalk.py:13:16:13:18 \| ControlFlowNode for g() \| baz \|`