diff --git a/python/ql/src/Statements/NonIteratorInForLoop.ql b/python/ql/src/Statements/NonIteratorInForLoop.ql index f8e6e51b55ff..2c04c8d97ee5 100644 --- a/python/ql/src/Statements/NonIteratorInForLoop.ql +++ b/python/ql/src/Statements/NonIteratorInForLoop.ql @@ -12,16 +12,48 @@ */ import python -private import LegacyPointsTo +private import semmle.python.dataflow.new.internal.DataFlowDispatch +private import semmle.python.ApiGraphs -from For loop, ControlFlowNodeWithPointsTo iter, Value v, ClassValue t, ControlFlowNode origin +/** + * Holds if `cls_arg` references a known iterable builtin type, either directly + * (e.g. `list`) or as an element of a tuple (e.g. `(list, tuple)`). + */ +private predicate isIterableTypeArg(DataFlow::Node cls_arg) { + cls_arg = + API::builtin([ + "list", "tuple", "set", "frozenset", "dict", "str", "bytes", "bytearray", "range", + "memoryview" + ]).getAValueReachableFromSource() + or + isIterableTypeArg(DataFlow::exprNode(cls_arg.asExpr().(Tuple).getAnElt())) +} + +/** + * Holds if `iter` is guarded by an `isinstance` check that tests for + * an iterable type (e.g. `list`, `tuple`, `set`, `dict`). + */ +predicate guardedByIsinstanceIterable(DataFlow::Node iter) { + exists( + DataFlow::GuardNode guard, DataFlow::CallCfgNode isinstance_call, DataFlow::LocalSourceNode src + | + isinstance_call = API::builtin("isinstance").getACall() and + src.flowsTo(isinstance_call.getArg(0)) and + src.flowsTo(iter) and + isIterableTypeArg(isinstance_call.getArg(1)) and + guard = isinstance_call.asCfgNode() and + guard.controlsBlock(iter.asCfgNode().getBasicBlock(), true) + ) +} + +from For loop, DataFlow::Node iter, Class cls where - loop.getIter().getAFlowNode() = iter and - iter.pointsTo(_, v, origin) and - v.getClass() = t and - not t.isIterable() and - not t.failedInference(_) and - not v = Value::named("None") and - not t.isDescriptorType() -select loop, "This for-loop may attempt to iterate over a $@ of class $@.", origin, - "non-iterable instance", t, t.getName() + iter.asExpr() = loop.getIter() and + iter = classInstanceTracker(cls) and + not DuckTyping::isIterable(cls) and + not DuckTyping::isDescriptor(cls) and + not (loop.isAsync() and DuckTyping::hasMethod(cls, "__aiter__")) and + not DuckTyping::hasUnresolvedBase(getADirectSuperclass*(cls)) and + not guardedByIsinstanceIterable(iter) +select loop, "This for-loop may attempt to iterate over a $@ of class $@.", iter.asExpr(), + "non-iterable instance", cls, cls.getName() diff --git a/python/ql/test/3/query-tests/Statements/iter/NonIteratorInForLoop.expected b/python/ql/test/3/query-tests/Statements/iter/NonIteratorInForLoop.expected index c59db3b2b657..396382d0b9f9 100644 --- a/python/ql/test/3/query-tests/Statements/iter/NonIteratorInForLoop.expected +++ b/python/ql/test/3/query-tests/Statements/iter/NonIteratorInForLoop.expected @@ -1,2 +1 @@ -| async_iterator.py:26:11:26:34 | For | This for-loop may attempt to iterate over a $@ of class $@. | async_iterator.py:26:20:26:33 | ControlFlowNode for MissingAiter() | non-iterable instance | async_iterator.py:13:1:13:19 | class MissingAiter | MissingAiter | -| statements_test.py:34:5:34:19 | For | This for-loop may attempt to iterate over a $@ of class $@. | statements_test.py:34:18:34:18 | ControlFlowNode for IntegerLiteral | non-iterable instance | file://:0:0:0:0 | builtin-class int | int | +| async_iterator.py:26:11:26:34 | For | This for-loop may attempt to iterate over a $@ of class $@. | async_iterator.py:26:20:26:33 | MissingAiter() | non-iterable instance | async_iterator.py:13:1:13:19 | Class MissingAiter | MissingAiter | diff --git a/python/ql/test/query-tests/Statements/general/NonIteratorInForLoop.expected b/python/ql/test/query-tests/Statements/general/NonIteratorInForLoop.expected index 4c79685061f5..d11b87191ba6 100644 --- a/python/ql/test/query-tests/Statements/general/NonIteratorInForLoop.expected +++ b/python/ql/test/query-tests/Statements/general/NonIteratorInForLoop.expected @@ -1 +1 @@ -| test.py:50:1:50:23 | For | This for-loop may attempt to iterate over a $@ of class $@. | test.py:50:10:50:22 | ControlFlowNode for NonIterator() | non-iterable instance | test.py:45:1:45:26 | class NonIterator | NonIterator | +| test.py:50:1:50:23 | For | This for-loop may attempt to iterate over a $@ of class $@. | test.py:50:10:50:22 | NonIterator() | non-iterable instance | test.py:45:1:45:26 | Class NonIterator | NonIterator | diff --git a/python/ql/test/query-tests/Statements/general/test.py b/python/ql/test/query-tests/Statements/general/test.py index eee63fa89e88..7a22fe292321 100644 --- a/python/ql/test/query-tests/Statements/general/test.py +++ b/python/ql/test/query-tests/Statements/general/test.py @@ -174,3 +174,24 @@ def assert_ok(seq): # False positive. ODASA-8042. Fixed in PR #2401. class false_positive: e = (x for x in []) + +# isinstance guard should suppress non-iterable warning +def guarded_iteration(x): + ni = NonIterator() + if isinstance(ni, (list, tuple)): + for item in ni: + pass + +def guarded_iteration_single(x): + ni = NonIterator() + if isinstance(ni, list): + for item in ni: + pass + +# Negated isinstance guard: early return when NOT iterable +def guarded_iteration_negated(x): + ni = NonIterator() + if not isinstance(ni, list): + return + for item in ni: # OK: guarded by negated isinstance + early return + pass