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