Merge pull request #14644 from yoff/python/filter-local-self-loops

Python: filter local self loops

Author: yoff

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

@@ -279,14 +279,16 @@ class NonSyntheticPostUpdateNode extends PostUpdateNodeImpl, CfgNode {
 class DataFlowExpr = Expr;
 
 /**
- * Flow between ESSA variables.
- * This includes both local and global variables.
- * Flow comes from definitions, uses and refinements.
+ * A module to compute local flow.
+ *
+ * Flow will generally go from control flow nodes into essa variables at definitions,
+ * and from there via use-use flow to other control flow nodes.
+ *
+ * Some syntaxtic constructs are handled separately.
  */
-// TODO: Consider constraining `nodeFrom` and `nodeTo` to be in the same scope.
-// If they have different enclosing callables, we get consistency errors.
-module EssaFlow {
-  predicate essaFlowStep(Node nodeFrom, Node nodeTo) {
+module LocalFlow {
+  /** Holds if `nodeFrom` is the control flow node defining the essa variable `nodeTo`. */
+  predicate definitionFlowStep(Node nodeFrom, Node nodeTo) {
     // Definition
     //   `x = f(42)`
     //   nodeFrom is `f(42)`, cfg node
@@ -336,24 +338,12 @@ module EssaFlow {
     //   nodeFrom is `x`, cfgNode
     //   nodeTo is `x`, essa var
     exists(ParameterDefinition pd |
-      nodeFrom.asCfgNode() = pd.getDefiningNode() and
-      nodeTo.asVar() = pd.getVariable()
+      nodeFrom.(CfgNode).getNode() = pd.getDefiningNode() and
+      nodeTo.(EssaNode).getVar() = pd.getVariable()
     )
-    or
-    // First use after definition
-    //   `y = 42`
-    //   `x = f(y)`
-    //   nodeFrom is `y` on first line, essa var
-    //   nodeTo is `y` on second line, cfg node
-    defToFirstUse(nodeFrom.asVar(), nodeTo.asCfgNode())
-    or
-    // Next use after use
-    //   `x = f(y)`
-    //   `z = y + 1`
-    //   nodeFrom is 'y' on first line, cfg node
-    //   nodeTo is `y` on second line, cfg node
-    useToNextUse(nodeFrom.asCfgNode(), nodeTo.asCfgNode())
-    or
+  }
+
+  predicate expressionFlowStep(Node nodeFrom, Node nodeTo) {
     // If expressions
     nodeFrom.asCfgNode() = nodeTo.asCfgNode().(IfExprNode).getAnOperand()
     or
@@ -366,6 +356,7 @@ module EssaFlow {
     // Flow inside an unpacking assignment
     iterableUnpackingFlowStep(nodeFrom, nodeTo)
     or
+    // Flow inside a match statement
     matchFlowStep(nodeFrom, nodeTo)
   }
 
@@ -376,6 +367,35 @@ module EssaFlow {
   predicate defToFirstUse(EssaVariable var, NameNode nodeTo) {
     AdjacentUses::firstUse(var.getDefinition(), nodeTo)
   }
+
+  predicate useUseFlowStep(Node nodeFrom, Node nodeTo) {
+    // First use after definition
+    //   `y = 42`
+    //   `x = f(y)`
+    //   nodeFrom is `y` on first line, essa var
+    //   nodeTo is `y` on second line, cfg node
+    defToFirstUse(nodeFrom.asVar(), nodeTo.asCfgNode())
+    or
+    // Next use after use
+    //   `x = f(y)`
+    //   `z = y + 1`
+    //   nodeFrom is 'y' on first line, cfg node
+    //   nodeTo is `y` on second line, cfg node
+    useToNextUse(nodeFrom.asCfgNode(), nodeTo.asCfgNode())
+  }
+
+  predicate localFlowStep(Node nodeFrom, Node nodeTo) {
+    IncludePostUpdateFlow<PhaseDependentFlow<definitionFlowStep/2>::step/2>::step(nodeFrom, nodeTo)
+    or
+    IncludePostUpdateFlow<PhaseDependentFlow<expressionFlowStep/2>::step/2>::step(nodeFrom, nodeTo)
+    or
+    // Blindly applying use-use flow can result in a node that steps to itself, for
+    // example in while-loops. To uphold dataflow consistency checks, we don't want
+    // that. However, we do want to allow `[post] n` to `n` (to handle while loops), so
+    // we should only do the filtering after `IncludePostUpdateFlow` has ben applied.
+    IncludePostUpdateFlow<PhaseDependentFlow<useUseFlowStep/2>::step/2>::step(nodeFrom, nodeTo) and
+    nodeFrom != nodeTo
+  }
 }
 
 //--------
@@ -481,7 +501,8 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
  * or at runtime when callables in the module are called.
  */
 predicate simpleLocalFlowStepForTypetracking(Node nodeFrom, Node nodeTo) {
-  IncludePostUpdateFlow<PhaseDependentFlow<EssaFlow::essaFlowStep/2>::step/2>::step(nodeFrom, nodeTo)
+  IncludePostUpdateFlow<PhaseDependentFlow<LocalFlow::localFlowStep/2>::step/2>::step(nodeFrom,
+    nodeTo)
 }
 
 private predicate summaryLocalStep(Node nodeFrom, Node nodeTo) {

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

@@ -111,13 +111,13 @@ module ImportResolution {
       allowedEssaImportStep*(firstDef, lastUseVar) and
       not allowedEssaImportStep(_, firstDef)
     |
-      not EssaFlow::defToFirstUse(firstDef, _) and
+      not LocalFlow::defToFirstUse(firstDef, _) and
       val.asVar() = firstDef
       or
       exists(ControlFlowNode mid, ControlFlowNode end |
-        EssaFlow::defToFirstUse(firstDef, mid) and
-        EssaFlow::useToNextUse*(mid, end) and
-        not EssaFlow::useToNextUse(end, _) and
+        LocalFlow::defToFirstUse(firstDef, mid) and
+        LocalFlow::useToNextUse*(mid, end) and
+        not LocalFlow::useToNextUse(end, _) and
         val.asCfgNode() = end
       )
     )

python/ql/test/experimental/dataflow/coverage/dataflow-consistency.expected

@@ -25,18 +25,5 @@ uniqueParameterNodeAtPosition
 uniqueParameterNodePosition
 uniqueContentApprox
 identityLocalStep
-| datamodel.py:84:15:84:15 | ControlFlowNode for x | Node steps to itself |
-| datamodel.py:166:11:166:11 | ControlFlowNode for x | Node steps to itself |
-| test.py:103:10:103:15 | ControlFlowNode for SOURCE | Node steps to itself |
-| test.py:130:10:130:15 | ControlFlowNode for SOURCE | Node steps to itself |
-| test.py:162:13:162:18 | ControlFlowNode for SOURCE | Node steps to itself |
-| test.py:167:13:167:18 | ControlFlowNode for SOURCE | Node steps to itself |
-| test.py:216:10:216:15 | ControlFlowNode for SOURCE | Node steps to itself |
-| test.py:242:9:242:12 | ControlFlowNode for SINK | Node steps to itself |
-| test.py:673:9:673:12 | ControlFlowNode for SINK | Node steps to itself |
-| test.py:674:9:674:14 | ControlFlowNode for SINK_F | Node steps to itself |
-| test.py:682:9:682:12 | ControlFlowNode for SINK | Node steps to itself |
-| test.py:690:9:690:12 | ControlFlowNode for SINK | Node steps to itself |
-| test.py:696:5:696:8 | ControlFlowNode for SINK | Node steps to itself |
 missingArgumentCall
 multipleArgumentCall

python/ql/test/experimental/dataflow/coverage/loops.py

@@ -0,0 +1,73 @@
+# All functions starting with "test_" should run and execute `print("OK")` exactly once.
+# This can be checked by running validTest.py.
+
+import sys
+import os
+
+sys.path.append(os.path.dirname(os.path.dirname((__file__))))
+from testlib import expects
+
+# These are defined so that we can evaluate the test code.
+NONSOURCE = "not a source"
+SOURCE = "source"
+
+
+def is_source(x):
+    return x == "source" or x == b"source" or x == 42 or x == 42.0 or x == 42j
+
+
+def SINK(x):
+    if is_source(x):
+        print("OK")
+    else:
+        print("Unexpected flow", x)
+
+
+def SINK_F(x):
+    if is_source(x):
+        print("Unexpected flow", x)
+    else:
+        print("OK")
+
+# ------------------------------------------------------------------------------
+# Actual tests
+# ------------------------------------------------------------------------------
+
+def test_while():
+    x = NONSOURCE
+    n = 2
+    while n > 0:
+        if n == 1:
+            SINK(x) #$ flow="SOURCE, l:+1 -> x"
+        x = SOURCE
+        n -= 1
+
+class MyObj(object):
+    def __init__(self, foo):
+        self.foo = foo
+
+def setFoo(obj, x):
+    obj.foo = x
+
+def test_while_obj():
+    myobj = MyObj(NONSOURCE)
+    n = 2
+    while n > 0:
+        if n == 1:
+            SINK(myobj.foo) #$ flow="SOURCE, l:+1 -> myobj.foo"
+        setFoo(myobj, SOURCE)
+        n -= 1
+
+def setAndTestFoo(obj, x, test):
+    if test:
+        # This flow is not found, if self-loops are broken at the SSA level.
+        SINK(obj.foo) #$ flow="SOURCE, l:+7 -> obj.foo"
+    obj.foo = x
+
+def test_while_obj_sink():
+    myobj = MyObj(NONSOURCE)
+    n = 2
+    while n > 0:
+        setAndTestFoo(myobj, SOURCE, n == 1)
+        n -= 1
+

python/ql/test/experimental/dataflow/module-initialization/localFlow.ql

@@ -12,7 +12,7 @@ module ImportTimeLocalFlowTest implements FlowTestSig {
     // results are displayed next to `nodeTo`, so we need a line to write on
     nodeTo.getLocation().getStartLine() > 0 and
     nodeTo.asVar() instanceof GlobalSsaVariable and
-    DP::PhaseDependentFlow<DP::EssaFlow::essaFlowStep/2>::importTimeStep(nodeFrom, nodeTo)
+    DP::PhaseDependentFlow<DP::LocalFlow::localFlowStep/2>::importTimeStep(nodeFrom, nodeTo)
   }
 }
 

python/ql/test/experimental/dataflow/strange-essaflow/testFlow.ql

@@ -33,5 +33,5 @@ query predicate jumpStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
 
 query predicate essaFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
   os_import(nodeFrom) and
-  DataFlowPrivate::EssaFlow::essaFlowStep(nodeFrom, nodeTo)
+  DataFlowPrivate::LocalFlow::localFlowStep(nodeFrom, nodeTo)
 }

python/ql/test/experimental/dataflow/tainttracking/defaultAdditionalTaintStep-py3/dataflow-consistency.expected

@@ -23,7 +23,5 @@ uniqueParameterNodeAtPosition
 uniqueParameterNodePosition
 uniqueContentApprox
 identityLocalStep
-| test_collections.py:20:9:20:22 | ControlFlowNode for ensure_tainted | Node steps to itself |
-| test_unpacking.py:31:9:31:22 | ControlFlowNode for ensure_tainted | Node steps to itself |
 missingArgumentCall
 multipleArgumentCall

python/ql/test/experimental/dataflow/tainttracking/defaultAdditionalTaintStep/dataflow-consistency.expected

@@ -23,20 +23,5 @@ uniqueParameterNodeAtPosition
 uniqueParameterNodePosition
 uniqueContentApprox
 identityLocalStep
-| test_async.py:48:9:48:22 | ControlFlowNode for ensure_tainted | Node steps to itself |
-| test_collections.py:64:10:64:21 | ControlFlowNode for tainted_list | Node steps to itself |
-| test_collections.py:71:9:71:22 | ControlFlowNode for ensure_tainted | Node steps to itself |
-| test_collections.py:73:9:73:22 | ControlFlowNode for ensure_tainted | Node steps to itself |
-| test_collections.py:88:10:88:21 | ControlFlowNode for tainted_list | Node steps to itself |
-| test_collections.py:89:10:89:23 | ControlFlowNode for TAINTED_STRING | Node steps to itself |
-| test_collections.py:97:9:97:22 | ControlFlowNode for ensure_tainted | Node steps to itself |
-| test_collections.py:99:9:99:22 | ControlFlowNode for ensure_tainted | Node steps to itself |
-| test_collections.py:112:9:112:22 | ControlFlowNode for ensure_tainted | Node steps to itself |
-| test_collections.py:114:9:114:22 | ControlFlowNode for ensure_tainted | Node steps to itself |
-| test_collections.py:147:9:147:22 | ControlFlowNode for ensure_tainted | Node steps to itself |
-| test_collections.py:149:9:149:22 | ControlFlowNode for ensure_tainted | Node steps to itself |
-| test_collections.py:246:9:246:15 | ControlFlowNode for my_dict | Node steps to itself |
-| test_collections.py:246:22:246:33 | ControlFlowNode for tainted_dict | Node steps to itself |
-| test_for.py:24:9:24:22 | ControlFlowNode for ensure_tainted | Node steps to itself |
 missingArgumentCall
 multipleArgumentCall

python/ql/test/experimental/dataflow/validTest.py

@@ -65,6 +65,7 @@ def check_tests_valid_after_version(testFile, version):
     check_tests_valid("coverage.argumentPassing")
     check_tests_valid("coverage.datamodel")
     check_tests_valid("coverage.test_builtins")
+    check_tests_valid("coverage.loops")
     check_tests_valid("coverage-py2.classes")
     check_tests_valid("coverage-py3.classes")
     check_tests_valid("variable-capture.in")

python/ql/test/experimental/dataflow/variable-capture/dataflow-consistency.expected

@@ -29,8 +29,5 @@ uniqueParameterNodeAtPosition
 uniqueParameterNodePosition
 uniqueContentApprox
 identityLocalStep
-| test_collections.py:36:10:36:15 | ControlFlowNode for SOURCE | Node steps to itself |
-| test_collections.py:45:19:45:21 | ControlFlowNode for mod | Node steps to itself |
-| test_collections.py:52:13:52:21 | ControlFlowNode for mod_local | Node steps to itself |
 missingArgumentCall
 multipleArgumentCall