Prepare dataflow for local annotations

python/ql/consistency-queries/DataFlowConsistency.ql

@@ -26,6 +26,8 @@ private module Input implements InputSig<Location, PythonDataFlow> {
     or
     // TODO: Implement post-updates for **kwargs, see tests added in https://github.com/github/codeql/pull/14936
     exists(ArgumentPosition apos | n.argumentOf(_, apos) and apos.isDictSplat())
+    or
+    missingArgumentCallExclude(n)
   }
 
   predicate reverseReadExclude(Node n) {
@@ -134,6 +136,18 @@ private module Input implements InputSig<Location, PythonDataFlow> {
       other.getNode().getScope() = f
     )
   }
+
+  predicate missingArgumentCallExclude(ArgumentNode arg) {
+    // We overapproximate the argument nodes in order to not rely on the global `getCallArg`
+    // predicate.
+    // Because of this, we must exclude the cases where we have an approximation but no actual
+    // argument node.
+    arg = getCallArgApproximation() and not getCallArg(_, _, _, arg, _)
+    or
+    // Likewise, capturing closure arguments do not have corresponding argument nodes in some cases.
+    arg instanceof SynthCapturedVariablesArgumentNode and
+    not arg.argumentOf(_, _)
+  }
 }
 
 import MakeConsistency<Location, PythonDataFlow, PythonTaintTracking, Input>

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowDispatch.qll

@@ -1714,36 +1714,66 @@ private class SummaryPostUpdateNode extends FlowSummaryNode, PostUpdateNodeImpl
  * This is also known as the environment part of a closure.
  *
  * This is used for tracking flow through captured variables.
- *
- * TODO:
- * We might want a synthetic node here, but currently that incurs problems
- * with non-monotonic recursion, because of the use of `resolveCall` in the
- * char pred. This may be solvable by using
- * `CallGraphConstruction::Make` in stead of
- * `CallGraphConstruction::Simple::Make` appropriately.
  */
