Repository: python/cpython
Pull request: gh-104482: Fix error handling bugs in ast.c
Fixes #104482.

<!-- gh-issue-number: gh-104482 -->
* Issue: gh-104482
<!-- /gh-issue-number -->


Problem statement:
Error handling bugs in ast.c
in validate_pattern, there are two places where in case of error we break out of a loop, but we also need to break out of the switch.

```
--- a/Python/ast.c
+++ b/Python/ast.c
@@ -580,7 +580,9 @@ validate_pattern(struct validator *state, pattern_ty p, int star_ok)
                     break;
                 }
             }
-
+            if (ret == 0) {
+                break;
+            }
             ret = validate_patterns(state, p->v.MatchMapping.patterns, /*star_ok=*/0);
             break;
         case MatchClass_kind:
@@ -620,6 +622,9 @@ validate_pattern(struct validator *state, pattern_ty p, int star_ok)
                 }
             }
 
+            if (ret == 0) {
+                break;
+            }
             if (!validate_patterns(state, p->v.MatchClass.patterns, /*star_ok=*/0)) {
                 ret = 0;
                 break;
```

If we don't do this we can end up calling _PyAST_Compile with an error set.



<!-- gh-linked-prs -->
### Linked PRs
* gh-104483
* gh-104514
<!-- /gh-linked-prs -->

Hints from issue discussion:

Commit message: add test to cover bad name case. Add assertions in validation functions to catch this as soon as it occurs

Patch to review:
diff --git a/Lib/test/test_ast.py b/Lib/test/test_ast.py
index fdd21aca06ffdd..34808ed8562e11 100644
--- a/Lib/test/test_ast.py
+++ b/Lib/test/test_ast.py
@@ -2035,6 +2035,12 @@ def test_stdlib_validates(self):
             kwd_attrs=[],
             kwd_patterns=[ast.MatchStar()]
         ),
