bpo-42609: Check recursion depth in the AST validator and optimizer #23744
Conversation
|
|
||
| /* 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; | ||
| } |
isidentical
Dec 14, 2020
Member
Do we really need to check this for every run? I feel like, it is more appropriate when activated only on debug builds.
serhiy-storchaka
Dec 16, 2020
Author
Member
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.
isidentical
Dec 16, 2020
Member
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.
| @@ -87,6 +87,9 @@ PyAPI_FUNC(int) PyCompile_OpcodeStackEffectWithJump(int opcode, int oparg, int j | |||
| typedef struct { | |||
| int optimize; | |||
| int ff_features; | |||
|
|
|||
| int recursion_depth; /* current recursion depth */ | |||
| int recursion_limit; /* recursion limit */ | |||
serhiy-storchaka
Apr 25, 2021
Author
Member
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.
|
|
||
| check_limit("a", "()") | ||
| check_limit("a", ".b") | ||
| check_limit("a", "[0]") | ||
| check_limit("a", "*a") | ||
| #check_limit("if a: pass", "\nelif a: pass", mode="exec") |
| @@ -0,0 +1,3 @@ | |||
| Prevented crashes in the AST validator and optimizer when compile some | |||
| 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) ? |
ZackerySpytz
Dec 21, 2020
Contributor
The PEP 7 style guide states that lines should be limited to 79 characters.
serhiy-storchaka
Apr 25, 2021
Author
Member
It is copied from symtable.c and ast.c. It is better to keep code the same in three places.
| details = "Compiling ({!r} + {!r} * {})".format( | ||
| prefix, repeated, depth) |
| int recursion_depth; /* current recursion depth */ | ||
| int recursion_limit; /* recursion limit */ |
| @@ -87,6 +87,9 @@ PyAPI_FUNC(int) PyCompile_OpcodeStackEffectWithJump(int opcode, int oparg, int j | |||
| typedef struct { | |||
| int optimize; | |||
| int ff_features; | |||
|
|
|||
| PyThreadState *tstate; | ||
| int recursion_limit = Py_GetRecursionLimit(); | ||
| int starting_recursion_depth; |
|
This PR is stale because it has been open for 30 days with no activity. |
face87c
into
python:master
|
Thanks @serhiy-storchaka for the PR |
|
Sorry, @serhiy-storchaka, I could not cleanly backport this to |
|
Sorry @serhiy-storchaka, I had trouble checking out the |
…izer (pythonGH-23744). (cherry picked from commit face87c) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
|
GH-25634 is a backport of this pull request to the 3.9 branch. |
https://bugs.python.org/issue42609