bpo-42609: Check recursion depth in the AST validator and optimizer by serhiy-storchaka · Pull Request #23744 · python/cpython

serhiy-storchaka · 2020-12-12T12:31:33Z

https://bugs.python.org/issue42609

Python/ast.c

isidentical · 2020-12-14T22:24:02Z

Python/ast.c

+
+    /* Check that the recursion depth counting balanced correctly */
+    if (res && state.recursion_depth != starting_recursion_depth) {
+        PyErr_Format(PyExc_SystemError,
+            "AST validator recursion depth mismatch (before=%d, after=%d)",
+            starting_recursion_depth, state.recursion_depth);
+        return 0;
+    }


Do we really need to check this for every run? I feel like, it is more appropriate when activated only on debug builds.

Every run? No. But it costs nothing and helped to catch bugs.

Symtable checks balance even in case of failure (res == 0). But supporting it have larger cost, larger chance of making error and is more difficult to test, so I omitted it.

I am not proposing to remove the check altogether, but I couldn't see the reasoning to keep it on the release builds. We have similar checks all over the code (such as asserts() or blocks guarded with #if PY_DEBUG) which helps us to catch bugs on the debug builds, and doesn't change anything on the end user's side. Ofc most of them have a certain cost, so I was just asking whether it would be suitable to guard this with PY_DEBUG or not, but since you said the cost is nothing, guess we can just leave it as is.

iritkatriel · 2020-12-20T17:22:17Z

Include/compile.h

    int ff_features;
+
+    int recursion_depth;            /* current recursion depth */
+    int recursion_limit;            /* recursion limit */


Could you not use Py_EnterRecursiveCall here instead?

The code is copied from symtable.c. It uses different recursion limit than in Py_EnterRecursiveCall. If we decide to use Py_EnterRecursiveCall, it can be done consistency in symtable.c, ast.c and ast_opt.c. But this is a different issue, for now it is simpler to just duplicate some code three times.

ZackerySpytz · 2020-12-21T23:20:26Z

Lib/test/test_compile.py

        check_limit("a", ".b")
        check_limit("a", "[0]")
        check_limit("a", "*a")
+        #check_limit("if a: pass", "\nelif a: pass", mode="exec")


I don't think this commented-out code should be added.

It is still a bug not fixed by this PR.

ZackerySpytz · 2020-12-21T23:20:46Z

Misc/NEWS.d/next/Core and Builtins/2020-12-12-14-28-31.bpo-42609.Qcd54b.rst

@@ -0,0 +1,3 @@
+Prevented crashes in the AST validator and optimizer when compile some


compile -> compiling

ZackerySpytz · 2020-12-21T23:23:20Z

Python/ast_opt.c

+    starting_recursion_depth = (tstate->recursion_depth < INT_MAX / COMPILER_STACK_FRAME_SCALE) ?
+        tstate->recursion_depth * COMPILER_STACK_FRAME_SCALE : tstate->recursion_depth;
+    state->recursion_depth = starting_recursion_depth;
+    state->recursion_limit = (recursion_limit < INT_MAX / COMPILER_STACK_FRAME_SCALE) ?


The PEP 7 style guide states that lines should be limited to 79 characters.

It is copied from symtable.c and ast.c. It is better to keep code the same in three places.

ZackerySpytz · 2020-12-21T23:24:01Z

Lib/test/test_compile.py

+                details = "Compiling ({!r} + {!r} * {})".format(
+                            prefix, repeated, depth)


An f-string could be used.

Yes, but this code already was here.

ZackerySpytz · 2020-12-21T23:27:50Z

Include/compile.h

+    int recursion_depth;            /* current recursion depth */
+    int recursion_limit;            /* recursion limit */


C99 // comments can be used.

Yes, but it was just copied from symtable.c.

ZackerySpytz · 2020-12-21T23:28:10Z

Include/compile.h

 typedef struct {
    int optimize;
    int ff_features;
+


Suggested change

Empty line separates groups of fields.

ZackerySpytz · 2020-12-21T23:31:36Z

Python/ast_opt.c

+    PyThreadState *tstate;
+    int recursion_limit = Py_GetRecursionLimit();
+    int starting_recursion_depth;


These declarations can be moved down (C99).

github-actions · 2021-01-21T01:02:24Z

This PR is stale because it has been open for 30 days with no activity.

miss-islington · 2021-04-25T10:38:03Z

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

miss-islington · 2021-04-25T10:38:06Z

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker face87c94e67ad9c72b9a3724f112fd76c1002b9 3.9

miss-islington · 2021-04-25T10:38:08Z

Sorry @serhiy-storchaka, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker face87c94e67ad9c72b9a3724f112fd76c1002b9 3.8

…izer (pythonGH-23744). (cherry picked from commit face87c) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

bedevere-bot · 2021-04-26T16:29:49Z

GH-25634 is a backport of this pull request to the 3.9 branch.

bpo-42609: Check recursion depth in the AST validator and optimizer

8c5cb1e

serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.8 labels Dec 12, 2020

serhiy-storchaka requested a review from methane December 12, 2020 12:31

the-knights-who-say-ni added the CLA signed label Dec 12, 2020

bedevere-bot added the awaiting core review label Dec 12, 2020

isidentical reviewed Dec 14, 2020

View reviewed changes

serhiy-storchaka added 2 commits December 16, 2020 16:32

Check recursion depth in validate_constant().

8d3a7a2

Fix typo

364d0d2

iritkatriel reviewed Dec 20, 2020

View reviewed changes

ZackerySpytz reviewed Dec 21, 2020

View reviewed changes

github-actions bot added the stale Stale PR or inactive for long period of time. label Jan 21, 2021

serhiy-storchaka added 2 commits April 25, 2021 12:56

Merge branch 'master' into ast-validator-optimizer-recursion-error

e0e64e2

Add an explainating comment for crashing tests

8c53a01

serhiy-storchaka merged commit face87c into python:master Apr 25, 2021

bedevere-bot removed the awaiting core review label Apr 25, 2021

miss-islington assigned serhiy-storchaka Apr 25, 2021

serhiy-storchaka deleted the ast-validator-optimizer-recursion-error branch April 25, 2021 10:38

bedevere-bot removed the needs backport to 3.9 label Apr 26, 2021

serhiy-storchaka removed the stale Stale PR or inactive for long period of time. label Aug 31, 2021

Erik-Lamers1 mannequin mentioned this pull request May 21, 2022

Eval with too high string multiplication crashes newer Python versions #86775

Closed

serhiy-storchaka removed the needs backport to 3.8 label Sep 29, 2024

serhiy-storchaka removed their assignment Sep 29, 2024

		@@ -0,0 +1,3 @@
		Prevented crashes in the AST validator and optimizer when compile some

		details = "Compiling ({!r} + {!r} * {})".format(
		prefix, repeated, depth)

		int recursion_depth; /* current recursion depth */
		int recursion_limit; /* recursion limit */

Uh oh!

Conversation

serhiy-storchaka commented Dec 12, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 21, 2021

Uh oh!

miss-islington commented Apr 25, 2021

Uh oh!

miss-islington commented Apr 25, 2021

Uh oh!

miss-islington commented Apr 25, 2021

Uh oh!

bedevere-bot commented Apr 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

serhiy-storchaka commented Dec 12, 2020 •

edited by bedevere-bot

Loading