-class CapturedVariablesArgumentNode extends CfgNode, ArgumentNode {
-  CallNode callNode;
+class SynthCapturedVariablesArgumentNode extends Node, TSynthCapturedVariablesArgumentNode {
+  ControlFlowNode callable;
 
-  CapturedVariablesArgumentNode() {
-    node = callNode.getFunction() and
-    exists(Function target | resolveCall(callNode, target, _) |
-      target = any(VariableCapture::CapturedVariable v).getACapturingScope()
-    )
-  }
+  SynthCapturedVariablesArgumentNode() { this = TSynthCapturedVariablesArgumentNode(callable) }
+
+  /** Gets the `CallNode` corresponding to this captured variables argument node. */
+  CallNode getCallNode() { result.getFunction() = callable }
+
+  /** Gets the `CfgNode` that corresponds to this synthetic node. */
+  CfgNode getUnderlyingNode() { result.asCfgNode() = callable }
+
+  override Scope getScope() { result = callable.getScope() }
+
+  override Location getLocation() { result = callable.getLocation() }
 
   override string toString() { result = "Capturing closure argument" }
+}
 
+/** A captured variables argument node viewed as an argument node. Needed because `argumentOf` is a global predicate. */
+class CapturedVariablesArgumentNodeAsArgumentNode extends ArgumentNode,
+  SynthCapturedVariablesArgumentNode
+{
   override predicate argumentOf(DataFlowCall call, ArgumentPosition pos) {
-    callNode = call.getNode() and
-    pos.isLambdaSelf()
+    exists(CallNode callNode | callNode = this.getCallNode() |
+      callNode = call.getNode() and
+      exists(Function target | resolveCall(callNode, target, _) |
+        target = any(VariableCapture::CapturedVariable v).getACapturingScope()
+      ) and
+      pos.isLambdaSelf()
+    )
   }
 }
 
-/** A synthetic node representing the values of variables captured by a comprehension. */
-class SynthCompCapturedVariablesArgumentNode extends Node, TSynthCompCapturedVariablesArgumentNode,
-  ArgumentNode
+/** A synthetic node representing the values of captured variables after the output has been computed. */
+class SynthCapturedVariablesArgumentPostUpdateNode extends PostUpdateNodeImpl,
+  TSynthCapturedVariablesArgumentPostUpdateNode
 {
+  ControlFlowNode callable;
+
+  SynthCapturedVariablesArgumentPostUpdateNode() {
+    this = TSynthCapturedVariablesArgumentPostUpdateNode(callable)
+  }
+
+  /** Gets the `PostUpdateNode` (for a `CfgNode`) that corresponds to this synthetic node. */
+  PostUpdateNode getUnderlyingNode() { result.getPreUpdateNode().asCfgNode() = callable }
+
+  override string toString() { result = "[post] Capturing closure argument" }
+
+  override Scope getScope() { result = callable.getScope() }
+
+  override Location getLocation() { result = callable.getLocation() }
+
+  override SynthCapturedVariablesArgumentNode getPreUpdateNode() {
+    result = TSynthCapturedVariablesArgumentNode(callable)
+  }
+}
+
+/** A synthetic node representing the values of variables captured by a comprehension. */
+class SynthCompCapturedVariablesArgumentNode extends Node, TSynthCompCapturedVariablesArgumentNode {
   Comp comp;
 
   SynthCompCapturedVariablesArgumentNode() { this = TSynthCompCapturedVariablesArgumentNode(comp) }
@@ -1755,7 +1785,11 @@ class SynthCompCapturedVariablesArgumentNode extends Node, TSynthCompCapturedVar
   override Location getLocation() { result = comp.getLocation() }
 
   Comp getComprehension() { result = comp }
+}
 
+class SynthCompCapturedVariablesArgumentNodeAsArgumentNode extends SynthCompCapturedVariablesArgumentNode,
+  ArgumentNode
+{
   override predicate argumentOf(DataFlowCall call, ArgumentPosition pos) {
     call.(ComprehensionCall).getComprehension() = comp and
     pos.isLambdaSelf()

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll

@@ -1128,6 +1128,14 @@ predicate nodeIsHidden(Node n) {
   n instanceof SynthCaptureNode
   or
   n instanceof SynthCapturedVariablesParameterNode
+  or
+  n instanceof SynthCapturedVariablesArgumentNode
+  or
+  n instanceof SynthCapturedVariablesArgumentPostUpdateNode
+  or
+  n instanceof SynthCompCapturedVariablesArgumentNode
+  or
+  n instanceof SynthCompCapturedVariablesArgumentPostUpdateNode
 }
 
 class LambdaCallKind = Unit;

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll

@@ -76,15 +76,7 @@ newtype TNode =
     node.getNode() = any(Comp c).getIterable()
   } or
   /** A node representing a global (module-level) variable in a specific module. */
-  TModuleVariableNode(Module m, GlobalVariable v) {
-    v.getScope() = m and
-    (
-      v.escapes()
-      or
-      isAccessedThroughImportStar(m) and
-      ImportStar::globalNameDefinedInModule(v.getId(), m)
-    )
-  } or
+  TModuleVariableNode(Module m, GlobalVariable v) { v.getScope() = m } or
   /**
    * A synthetic node representing that an iterable sequence flows to consumer.
    */
@@ -129,6 +121,20 @@ newtype TNode =
     f = any(VariableCapture::CapturedVariable v).getACapturingScope() and
     exists(TFunction(f))
   } or
+  /**
+   * A synthetic node representing the values of the variables captured
+   * by the callable being called.
+   */
+  TSynthCapturedVariablesArgumentNode(ControlFlowNode callable) {
+    callable = any(CallNode c).getFunction()
+  } or
+  /**
+   * A synthetic node representing the values of the variables captured
+   * by the callable being called, after the output has been computed.
+   */
+  TSynthCapturedVariablesArgumentPostUpdateNode(ControlFlowNode callable) {
+    callable = any(CallNode c).getFunction()
+  } or
   /** A synthetic node representing the values of variables captured by a comprehension. */
   TSynthCompCapturedVariablesArgumentNode(Comp comp) {
     comp.getFunction() = any(VariableCapture::CapturedVariable v).getACapturingScope()
@@ -347,27 +353,51 @@ abstract class ArgumentNode extends Node {
   final ExtractedDataFlowCall getCall() { this.argumentOf(result, _) }
 }
 
+/** Gets an overapproximation of the argument nodes that are included in `getCallArg`. */
+Node getCallArgApproximation() {
+  // pre-update nodes for calls
+  result = any(CallCfgNode c).(PostUpdateNode).getPreUpdateNode()
+  or
+  // self parameters in methods
+  exists(Class c | result.asExpr() = c.getAMethod().getArg(0))
+  or
+  // the object part of an attribute expression (which might be a bound method)
+  result.asCfgNode() = any(AttrNode a).getObject()
+  or
+  // the function part of any call
+  result.asCfgNode() = any(CallNode c).getFunction()
+}
+
+/** Gets the extracted argument nodes that do not rely on `getCallArg`. */
+private Node otherArgs() {
+  // for potential summaries we allow all normal call arguments
+  normalCallArg(_, result, _)
+  or
+  // and self arguments
+  result.asCfgNode() = any(CallNode c).getFunction().(AttrNode).getObject()
+  or
+  // for comprehensions, we allow the synthetic `iterable` argument
+  result.asExpr() = any(Comp c).getIterable()
+}
+
 /**
  * A data flow node that represents a call argument found in the source code.
  */
 class ExtractedArgumentNode extends ArgumentNode {
   ExtractedArgumentNode() {
-    // for resolved calls, we need to allow all argument nodes
-    getCallArg(_, _, _, this, _)
-    or
-    // for potential summaries we allow all normal call arguments
-    normalCallArg(_, this, _)
+    this = getCallArgApproximation()
     or
-    // and self arguments
-    this.asCfgNode() = any(CallNode c).getFunction().(AttrNode).getObject()
-    or
-    // for comprehensions, we allow the synthetic `iterable` argument
-    this.asExpr() = any(Comp c).getIterable()
+    this = otherArgs()
   }
 
   final override predicate argumentOf(DataFlowCall call, ArgumentPosition pos) {
     this = call.getArgument(pos) and
-    call instanceof ExtractedDataFlowCall
+    call instanceof ExtractedDataFlowCall and
+    (
+      this = otherArgs()
+      or
+      this = getCallArgApproximation() and getCallArg(_, _, _, this, _)
+    )
   }
 }
 
@@ -440,13 +470,17 @@ class ModuleVariableNode extends Node, TModuleVariableNode {
 
   /** Gets a node that reads this variable. */
   Node getARead() {
-    result.asCfgNode() = var.getALoad().getAFlowNode() and
-    // Ignore reads that happen when the module is imported. These are only executed once.
-    not result.getScope() = mod
+    result = this.getALocalRead()
     or
     this = import_star_read(result)
   }
 
+  /** Gets a node that reads this variable, excluding reads that happen through `from ... import *`. */
+  Node getALocalRead() {
+    result.asCfgNode() = var.getALoad().getAFlowNode() and
+    not result.getScope() = mod
+  }
+
   /** Gets an `EssaNode` that corresponds to an assignment of this global variable. */
   Node getAWrite() {
     any(EssaNodeDefinition def).definedBy(var, result.asCfgNode().(DefinitionNode))
@@ -466,8 +500,6 @@ class ModuleVariableNode extends Node, TModuleVariableNode {
   override Location getLocation() { result = mod.getLocation() }
 }
 
-private predicate isAccessedThroughImportStar(Module m) { m = ImportStar::getStarImported(_) }
-
 private ModuleVariableNode import_star_read(Node n) {
   resolved_import_star_module(result.getModule(), result.getVariable().getId(), n)
 }

python/ql/lib/semmle/python/dataflow/new/internal/LocalSources.qll

@@ -67,7 +67,7 @@ class LocalSourceNode extends Node {
     or
     // We explicitly include any read of a global variable, as some of these may have local flow going
     // into them.
-    this = any(ModuleVariableNode mvn).getARead()
+    this = any(ModuleVariableNode v).getALocalRead()
     or
     // We include all scope entry definitions, as these act as the local source within the scope they
     // enter.
@@ -248,7 +248,7 @@ private module Cached {
   pragma[nomagic]
   private predicate localSourceFlowStep(Node nodeFrom, Node nodeTo) {
     simpleLocalFlowStep(nodeFrom, nodeTo, _) and
-    not nodeTo = any(ModuleVariableNode v).getARead()
+    not nodeTo = any(ModuleVariableNode v).getALocalRead()
   }
 
   /**

python/ql/lib/semmle/python/dataflow/new/internal/VariableCapture.qll

@@ -114,6 +114,12 @@ private Flow::ClosureNode asClosureNode(Node n) {
     result.(Flow::ExprNode).getExpr().getNode() = comp
   )
   or
+  // For captured variable argument nodes (and their post-update variants), we use the closure node
+  // for the underlying node.
+  result = asClosureNode(n.(SynthCapturedVariablesArgumentNode).getUnderlyingNode())
+  or
+  result = asClosureNode(n.(SynthCapturedVariablesArgumentPostUpdateNode).getUnderlyingNode())
+  or
   // TODO: Should the `Comp`s above be excluded here?
   result.(Flow::ExprNode).getExpr() = n.(CfgNode).getNode()
   or

python/ql/lib/utils/test/dataflow/MaximalFlowTest.qll

@@ -8,7 +8,9 @@ module MaximalFlowTest implements FlowTestSig {
 
   predicate relevantFlow(DataFlow::Node source, DataFlow::Node sink) {
     source != sink and
-    MaximalFlows::flow(source, sink)
+    MaximalFlows::flow(source, sink) and
+    // exclude ModuleVariableNodes (which have location 0:0:0:0)
+    not sink instanceof DataFlow::ModuleVariableNode
   }
 }
 
@@ -33,7 +35,7 @@ module MaximalFlowsConfig implements DataFlow::ConfigSig {
   predicate isSink(DataFlow::Node node) {
     exists(node.getLocation().getFile().getRelativePath()) and
     not any(CallNode c).getArg(_) = node.asCfgNode() and
-    not node instanceof DataFlow::ArgumentNode and
+    not isArgumentNode(node, _, _) and
     not node.asCfgNode().(NameNode).getId().matches("SINK%") and
     not DataFlow::localFlowStep(node, _)
   }

python/ql/lib/utils/test/dataflow/callGraphConfig.qll

@@ -9,7 +9,7 @@ module CallGraphConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node node) {
     node instanceof DataFlowPrivate::ReturnNode
     or
-    node instanceof DataFlow::ArgumentNode
+    DataFlowPrivate::isArgumentNode(node, _, _)
   }
 
   predicate isSink(DataFlow::Node node) {

python/ql/src/Classes/InitCallsSubclass/InitCallsSubclassMethod.ql

@@ -14,7 +14,8 @@
 
 import python
 import semmle.python.dataflow.new.DataFlow
 import semmle.python.dataflow.new.internal.DataFlowDispatch
+import semmle.python.dataflow.new.internal.DataFlowPrivate
 
 predicate initSelfCallOverridden(
   Function init, DataFlow::Node self, DataFlow::MethodCallNode call, Function target,
@@ -39,7 +40,7 @@
     self.getParameter() = method.getArg(0) and
     DataFlow::localFlow(self, sink)
   |
-    sink instanceof DataFlow::ArgumentNode
+    isArgumentNode(sink, _, _)
     or
     sink = any(DataFlow::AttrRead a).getObject()
   )

python/ql/test/experimental/attrs/AttrWrites.expected

@@ -1,4 +1,5 @@
 | test.py:5:9:5:16 | ControlFlowNode for __init__ | test.py:4:1:4:20 | ControlFlowNode for ClassExpr | __init__ | test.py:5:5:5:28 | ControlFlowNode for FunctionExpr |
 | test.py:6:9:6:16 | ControlFlowNode for Attribute | test.py:6:9:6:12 | ControlFlowNode for self | foo | test.py:6:20:6:22 | ControlFlowNode for foo |
+| test.py:9:1:9:9 | ControlFlowNode for Attribute | test.py:0:0:0:0 | ModuleVariableNode in Module test for myobj | foo | test.py:9:13:9:17 | ControlFlowNode for StringLiteral |
 | test.py:9:1:9:9 | ControlFlowNode for Attribute | test.py:9:1:9:5 | ControlFlowNode for myobj | foo | test.py:9:13:9:17 | ControlFlowNode for StringLiteral |
 | test.py:12:1:12:25 | ControlFlowNode for setattr() | test.py:12:9:12:13 | ControlFlowNode for myobj | foo | test.py:12:23:12:24 | ControlFlowNode for IntegerLiteral |