Skip to content

Commit 83c5f33

Browse files
yoffCopilot
yoff
and
Copilot
committed
Python: model exception edges for raise-prone expressions inside try/with
The new CFG previously only emitted exception edges for explicit `raise` and `assert` statements. As a result, code that became reachable only via the exception path of an arbitrary expression (e.g., the body of an `except` handler following a try-body whose `call()` could raise) was classified as dead, breaking analyses like StackTraceExposure, FileNotAlwaysClosed, ExceptionInfo, UseOfExit, and CatchingBaseException. This commit adds a `mayThrow` predicate over expressions that are known sources of implicit exceptions in Python (calls, attribute access, subscripts, arithmetic/comparison operators, imports, await/yield/yield from) plus `from m import *` at the statement level, and routes them through the shared CFG's `beginAbruptCompletion(_, _, ExceptionSuccessor, always=false)` hook. The set of exception sources is restricted to nodes that are syntactically inside a `try`/`with` statement in the same scope. This mirrors Java's `ControlFlowGraph::mayThrow`, which only emits exception edges where local handling can observe them — outside such contexts, the edges add CFG complexity (weakening BarrierGuard precision and breaking SSA continuity around augmented assignments and subscript stores) without analysis benefit, since exceptions just propagate to the function exit anyway. Net effect on the test suite: ~100 alerts restored across the exception- related query tests (StackTraceExposure +29, ExceptionInfo +17, FileNotAlwaysClosed +52, UseOfExit +1, CatchingBaseException restored) with no precision regressions. Affected `.expected` files and the regression-guard `dead_under_no_raise.py` are updated accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 3f9c603 commit 83c5f33

9 files changed

Lines changed: 139 additions & 70 deletions

File tree

