Python: Fix false positive in py/non-iterator-in-for-loop

taus-semmle · taus-semmle · commit 9fda4ab4806c · 2019-11-21T11:49:29.000+01:00
Should fix #1833, #2137, and #2187. Internally, comprehensions are (at present) elaborated into local functions and iterators as described in [PEP-289](https://www.python.org/dev/peps/pep-0289/). That is, something like: ``` g = (x**2 for x in range(10)) ``` becomes something akin to ``` def __gen(exp): for x in exp: yield x**2 g = __gen(iter(range(10))) ``` In the context of the top-level of a class, this means `__gen` looks as if it is a method of the class, and in particular `exp` looks like it's the `self` argument of this method, which leads the points-to analysis to think that `exp` is an instance of the surrounding class itself. The fix in this case is pretty simple: we look for occurrences of `exp` (in fact called `.0` internally -- carefully chosen to _not_ be a valid Python identifier) and explicitly exclude this parameter from being classified as a `self` parameter.
diff --git a/python/ql/src/semmle/python/objects/TObject.qll b/python/ql/src/semmle/python/objects/TObject.qll
@@ -367,9 +367,10 @@ predicate receiver(AttrNode instantiation, PointsToContext context, ObjectIntern
 pragma [noinline]
 private predicate self_parameter(ParameterDefinition def, PointsToContext context, PythonClassObjectInternal cls) {
     def.isSelf() and
+    /* Exclude the special parameter name `.0` which is used for unfolded comprehensions. */
+    def.getName() != ".0" and
     exists(Function scope |
         def.getScope() = scope and
-        def.isSelf() and
         context.isRuntime() and context.appliesToScope(scope) and
         scope.getScope() = cls.getScope() and
         concrete_class(cls) and
diff --git a/python/ql/test/query-tests/Statements/general/NonIteratorInForLoop.expected b/python/ql/test/query-tests/Statements/general/NonIteratorInForLoop.expected
@@ -1,2 +1 @@
 | test.py:50:1:50:23 | For | $@ of class '$@' may be used in for-loop. | test.py:50:10:50:22 | ControlFlowNode for NonIterator() | Non-iterator | test.py:45:1:45:26 | class NonIterator | NonIterator |
-| test.py:170:10:170:22 | For | $@ of class '$@' may be used in for-loop. | test.py:170:10:170:22 | ControlFlowNode for .0 | Non-iterator | test.py:169:1:169:21 | class false_positive | false_positive |
diff --git a/python/ql/test/query-tests/Statements/general/test.py b/python/ql/test/query-tests/Statements/general/test.py
@@ -165,7 +165,7 @@ def no_with():
 def assert_ok(seq):
     assert all(isinstance(element, (str, unicode)) for element in seq)
 
-# False positive. ODASA-8042
+# False positive. ODASA-8042. Fixed in PR #2401.
 class false_positive:
     e = (x for x in [])
 

Original file line number	Diff line number	Diff line change
`@@ -1,2 +1 @@`
`1`	`1`	`\| test.py:50:1:50:23 \| For \| $@ of class '$@' may be used in for-loop. \| test.py:50:10:50:22 \| ControlFlowNode for NonIterator() \| Non-iterator \| test.py:45:1:45:26 \| class NonIterator \| NonIterator \|`
`2`		`-\| test.py:170:10:170:22 \| For \| $@ of class '$@' may be used in for-loop. \| test.py:170:10:170:22 \| ControlFlowNode for .0 \| Non-iterator \| test.py:169:1:169:21 \| class false_positive \| false_positive \|`