+        ast.MatchClass(
+            constant_true,  # invalid name
+            patterns=[],
+            kwd_attrs=['True'],
+            kwd_patterns=[pattern_1]
+        ),
         ast.MatchSequence(
             [
                 ast.MatchStar("True")
diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-05-14-18-56-54.gh-issue-104482.yaQsv8.rst b/Misc/NEWS.d/next/Core and Builtins/2023-05-14-18-56-54.gh-issue-104482.yaQsv8.rst
new file mode 100644
index 00000000000000..38121fa91a6e24
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and Builtins/2023-05-14-18-56-54.gh-issue-104482.yaQsv8.rst	
@@ -0,0 +1 @@
+Fix two error handling bugs in ast.c's parsing of pattern matching statements.
diff --git a/Python/ast.c b/Python/ast.c
index 50fc8e01fb3f69..17f4663024db8a 100644
--- a/Python/ast.c
+++ b/Python/ast.c
@@ -46,6 +46,7 @@ static int validate_pattern(struct validator *, pattern_ty, int);
 static int
 validate_name(PyObject *name)
 {
+    assert(!PyErr_Occurred());
     assert(PyUnicode_Check(name));
     static const char * const forbidden[] = {
         "None",
@@ -65,6 +66,7 @@ validate_name(PyObject *name)
 static int
 validate_comprehension(struct validator *state, asdl_comprehension_seq *gens)
 {
+    assert(!PyErr_Occurred());
     Py_ssize_t i;
     if (!asdl_seq_LEN(gens)) {
         PyErr_SetString(PyExc_ValueError, "comprehension with no generators");
@@ -83,6 +85,7 @@ validate_comprehension(struct validator *state, asdl_comprehension_seq *gens)
 static int
 validate_keywords(struct validator *state, asdl_keyword_seq *keywords)
 {
+    assert(!PyErr_Occurred());
     Py_ssize_t i;
     for (i = 0; i < asdl_seq_LEN(keywords); i++)
         if (!validate_expr(state, (asdl_seq_GET(keywords, i))->value, Load))
@@ -93,6 +96,7 @@ validate_keywords(struct validator *state, asdl_keyword_seq *keywords)
 static int
 validate_args(struct validator *state, asdl_arg_seq *args)
 {
+    assert(!PyErr_Occurred());
     Py_ssize_t i;
     for (i = 0; i < asdl_seq_LEN(args); i++) {
         arg_ty arg = asdl_seq_GET(args, i);
@@ -121,6 +125,7 @@ expr_context_name(expr_context_ty ctx)
 static int
 validate_arguments(struct validator *state, arguments_ty args)
 {
+    assert(!PyErr_Occurred());
     if (!validate_args(state, args->posonlyargs) || !validate_args(state, args->args)) {
         return 0;
     }
@@ -149,6 +154,7 @@ validate_arguments(struct validator *state, arguments_ty args)
 static int
 validate_constant(struct validator *state, PyObject *value)
 {
+    assert(!PyErr_Occurred());
     if (value == Py_None || value == Py_Ellipsis)
         return 1;
 
@@ -205,6 +211,7 @@ validate_constant(struct validator *state, PyObject *value)
 static int
 validate_expr(struct validator *state, expr_ty exp, expr_context_ty ctx)
 {
+    assert(!PyErr_Occurred());
     VALIDATE_POSITIONS(exp);
     int ret = -1;
     if (++state->recursion_depth > state->recursion_limit) {
@@ -465,6 +472,7 @@ ensure_literal_complex(expr_ty exp)
 static int
 validate_pattern_match_value(struct validator *state, expr_ty exp)
 {
+    assert(!PyErr_Occurred());
     if (!validate_expr(state, exp, Load)) {
         return 0;
     }
@@ -518,6 +526,7 @@ validate_pattern_match_value(struct validator *state, expr_ty exp)
 static int
 validate_capture(PyObject *name)
 {
+    assert(!PyErr_Occurred());
     if (_PyUnicode_EqualToASCIIString(name, "_")) {
         PyErr_Format(PyExc_ValueError, "can't capture name '_' in patterns");
         return 0;
@@ -528,6 +537,7 @@ validate_capture(PyObject *name)
 static int
 validate_pattern(struct validator *state, pattern_ty p, int star_ok)
 {
+    assert(!PyErr_Occurred());
     VALIDATE_POSITIONS(p);
     int ret = -1;
     if (++state->recursion_depth > state->recursion_limit) {
@@ -580,7 +590,9 @@ validate_pattern(struct validator *state, pattern_ty p, int star_ok)
                     break;
                 }
             }
-
+            if (ret == 0) {
+                break;
+            }
             ret = validate_patterns(state, p->v.MatchMapping.patterns, /*star_ok=*/0);
             break;
         case MatchClass_kind:
@@ -611,6 +623,9 @@ validate_pattern(struct validator *state, pattern_ty p, int star_ok)
                     break;
                 }
             }
+            if (ret == 0) {
+                break;
+            }
 
             for (Py_ssize_t i = 0; i < asdl_seq_LEN(p->v.MatchClass.kwd_attrs); i++) {
                 PyObject *identifier = asdl_seq_GET(p->v.MatchClass.kwd_attrs, i);
@@ -619,6 +634,9 @@ validate_pattern(struct validator *state, pattern_ty p, int star_ok)
                     break;
                 }
             }
+            if (ret == 0) {
+                break;
+            }
 
             if (!validate_patterns(state, p->v.MatchClass.patterns, /*star_ok=*/0)) {
                 ret = 0;
@@ -685,6 +703,7 @@ _validate_nonempty_seq(asdl_seq *seq, const char *what, const char *owner)
 static int
 validate_assignlist(struct validator *state, asdl_expr_seq *targets, expr_context_ty ctx)
 {
+    assert(!PyErr_Occurred());
     return validate_nonempty_seq(targets, "targets", ctx == Del ? "Delete" : "Assign") &&
         validate_exprs(state, targets, ctx, 0);
 }
@@ -692,15 +711,16 @@ validate_assignlist(struct validator *state, asdl_expr_seq *targets, expr_contex
 static int
 validate_body(struct validator *state, asdl_stmt_seq *body, const char *owner)
 {
+    assert(!PyErr_Occurred());
     return validate_nonempty_seq(body, "body", owner) && validate_stmts(state, body);
 }
 
 static int
 validate_stmt(struct validator *state, stmt_ty stmt)
 {
+    assert(!PyErr_Occurred());
     VALIDATE_POSITIONS(stmt);
     int ret = -1;
-    Py_ssize_t i;
     if (++state->recursion_depth > state->recursion_limit) {
         PyErr_SetString(PyExc_RecursionError,
                         "maximum recursion depth exceeded during compilation");
@@ -771,7 +791,7 @@ validate_stmt(struct validator *state, stmt_ty stmt)
     case With_kind:
         if (!validate_nonempty_seq(stmt->v.With.items, "items", "With"))
             return 0;
-        for (i = 0; i < asdl_seq_LEN(stmt->v.With.items); i++) {
+        for (Py_ssize_t i = 0; i < asdl_seq_LEN(stmt->v.With.items); i++) {
             withitem_ty item = asdl_seq_GET(stmt->v.With.items, i);
             if (!validate_expr(state, item->context_expr, Load) ||
                 (item->optional_vars && !validate_expr(state, item->optional_vars, Store)))
@@ -782,7 +802,7 @@ validate_stmt(struct validator *state, stmt_ty stmt)
     case AsyncWith_kind:
         if (!validate_nonempty_seq(stmt->v.AsyncWith.items, "items", "AsyncWith"))
             return 0;
-        for (i = 0; i < asdl_seq_LEN(stmt->v.AsyncWith.items); i++) {
+        for (Py_ssize_t i = 0; i < asdl_seq_LEN(stmt->v.AsyncWith.items); i++) {
             withitem_ty item = asdl_seq_GET(stmt->v.AsyncWith.items, i);
             if (!validate_expr(state, item->context_expr, Load) ||
                 (item->optional_vars && !validate_expr(state, item->optional_vars, Store)))
@@ -795,7 +815,7 @@ validate_stmt(struct validator *state, stmt_ty stmt)
             || !validate_nonempty_seq(stmt->v.Match.cases, "cases", "Match")) {
             return 0;
         }
-        for (i = 0; i < asdl_seq_LEN(stmt->v.Match.cases); i++) {
+        for (Py_ssize_t i = 0; i < asdl_seq_LEN(stmt->v.Match.cases); i++) {
             match_case_ty m = asdl_seq_GET(stmt->v.Match.cases, i);
             if (!validate_pattern(state, m->pattern, /*star_ok=*/0)
                 || (m->guard && !validate_expr(state, m->guard, Load))
@@ -830,7 +850,7 @@ validate_stmt(struct validator *state, stmt_ty stmt)
             PyErr_SetString(PyExc_ValueError, "Try has orelse but no except handlers");
             return 0;
         }
-        for (i = 0; i < asdl_seq_LEN(stmt->v.Try.handlers); i++) {
+        for (Py_ssize_t i = 0; i < asdl_seq_LEN(stmt->v.Try.handlers); i++) {
             excepthandler_ty handler = asdl_seq_GET(stmt->v.Try.handlers, i);
             VALIDATE_POSITIONS(handler);
             if ((handler->v.ExceptHandler.type &&
@@ -856,7 +876,7 @@ validate_stmt(struct validator *state, stmt_ty stmt)
             PyErr_SetString(PyExc_ValueError, "TryStar has orelse but no except handlers");
             return 0;
         }
-        for (i = 0; i < asdl_seq_LEN(stmt->v.TryStar.handlers); i++) {
+        for (Py_ssize_t i = 0; i < asdl_seq_LEN(stmt->v.TryStar.handlers); i++) {
             excepthandler_ty handler = asdl_seq_GET(stmt->v.TryStar.handlers, i);
             if ((handler->v.ExceptHandler.type &&
                  !validate_expr(state, handler->v.ExceptHandler.type, Load)) ||
@@ -916,8 +936,8 @@ validate_stmt(struct validator *state, stmt_ty stmt)
 static int
 validate_stmts(struct validator *state, asdl_stmt_seq *seq)
 {
-    Py_ssize_t i;
-    for (i = 0; i < asdl_seq_LEN(seq); i++) {
+    assert(!PyErr_Occurred());
+    for (Py_ssize_t i = 0; i < asdl_seq_LEN(seq); i++) {
         stmt_ty stmt = asdl_seq_GET(seq, i);
         if (stmt) {
             if (!validate_stmt(state, stmt))
@@ -935,8 +955,8 @@ validate_stmts(struct validator *state, asdl_stmt_seq *seq)
 static int
 validate_exprs(struct validator *state, asdl_expr_seq *exprs, expr_context_ty ctx, int null_ok)
 {
-    Py_ssize_t i;
-    for (i = 0; i < asdl_seq_LEN(exprs); i++) {
+    assert(!PyErr_Occurred());
+    for (Py_ssize_t i = 0; i < asdl_seq_LEN(exprs); i++) {
         expr_ty expr = asdl_seq_GET(exprs, i);
         if (expr) {
             if (!validate_expr(state, expr, ctx))
@@ -955,8 +975,8 @@ validate_exprs(struct validator *state, asdl_expr_seq *exprs, expr_context_ty ct
 static int
 validate_patterns(struct validator *state, asdl_pattern_seq *patterns, int star_ok)
 {
-    Py_ssize_t i;
-    for (i = 0; i < asdl_seq_LEN(patterns); i++) {
+    assert(!PyErr_Occurred());
+    for (Py_ssize_t i = 0; i < asdl_seq_LEN(patterns); i++) {
         pattern_ty pattern = asdl_seq_GET(patterns, i);
         if (!validate_pattern(state, pattern, star_ok)) {
             return 0;
@@ -972,6 +992,7 @@ validate_patterns(struct validator *state, asdl_pattern_seq *patterns, int star_
 int
 _PyAST_Validate(mod_ty mod)
 {
+    assert(!PyErr_Occurred());
     int res = -1;
     struct validator state;
     PyThreadState *tstate;
diff --git a/Python/compile.c b/Python/compile.c
index f8d0197e9f0682..bf5e4a52482a4a 100644
--- a/Python/compile.c
+++ b/Python/compile.c
@@ -567,6 +567,7 @@ PyCodeObject *
 _PyAST_Compile(mod_ty mod, PyObject *filename, PyCompilerFlags *pflags,
                int optimize, PyArena *arena)
 {
+    assert(!PyErr_Occurred());
     struct compiler *c = new_compiler(mod, filename, pflags, optimize, arena);
     if (c == NULL) {
         return NULL;


Code review comments:
diff_hunk: @@ -0,0 +1 @@
+Fix two error handling bugs in ast.c's parsing of pattern matching statements.
path: Misc/NEWS.d/next/Core and Builtins/2023-05-14-18-56-54.gh-issue-104482.yaQsv8.rst
text: @user1:
s/two/three/ :)

diff_hunk: @@ -0,0 +1 @@
+Fix two error handling bugs in ast.c's parsing of pattern matching statements.
path: Misc/NEWS.d/next/Core and Builtins/2023-05-14-18-56-54.gh-issue-104482.yaQsv8.rst
text: @author:
```suggestion
Fix three error handling bugs in ast.c's parsing of pattern matching statements.
```