‎python/ql/lib/semmle/python/controlflow/internal/AstNodeImpl.qll‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1538,6 +1538,89 @@ private module Input implements InputSig1, InputSig2 {
15381538

15391539
private string assertThrowTag() { result = "[assert-throw]" }
15401540

1541+
/**
1542+
* Holds if the AST node `n` may raise an exception at runtime as part of
1543+
* its normal evaluation (not via an explicit `raise`/`assert`, which are
1544+
* modelled separately).
1545+
*
1546+
* The set mirrors what the legacy CFG used to flag implicitly: function
1547+
* calls (anything can raise), attribute access (`AttributeError`),
1548+
* subscript access (`IndexError`/`KeyError`/`TypeError`), arithmetic and
1549+
* comparison operators (`TypeError`/`ZeroDivisionError`), imports
1550+
* (`ImportError`/`ModuleNotFoundError`), and generator/coroutine
1551+
* suspension points (`await`/`yield`/`yield from`).
1552+
*
1553+
* Bare `Name` reads are intentionally excluded — modelling every name
1554+
* read as `mayThrow` would explode CFG edge count for negligible
1555+
* analysis value. `BoolExpr`/`IfExp` containers are also excluded; the
1556+
* operands they evaluate contribute their own exception edges.
1557+
*/
1558+
private predicate exprMayThrow(Py::Expr e) {
1559+
e instanceof Py::Call
1560+
or
1561+
e instanceof Py::Attribute
1562+
or
1563+
e instanceof Py::Subscript
1564+
or
1565+
e instanceof Py::BinaryExpr
1566+
or
1567+
e instanceof Py::UnaryExpr
1568+
or
1569+
e instanceof Py::Compare
1570+
or
1571+
e instanceof Py::ImportExpr
1572+
or
1573+
e instanceof Py::ImportMember
1574+
or
1575+
e instanceof Py::Await
1576+
or
1577+
e instanceof Py::Yield
1578+
or
1579+
e instanceof Py::YieldFrom
1580+
}
1581+
1582+
/**
1583+
* Holds if the statement `s` may raise an exception at runtime as part
1584+
* of its normal evaluation. Currently restricted to `from m import *`
1585+
* (which performs the import as a statement-level side effect).
1586+
*/
1587+
private predicate stmtMayThrow(Py::Stmt s) { s instanceof Py::ImportStar }
1588+
1589+
/**
1590+
* Holds if `n` is syntactically inside the body, handlers, `else`, or
1591+
* `finally` of a `try` statement (or the body of a `with` statement,
1592+
* which compiles to an implicit try/finally for `__exit__`) in the
1593+
* same scope.
1594+
*
1595+
* This mirrors Java's `ControlFlowGraph::mayThrow`, which only emits
1596+
* exception edges when there is local exception handling that would
1597+
* observe them. Outside such contexts, exception edges would add CFG
1598+
* complexity (weakening BarrierGuard precision and breaking SSA
1599+
* continuity around augmented assignments and subscript stores)
1600+
* without any analysis benefit, since exceptions just propagate to
1601+
* the function exit anyway.
1602+
*/
1603+
private predicate inExceptionContext(Py::AstNode py) {
1604+
exists(Py::Try t | t.containsInScope(py))
1605+
or
1606+
exists(Py::With w | w.containsInScope(py))
1607+
}
1608+
1609+
/**
1610+
* Holds if `n` may raise an exception during normal evaluation. See
1611+
* `exprMayThrow` and `stmtMayThrow` for the included AST classes.
1612+
*
1613+
* Restricted to nodes inside a `try`/`with` statement: matches Java's
1614+
* approach of only modelling exception flow where it can be observed
1615+
* by local handling.
1616+
*/
1617+
private predicate mayThrow(Ast::AstNode n) {
1618+
exists(Py::AstNode py | py = n.asExpr() or py = n.asStmt() |
1619+
(exprMayThrow(py) or stmtMayThrow(py)) and
1620+
inExceptionContext(py)
1621+
)
1622+
}
1623+
15411624
predicate additionalNode(Ast::AstNode n, string tag, NormalSuccessor t) {
15421625
n instanceof Ast::AssertStmt and tag = assertThrowTag() and t instanceof DirectSuccessor
15431626
}
@@ -1549,6 +1632,11 @@ private module Input implements InputSig1, InputSig2 {
15491632
n.isAdditional(ast, assertThrowTag()) and
15501633
c.asSimpleAbruptCompletion() instanceof ExceptionSuccessor and
15511634
always = true
1635+
or
1636+
mayThrow(ast) and
1637+
n.isIn(ast) and
1638+
c.asSimpleAbruptCompletion() instanceof ExceptionSuccessor and
1639+
always = false
15521640
}
15531641

15541642
predicate endAbruptCompletion(Ast::AstNode ast, PreControlFlowNode n, AbruptCompletion c) {

‎python/ql/test/experimental/library-tests/FindSubclass/Find.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
| flask.MethodView~Subclass | find_subclass_test | Member[C] |
55
| flask.View~Subclass | find_subclass_test | Member[A] |
66
| flask.View~Subclass | find_subclass_test | Member[B] |
7+
| flask.View~Subclass | find_subclass_test | Member[ViewAliasInExcept] |
78
| flask.View~Subclass | find_subclass_test | Member[ViewAliasInTry] |
89
| flask.View~Subclass | find_subclass_test | Member[ViewAlias] |
910
| flask.View~Subclass | find_subclass_test | Member[ViewAlias_no_use] |

‎python/ql/test/library-tests/ControlFlow/bindings/dead_under_no_raise.py‎

Lines changed: 19 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
1-
# Dead bindings under the "no expressions raise" CFG abstraction.
1+
# Reachability of code following a try whose body always returns.
22
#
3-
# The new CFG does not currently model raise edges from arbitrary
4-
# expressions. As a consequence, code that is only reachable through
5-
# exception flow is (correctly) classified as dead and has no CFG node.
6-
# Variable bindings in dead code do not need CFG nodes - SSA / dataflow
7-
# over dead code is moot.
3+
# The new CFG models exception edges for raise-prone expressions when
4+
# they appear inside a `try` (or `with`) statement, mirroring Java's
5+
# `mayThrow`. This means the body of a `try` has both a normal
6+
# completion edge and an exception edge to its handlers, so code
7+
# following the try-statement is reachable via the except-handler path
8+
# even when the try-body would otherwise always return.
89
#
9-
# These tests act as a regression guard: the bindings below intentionally
10-
# have no `cfgdefines=` annotations. If raise modelling is later added,
11-
# the BindingsTest infrastructure will surface the new CFG nodes as
12-
# unexpected results, and this file will need to be revisited.
10+
# Code that is not reachable under either normal or exception flow
11+
# (for example, the `else` clause of a try whose body unconditionally
12+
# raises) remains correctly classified as dead.
1313

1414

1515
def f(obj): # $ cfgdefines=f cfgdefines=obj
@@ -18,12 +18,12 @@ def f(obj): # $ cfgdefines=f cfgdefines=obj
1818
except TypeError:
1919
pass
2020

21-
# The first try's body always returns; its except handler does not
22-
# raise or otherwise transfer control, so under "no expressions
23-
# raise" the only paths out of the try-statement are dead. Everything
24-
# below is unreachable.
21+
# The try-body always returns, but `len(obj)` can raise (it is
22+
# inside the try, so we model its exception edge). The
23+
# `except TypeError: pass` handler falls through to here, making
24+
# the code below reachable.
2525
try:
26-
hint = type(obj).__length_hint__
26+
hint = type(obj).__length_hint__ # $ cfgdefines=hint
2727
except AttributeError:
2828
return None
2929
return hint
@@ -35,7 +35,8 @@ def g(): # $ cfgdefines=g
3535
except:
3636
raise Exception("outer")
3737
else:
38-
# Unreachable: the inner try body always raises, so the `else:`
38+
# Unreachable: the inner try body always raises (via an explicit
39+
# `raise`, which is modelled unconditionally), so the `else:`
3940
# clause never runs.
4041
hit_inner_else = True
4142

@@ -46,7 +47,7 @@ def h(cache, key): # $ cfgdefines=h cfgdefines=cache cfgdefines=key
4647
except KeyError:
4748
pass
4849

49-
# Same pattern as `f`: dead under "no expressions raise".
50-
value = compute(key)
50+
# Same pattern as `f`: reachable via the except-handler fall-through.
51+
value = compute(key) # $ cfgdefines=value
5152
cache[key] = value
5253
return value

‎python/ql/test/library-tests/dataflow-new-ssa-vs-legacy/CmpTest.expected‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,5 +2,4 @@
22
| def-only-old | __name__:0:0 |
33
| def-only-old | __package__:0:0 |
44
| def-only-old | e:37:1 |
5-
| def-only-old | e:40:25 |
65
| def-only-old | x:20:1 |

‎python/ql/test/library-tests/dataflow/coverage/NormalDataflowTest.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,3 @@
11
missingAnnotationOnSink
22
testFailures
3+
| test.py:847:10:847:36 | After return_from_inner_scope() | Fixed missing result: flow="SOURCE, l:-3 -> return_from_inner_scope(..)" |

‎python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected‎

Lines changed: 0 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,43 +1,9 @@
1-
#select
21
| 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 |
32
| resources_test.py:9:10:9:25 | After open() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation |
4-
| 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 |
5-
| 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 |
6-
| 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 |
7-
| 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 |
8-
| 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 |
9-
| 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 |
10-
| 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 |
11-
| 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 |
12-
| 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 |
13-
| 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 |
14-
| 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 |
153
| resources_test.py:108:11:108:20 | After open() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation |
164
| 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 |
175
| 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 |
186
| 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 |
19-
| 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 |
20-
| 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 |
21-
| 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 |
22-
| 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 |
23-
| 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 |
247
| resources_test.py:248:11:248:25 | After open() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation |
25-
| 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 |
268
| 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 |
27-
| 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 |
289
| 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 |
29-
testFailures
30-
| resources_test.py:20:10:20:25 | File may not be closed if $@ raises an exception. | Unexpected result: Alert |
31-
| resources_test.py:30:14:30:29 | File may not be closed if $@ raises an exception. | Unexpected result: Alert |
32-
| resources_test.py:39:14:39:29 | File may not be closed if $@ raises an exception. | Unexpected result: Alert |
33-
| resources_test.py:49:14:49:29 | File may not be closed if $@ raises an exception. | Fixed missing result: Alert |
34-
| resources_test.py:58:14:58:29 | File may not be closed if $@ raises an exception. | Unexpected result: Alert |
35-
| resources_test.py:69:11:69:26 | File may not be closed if $@ raises an exception. | Unexpected result: Alert |
36-
| resources_test.py:79:11:79:26 | File may not be closed if $@ raises an exception. | Unexpected result: Alert |
37-
| resources_test.py:91:11:91:26 | File may not be closed if $@ raises an exception. | Unexpected result: Alert |
38-
| resources_test.py:141:11:141:26 | File may not be closed if $@ raises an exception. | Fixed missing result: Alert |
39-
| resources_test.py:182:15:182:54 | File may not be closed if $@ raises an exception. | Unexpected result: Alert |
40-
| resources_test.py:225:11:225:26 | File may not be closed if $@ raises an exception. | Unexpected result: Alert |
41-
| resources_test.py:237:11:237:26 | File may not be closed if $@ raises an exception. | Fixed missing result: Alert |
42-
| resources_test.py:252:11:252:25 | File may not be closed if $@ raises an exception. | Unexpected result: Alert |
43-
| resources_test.py:275:10:275:35 | File may not be closed if $@ raises an exception. | Unexpected result: Alert |

‎python/ql/test/query-tests/Security/CWE-209-StackTraceExposure/ExceptionInfo.expected‎

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +0,0 @@
1-
| Exceptions.py:3:25:3:41 | Comment # $ exceptionInfo | Missing result: exceptionInfo |
2-
| Exceptions.py:9:29:9:45 | Comment # $ exceptionInfo | Missing result: exceptionInfo |
3-
| Stacktrace.py:6:57:6:73 | Comment # $ exceptionInfo | Missing result: exceptionInfo |
4-
| Stacktrace.py:8:46:8:62 | Comment # $ exceptionInfo | Missing result: exceptionInfo |
5-
| Stacktrace.py:9:39:9:55 | Comment # $ exceptionInfo | Missing result: exceptionInfo |
6-
| Stacktrace.py:10:40:10:56 | Comment # $ exceptionInfo | Missing result: exceptionInfo |
7-
| Stacktrace.py:11:75:11:91 | Comment # $ exceptionInfo | Missing result: exceptionInfo |
8-
| Stacktrace.py:12:61:12:77 | Comment # $ exceptionInfo | Missing result: exceptionInfo |
9-
| Stacktrace.py:13:41:13:57 | Comment # $ exceptionInfo | Missing result: exceptionInfo |
10-
| Stacktrace.py:14:37:14:53 | Comment # $ exceptionInfo | Missing result: exceptionInfo |
11-
| Stacktrace.py:15:47:15:63 | Comment # $ exceptionInfo | Missing result: exceptionInfo |
12-
| test.py:16:40:16:56 | Comment # $ exceptionInfo | Missing result: exceptionInfo |
13-
| test.py:23:29:23:45 | Comment # $ exceptionInfo | Missing result: exceptionInfo |
14-
| test.py:31:29:31:45 | Comment # $ exceptionInfo | Missing result: exceptionInfo |
15-
| test.py:40:38:40:54 | Comment # $ exceptionInfo | Missing result: exceptionInfo |
16-
| test.py:49:39:49:55 | Comment # $ exceptionInfo | Missing result: exceptionInfo |
17-
| test.py:65:28:65:44 | Comment # $ exceptionInfo | Missing result: exceptionInfo |

‎python/ql/test/query-tests/Security/CWE-209-StackTraceExposure/StackTraceExposure.expected‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,33 @@
11
edges
2+
| test.py:23:25:23:25 | e | test.py:24:16:24:16 | e | provenance | |
3+
| test.py:31:25:31:25 | e | test.py:32:16:32:16 | e | provenance | |
4+
| test.py:32:16:32:16 | e | test.py:32:16:32:30 | After Attribute | provenance | Config |
5+
| test.py:49:9:49:11 | err | test.py:50:29:50:31 | err | provenance | |
6+
| test.py:49:15:49:36 | After Attribute() | test.py:49:9:49:11 | err | provenance | |
7+
| test.py:50:29:50:31 | err | test.py:50:16:50:32 | After format_error() | provenance | |
8+
| test.py:50:29:50:31 | err | test.py:52:18:52:20 | msg | provenance | |
9+
| test.py:52:18:52:20 | msg | test.py:53:12:53:27 | After BinaryExpr | provenance | |
10+
| test.py:65:25:65:25 | e | test.py:66:24:66:40 | After Dict | provenance | |
211
nodes
12+
| test.py:16:16:16:37 | After Attribute() | semmle.label | After Attribute() |
13+
| test.py:23:25:23:25 | e | semmle.label | e |
14+
| test.py:24:16:24:16 | e | semmle.label | e |
15+
| test.py:31:25:31:25 | e | semmle.label | e |
16+
| test.py:32:16:32:16 | e | semmle.label | e |
17+
| test.py:32:16:32:30 | After Attribute | semmle.label | After Attribute |
18+
| test.py:49:9:49:11 | err | semmle.label | err |
19+
| test.py:49:15:49:36 | After Attribute() | semmle.label | After Attribute() |
20+
| test.py:50:16:50:32 | After format_error() | semmle.label | After format_error() |
21+
| test.py:50:29:50:31 | err | semmle.label | err |
22+
| test.py:52:18:52:20 | msg | semmle.label | msg |
23+
| test.py:53:12:53:27 | After BinaryExpr | semmle.label | After BinaryExpr |
24+
| test.py:65:25:65:25 | e | semmle.label | e |
25+
| test.py:66:24:66:40 | After Dict | semmle.label | After Dict |
326
subpaths
27+
| 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() |
428
#select
29+
| 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 |
30+
| 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 |
31+
| 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 |
32+
| 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 |
33+
| 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 |

‎python/ql/test/query-tests/Statements/exit/UseOfExit.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
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. |

0 commit comments

Comments
 (0)