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.
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.
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.
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 */ | |||
iritkatriel
Dec 20, 2020
Member
Could you not use Py_EnterRecursiveCall here instead?
Could you not use Py_EnterRecursiveCall here instead?
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.
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") |
ZackerySpytz
Dec 21, 2020
Contributor
I don't think this commented-out code should be added.
I don't think this commented-out code should be added.
serhiy-storchaka
Apr 25, 2021
Author
Member
It is still a bug not fixed by this PR.
It is still a bug not fixed by this PR.
| @@ -0,0 +1,3 @@ | |||
| Prevented crashes in the AST validator and optimizer when compile some | |||
ZackerySpytz
Dec 21, 2020
Contributor
compile -> compiling
compile -> compiling
serhiy-storchaka
Apr 25, 2021
Author
Member
Thanks.
Thanks.
| 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.
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.
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) |
ZackerySpytz
Dec 21, 2020
Contributor
An f-string could be used.
An f-string could be used.
serhiy-storchaka
Apr 25, 2021
Author
Member
Yes, but this code already was here.
Yes, but this code already was here.
| int recursion_depth; /* current recursion depth */ | ||
| int recursion_limit; /* recursion limit */ |
ZackerySpytz
Dec 21, 2020
Contributor
C99 // comments can be used.
C99 // comments can be used.
serhiy-storchaka
Apr 25, 2021
Author
Member
Yes, but it was just copied from symtable.c.
Yes, but it was just copied from symtable.c.
| @@ -87,6 +87,9 @@ PyAPI_FUNC(int) PyCompile_OpcodeStackEffectWithJump(int opcode, int oparg, int j | |||
| typedef struct { | |||
| int optimize; | |||
| int ff_features; | |||
|
|
|||
ZackerySpytz
Dec 21, 2020
Contributor
Suggested change
serhiy-storchaka
Apr 25, 2021
Author
Member
Empty line separates groups of fields.
Empty line separates groups of fields.
| PyThreadState *tstate; | ||
| int recursion_limit = Py_GetRecursionLimit(); | ||
| int starting_recursion_depth; |
ZackerySpytz
Dec 21, 2020
Contributor
These declarations can be moved down (C99).
These declarations can be moved down (C99).
|
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 <[email protected]>
|
GH-25634 is a backport of this pull request to the 3.9 branch. |


https://bugs.python.org/issue42609