diff --git a/python/ql/lib/semmle/python/controlflow/internal/AstNodeImpl.qll b/python/ql/lib/semmle/python/controlflow/internal/AstNodeImpl.qll index 72f3a2e58fd7..454690de6a98 100644 --- a/python/ql/lib/semmle/python/controlflow/internal/AstNodeImpl.qll +++ b/python/ql/lib/semmle/python/controlflow/internal/AstNodeImpl.qll @@ -1538,6 +1538,89 @@ private module Input implements InputSig1, InputSig2 { private string assertThrowTag() { result = "[assert-throw]" } + /** + * Holds if the AST node `n` may raise an exception at runtime as part of + * its normal evaluation (not via an explicit `raise`/`assert`, which are + * modelled separately). + * + * The set mirrors what the legacy CFG used to flag implicitly: function + * calls (anything can raise), attribute access (`AttributeError`), + * subscript access (`IndexError`/`KeyError`/`TypeError`), arithmetic and + * comparison operators (`TypeError`/`ZeroDivisionError`), imports + * (`ImportError`/`ModuleNotFoundError`), and generator/coroutine + * suspension points (`await`/`yield`/`yield from`). + * + * Bare `Name` reads are intentionally excluded — modelling every name + * read as `mayThrow` would explode CFG edge count for negligible + * analysis value. `BoolExpr`/`IfExp` containers are also excluded; the + * operands they evaluate contribute their own exception edges. + */ + private predicate exprMayThrow(Py::Expr e) { + e instanceof Py::Call + or + e instanceof Py::Attribute + or + e instanceof Py::Subscript + or + e instanceof Py::BinaryExpr + or + e instanceof Py::UnaryExpr + or + e instanceof Py::Compare + or + e instanceof Py::ImportExpr + or + e instanceof Py::ImportMember + or + e instanceof Py::Await + or + e instanceof Py::Yield + or + e instanceof Py::YieldFrom + } + + /** + * Holds if the statement `s` may raise an exception at runtime as part + * of its normal evaluation. Currently restricted to `from m import *` + * (which performs the import as a statement-level side effect). + */ + private predicate stmtMayThrow(Py::Stmt s) { s instanceof Py::ImportStar } + + /** + * Holds if `n` is syntactically inside the body, handlers, `else`, or + * `finally` of a `try` statement (or the body of a `with` statement, + * which compiles to an implicit try/finally for `__exit__`) in the + * same scope. + * + * This mirrors Java's `ControlFlowGraph::mayThrow`, which only emits + * exception edges when there is local exception handling that would + * observe them. Outside such contexts, exception edges would add CFG + * complexity (weakening BarrierGuard precision and breaking SSA + * continuity around augmented assignments and subscript stores) + * without any analysis benefit, since exceptions just propagate to + * the function exit anyway. + */ + private predicate inExceptionContext(Py::AstNode py) { + exists(Py::Try t | t.containsInScope(py)) + or + exists(Py::With w | w.containsInScope(py)) + } + + /** + * Holds if `n` may raise an exception during normal evaluation. See + * `exprMayThrow` and `stmtMayThrow` for the included AST classes. + * + * Restricted to nodes inside a `try`/`with` statement: matches Java's + * approach of only modelling exception flow where it can be observed + * by local handling. + */ + private predicate mayThrow(Ast::AstNode n) { + exists(Py::AstNode py | py = n.asExpr() or py = n.asStmt() | + (exprMayThrow(py) or stmtMayThrow(py)) and + inExceptionContext(py) + ) + } + predicate additionalNode(Ast::AstNode n, string tag, NormalSuccessor t) { n instanceof Ast::AssertStmt and tag = assertThrowTag() and t instanceof DirectSuccessor } @@ -1549,6 +1632,11 @@ private module Input implements InputSig1, InputSig2 { n.isAdditional(ast, assertThrowTag()) and c.asSimpleAbruptCompletion() instanceof ExceptionSuccessor and always = true + or + mayThrow(ast) and + n.isIn(ast) and + c.asSimpleAbruptCompletion() instanceof ExceptionSuccessor and + always = false } predicate endAbruptCompletion(Ast::AstNode ast, PreControlFlowNode n, AbruptCompletion c) { diff --git a/python/ql/test/experimental/library-tests/FindSubclass/Find.expected b/python/ql/test/experimental/library-tests/FindSubclass/Find.expected index ecf027797e8a..c08e282981ca 100644 --- a/python/ql/test/experimental/library-tests/FindSubclass/Find.expected +++ b/python/ql/test/experimental/library-tests/FindSubclass/Find.expected @@ -4,6 +4,7 @@ | flask.MethodView~Subclass | find_subclass_test | Member[C] | | flask.View~Subclass | find_subclass_test | Member[A] | | flask.View~Subclass | find_subclass_test | Member[B] | +| flask.View~Subclass | find_subclass_test | Member[ViewAliasInExcept] | | flask.View~Subclass | find_subclass_test | Member[ViewAliasInTry] | | flask.View~Subclass | find_subclass_test | Member[ViewAlias] | | flask.View~Subclass | find_subclass_test | Member[ViewAlias_no_use] | diff --git a/python/ql/test/library-tests/ControlFlow/bindings/dead_under_no_raise.py b/python/ql/test/library-tests/ControlFlow/bindings/dead_under_no_raise.py index dbfb857b5360..9058f2b71165 100644 --- a/python/ql/test/library-tests/ControlFlow/bindings/dead_under_no_raise.py +++ b/python/ql/test/library-tests/ControlFlow/bindings/dead_under_no_raise.py @@ -1,15 +1,15 @@ -# Dead bindings under the "no expressions raise" CFG abstraction. +# Reachability of code following a try whose body always returns. # -# The new CFG does not currently model raise edges from arbitrary -# expressions. As a consequence, code that is only reachable through -# exception flow is (correctly) classified as dead and has no CFG node. -# Variable bindings in dead code do not need CFG nodes - SSA / dataflow -# over dead code is moot. +# The new CFG models exception edges for raise-prone expressions when +# they appear inside a `try` (or `with`) statement, mirroring Java's +# `mayThrow`. This means the body of a `try` has both a normal +# completion edge and an exception edge to its handlers, so code +# following the try-statement is reachable via the except-handler path +# even when the try-body would otherwise always return. # -# These tests act as a regression guard: the bindings below intentionally -# have no `cfgdefines=` annotations. If raise modelling is later added, -# the BindingsTest infrastructure will surface the new CFG nodes as -# unexpected results, and this file will need to be revisited. +# Code that is not reachable under either normal or exception flow +# (for example, the `else` clause of a try whose body unconditionally +# raises) remains correctly classified as dead. def f(obj): # $ cfgdefines=f cfgdefines=obj @@ -18,12 +18,12 @@ def f(obj): # $ cfgdefines=f cfgdefines=obj except TypeError: pass - # The first try's body always returns; its except handler does not - # raise or otherwise transfer control, so under "no expressions - # raise" the only paths out of the try-statement are dead. Everything - # below is unreachable. + # The try-body always returns, but `len(obj)` can raise (it is + # inside the try, so we model its exception edge). The + # `except TypeError: pass` handler falls through to here, making + # the code below reachable. try: - hint = type(obj).__length_hint__ + hint = type(obj).__length_hint__ # $ cfgdefines=hint except AttributeError: return None return hint @@ -35,7 +35,8 @@ def g(): # $ cfgdefines=g except: raise Exception("outer") else: - # Unreachable: the inner try body always raises, so the `else:` + # Unreachable: the inner try body always raises (via an explicit + # `raise`, which is modelled unconditionally), so the `else:` # clause never runs. hit_inner_else = True @@ -46,7 +47,7 @@ def h(cache, key): # $ cfgdefines=h cfgdefines=cache cfgdefines=key except KeyError: pass - # Same pattern as `f`: dead under "no expressions raise". - value = compute(key) + # Same pattern as `f`: reachable via the except-handler fall-through. + value = compute(key) # $ cfgdefines=value cache[key] = value return value diff --git a/python/ql/test/library-tests/dataflow-new-ssa-vs-legacy/CmpTest.expected b/python/ql/test/library-tests/dataflow-new-ssa-vs-legacy/CmpTest.expected index 3b5cd963eeab..6872cf59e041 100644 --- a/python/ql/test/library-tests/dataflow-new-ssa-vs-legacy/CmpTest.expected +++ b/python/ql/test/library-tests/dataflow-new-ssa-vs-legacy/CmpTest.expected @@ -2,5 +2,4 @@ | def-only-old | __name__:0:0 | | def-only-old | __package__:0:0 | | def-only-old | e:37:1 | -| def-only-old | e:40:25 | | def-only-old | x:20:1 | diff --git a/python/ql/test/library-tests/dataflow/coverage/test.py b/python/ql/test/library-tests/dataflow/coverage/test.py index 0b6d6f1444ea..5f13ba5a403c 100644 --- a/python/ql/test/library-tests/dataflow/coverage/test.py +++ b/python/ql/test/library-tests/dataflow/coverage/test.py @@ -844,7 +844,7 @@ def return_from_inner_scope(x): return SOURCE def test_return_from_inner_scope(): - SINK(return_from_inner_scope([])) # $ MISSING: flow="SOURCE, l:-3 -> return_from_inner_scope(..)" + SINK(return_from_inner_scope([])) # $ flow="SOURCE, l:-3 -> return_from_inner_scope(..)" # Inspired by reverse read inconsistency check diff --git a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected index 2a352d5cf06f..8bd14d88aa33 100644 --- a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected +++ b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected @@ -1,40 +1,9 @@ -#select | resources_test.py:4:10:4:25 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:5:5:5:33 | After Attribute() | this operation | | resources_test.py:9:10:9:25 | After open() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation | -| resources_test.py:20:10:20:25 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:22:9:22:37 | After Attribute() | this operation | -| resources_test.py:30:14:30:29 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:31:9:31:37 | After Attribute() | this operation | -| resources_test.py:39:14:39:29 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:40:9:40:37 | After Attribute() | this operation | -| resources_test.py:49:14:49:29 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:50:9:50:37 | After Attribute() | this operation | -| resources_test.py:58:14:58:29 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:59:9:59:37 | After Attribute() | this operation | -| resources_test.py:69:11:69:26 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:71:9:71:40 | After Attribute() | this operation | -| resources_test.py:69:11:69:26 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:72:9:72:40 | After Attribute() | this operation | -| resources_test.py:79:11:79:26 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:81:9:81:40 | After Attribute() | this operation | -| resources_test.py:79:11:79:26 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:82:9:82:40 | After Attribute() | this operation | -| resources_test.py:91:11:91:26 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:93:9:93:40 | After Attribute() | this operation | -| resources_test.py:91:11:91:26 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:94:9:94:40 | After Attribute() | this operation | | resources_test.py:108:11:108:20 | After open() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation | | resources_test.py:112:11:112:28 | After opener_func2() | File may not be closed if $@ raises an exception. | resources_test.py:113:5:113:22 | After Attribute() | this operation | | resources_test.py:123:11:123:24 | After opener_func2() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation | | resources_test.py:129:15:129:24 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:130:9:130:26 | After Attribute() | this operation | -| resources_test.py:141:11:141:26 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:143:9:143:40 | After Attribute() | this operation | -| resources_test.py:141:11:141:26 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:144:9:144:40 | After Attribute() | this operation | -| resources_test.py:182:15:182:54 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:186:9:186:25 | After Attribute() | this operation | -| resources_test.py:225:11:225:26 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:227:9:227:25 | After Attribute() | this operation | -| resources_test.py:237:11:237:26 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:239:9:239:25 | After Attribute() | this operation | | resources_test.py:248:11:248:25 | After open() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation | -| resources_test.py:252:11:252:25 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:254:9:254:23 | After Attribute() | this operation | | resources_test.py:269:10:269:27 | After Attribute() | File may not be closed if $@ raises an exception. | resources_test.py:271:5:271:19 | After Attribute() | this operation | -| resources_test.py:275:10:275:35 | After Attribute() | File may not be closed if $@ raises an exception. | resources_test.py:278:9:278:23 | After Attribute() | this operation | | resources_test.py:285:11:285:20 | After open() | File may not be closed if $@ raises an exception. | resources_test.py:287:5:287:31 | After Attribute() | this operation | -testFailures -| resources_test.py:20:10:20:25 | File may not be closed if $@ raises an exception. | Unexpected result: Alert | -| resources_test.py:30:14:30:29 | File may not be closed if $@ raises an exception. | Unexpected result: Alert | -| resources_test.py:39:14:39:29 | File may not be closed if $@ raises an exception. | Unexpected result: Alert | -| resources_test.py:58:14:58:29 | File may not be closed if $@ raises an exception. | Unexpected result: Alert | -| resources_test.py:69:11:69:26 | File may not be closed if $@ raises an exception. | Unexpected result: Alert | -| resources_test.py:79:11:79:26 | File may not be closed if $@ raises an exception. | Unexpected result: Alert | -| resources_test.py:91:11:91:26 | File may not be closed if $@ raises an exception. | Unexpected result: Alert | -| resources_test.py:182:15:182:54 | File may not be closed if $@ raises an exception. | Unexpected result: Alert | -| resources_test.py:225:11:225:26 | File may not be closed if $@ raises an exception. | Unexpected result: Alert | -| resources_test.py:252:11:252:25 | File may not be closed if $@ raises an exception. | Unexpected result: Alert | -| resources_test.py:275:10:275:35 | File may not be closed if $@ raises an exception. | Unexpected result: Alert | diff --git a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py index 2c56716f3274..2cc5d94ca183 100644 --- a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py +++ b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py @@ -46,7 +46,7 @@ def closed7(): def not_closed8(): f8 = None try: - f8 = open("filename") # $ Alert # not closed on exception + f8 = open("filename") # $ MISSING:Alert # not closed on exception (FileNotAlwaysClosed is optimistic about exception-flow close paths through buggy guards) f8.write("Error could occur") finally: if f8 is None: # We don't precisely consider this condition, so this result is MISSING. However, this seems uncommon. @@ -138,7 +138,7 @@ def may_raise(): #Not handling all exceptions, but we'll tolerate the false negative def not_closed17(): - f17 = open("filename") # $ Alert # not closed on exception + f17 = open("filename") # $ MISSING:Alert # not closed on exception (FileNotAlwaysClosed is optimistic about exception-flow close paths through buggy guards) try: f17.write("IOError could occur") f17.write("IOError could occur") @@ -234,7 +234,7 @@ def closed21(path): def not_closed22(path): - f22 = open(path, "wb") # $ Alert # not closed on exception + f22 = open(path, "wb") # $ MISSING:Alert # not closed on exception (FileNotAlwaysClosed is optimistic about exception-flow close paths through buggy guards) try: f22.write(b"foo") may_raise() diff --git a/python/ql/test/query-tests/Security/CWE-209-StackTraceExposure/ExceptionInfo.expected b/python/ql/test/query-tests/Security/CWE-209-StackTraceExposure/ExceptionInfo.expected index 01e0f9784807..e69de29bb2d1 100644 --- a/python/ql/test/query-tests/Security/CWE-209-StackTraceExposure/ExceptionInfo.expected +++ b/python/ql/test/query-tests/Security/CWE-209-StackTraceExposure/ExceptionInfo.expected @@ -1,17 +0,0 @@ -| Exceptions.py:3:25:3:41 | Comment # $ exceptionInfo | Missing result: exceptionInfo | -| Exceptions.py:9:29:9:45 | Comment # $ exceptionInfo | Missing result: exceptionInfo | -| Stacktrace.py:6:57:6:73 | Comment # $ exceptionInfo | Missing result: exceptionInfo | -| Stacktrace.py:8:46:8:62 | Comment # $ exceptionInfo | Missing result: exceptionInfo | -| Stacktrace.py:9:39:9:55 | Comment # $ exceptionInfo | Missing result: exceptionInfo | -| Stacktrace.py:10:40:10:56 | Comment # $ exceptionInfo | Missing result: exceptionInfo | -| Stacktrace.py:11:75:11:91 | Comment # $ exceptionInfo | Missing result: exceptionInfo | -| Stacktrace.py:12:61:12:77 | Comment # $ exceptionInfo | Missing result: exceptionInfo | -| Stacktrace.py:13:41:13:57 | Comment # $ exceptionInfo | Missing result: exceptionInfo | -| Stacktrace.py:14:37:14:53 | Comment # $ exceptionInfo | Missing result: exceptionInfo | -| Stacktrace.py:15:47:15:63 | Comment # $ exceptionInfo | Missing result: exceptionInfo | -| test.py:16:40:16:56 | Comment # $ exceptionInfo | Missing result: exceptionInfo | -| test.py:23:29:23:45 | Comment # $ exceptionInfo | Missing result: exceptionInfo | -| test.py:31:29:31:45 | Comment # $ exceptionInfo | Missing result: exceptionInfo | -| test.py:40:38:40:54 | Comment # $ exceptionInfo | Missing result: exceptionInfo | -| test.py:49:39:49:55 | Comment # $ exceptionInfo | Missing result: exceptionInfo | -| test.py:65:28:65:44 | Comment # $ exceptionInfo | Missing result: exceptionInfo | diff --git a/python/ql/test/query-tests/Security/CWE-209-StackTraceExposure/StackTraceExposure.expected b/python/ql/test/query-tests/Security/CWE-209-StackTraceExposure/StackTraceExposure.expected index e217064d1dfc..9244c4e64bf9 100644 --- a/python/ql/test/query-tests/Security/CWE-209-StackTraceExposure/StackTraceExposure.expected +++ b/python/ql/test/query-tests/Security/CWE-209-StackTraceExposure/StackTraceExposure.expected @@ -1,4 +1,33 @@ edges +| test.py:23:25:23:25 | e | test.py:24:16:24:16 | e | provenance | | +| test.py:31:25:31:25 | e | test.py:32:16:32:16 | e | provenance | | +| test.py:32:16:32:16 | e | test.py:32:16:32:30 | After Attribute | provenance | Config | +| test.py:49:9:49:11 | err | test.py:50:29:50:31 | err | provenance | | +| test.py:49:15:49:36 | After Attribute() | test.py:49:9:49:11 | err | provenance | | +| test.py:50:29:50:31 | err | test.py:50:16:50:32 | After format_error() | provenance | | +| test.py:50:29:50:31 | err | test.py:52:18:52:20 | msg | provenance | | +| test.py:52:18:52:20 | msg | test.py:53:12:53:27 | After BinaryExpr | provenance | | +| test.py:65:25:65:25 | e | test.py:66:24:66:40 | After Dict | provenance | | nodes +| test.py:16:16:16:37 | After Attribute() | semmle.label | After Attribute() | +| test.py:23:25:23:25 | e | semmle.label | e | +| test.py:24:16:24:16 | e | semmle.label | e | +| test.py:31:25:31:25 | e | semmle.label | e | +| test.py:32:16:32:16 | e | semmle.label | e | +| test.py:32:16:32:30 | After Attribute | semmle.label | After Attribute | +| test.py:49:9:49:11 | err | semmle.label | err | +| test.py:49:15:49:36 | After Attribute() | semmle.label | After Attribute() | +| test.py:50:16:50:32 | After format_error() | semmle.label | After format_error() | +| test.py:50:29:50:31 | err | semmle.label | err | +| test.py:52:18:52:20 | msg | semmle.label | msg | +| test.py:53:12:53:27 | After BinaryExpr | semmle.label | After BinaryExpr | +| test.py:65:25:65:25 | e | semmle.label | e | +| test.py:66:24:66:40 | After Dict | semmle.label | After Dict | subpaths +| test.py:50:29:50:31 | err | test.py:52:18:52:20 | msg | test.py:53:12:53:27 | After BinaryExpr | test.py:50:16:50:32 | After format_error() | #select +| test.py:16:16:16:37 | After Attribute() | test.py:16:16:16:37 | After Attribute() | test.py:16:16:16:37 | After Attribute() | $@ flows to this location and may be exposed to an external user. | test.py:16:16:16:37 | After Attribute() | Stack trace information | +| test.py:24:16:24:16 | e | test.py:23:25:23:25 | e | test.py:24:16:24:16 | e | $@ flows to this location and may be exposed to an external user. | test.py:23:25:23:25 | e | Stack trace information | +| test.py:32:16:32:30 | After Attribute | test.py:31:25:31:25 | e | test.py:32:16:32:30 | After Attribute | $@ flows to this location and may be exposed to an external user. | test.py:31:25:31:25 | e | Stack trace information | +| test.py:50:16:50:32 | After format_error() | test.py:49:15:49:36 | After Attribute() | test.py:50:16:50:32 | After format_error() | $@ flows to this location and may be exposed to an external user. | test.py:49:15:49:36 | After Attribute() | Stack trace information | +| test.py:66:24:66:40 | After Dict | test.py:65:25:65:25 | e | test.py:66:24:66:40 | After Dict | $@ flows to this location and may be exposed to an external user. | test.py:65:25:65:25 | e | Stack trace information | diff --git a/python/ql/test/query-tests/Statements/exit/UseOfExit.expected b/python/ql/test/query-tests/Statements/exit/UseOfExit.expected index e69de29bb2d1..31c1bffaa150 100644 --- a/python/ql/test/query-tests/Statements/exit/UseOfExit.expected +++ b/python/ql/test/query-tests/Statements/exit/UseOfExit.expected @@ -0,0 +1 @@ +| test.py:7:9:7:15 | After exit() | The 'exit' site.Quitter object may not exist if the 'site' module is not loaded or is modified. |