Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Dec 12, 2020

Python/ast.c Show resolved Hide resolved

/* 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;
}
Comment on lines +637 to +644

This comment has been minimized.

@isidentical

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.

This comment has been minimized.

@serhiy-storchaka

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.

This comment has been minimized.

@isidentical

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 */

This comment has been minimized.

@iritkatriel

iritkatriel Dec 20, 2020
Member

Could you not use Py_EnterRecursiveCall here instead?

This comment has been minimized.

@serhiy-storchaka

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")

This comment has been minimized.

@ZackerySpytz

ZackerySpytz Dec 21, 2020
Contributor

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

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Apr 25, 2021
Author Member

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

This comment has been minimized.

@ZackerySpytz

ZackerySpytz Dec 21, 2020
Contributor

compile -> compiling

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Apr 25, 2021
Author Member

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) ?
Comment on lines +784 to +787

This comment has been minimized.

@ZackerySpytz

ZackerySpytz Dec 21, 2020
Contributor

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

This comment has been minimized.

@serhiy-storchaka

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)
Comment on lines +554 to +555

This comment has been minimized.

@ZackerySpytz

ZackerySpytz Dec 21, 2020
Contributor

An f-string could be used.

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Apr 25, 2021
Author Member

Yes, but this code already was here.

int recursion_depth; /* current recursion depth */
int recursion_limit; /* recursion limit */
Comment on lines 91 to 92

This comment has been minimized.

@ZackerySpytz

ZackerySpytz Dec 21, 2020
Contributor

C99 // comments can be used.

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Apr 25, 2021
Author Member

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;

This comment has been minimized.

@ZackerySpytz

ZackerySpytz Dec 21, 2020
Contributor

Suggested change

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Apr 25, 2021
Author Member

Empty line separates groups of fields.

PyThreadState *tstate;
int recursion_limit = Py_GetRecursionLimit();
int starting_recursion_depth;
Comment on lines +774 to +776

This comment has been minimized.

@ZackerySpytz

ZackerySpytz Dec 21, 2020
Contributor

These declarations can be moved down (C99).

@github-actions
Copy link

@github-actions github-actions bot commented Jan 21, 2021

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

@github-actions github-actions bot added the stale label Jan 21, 2021
@serhiy-storchaka serhiy-storchaka merged commit face87c into python:master Apr 25, 2021
12 checks passed
12 checks passed
Docs
Details
Check for source changes
Details
Check if generated files are up to date
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Ubuntu SSL tests with OpenSSL ${{ matrix.openssl_ver }}
Details
Azure Pipelines PR #20210425.14 succeeded
Details
Travis CI - Pull Request Build Passed
Details
bedevere/issue-number Issue number 42609 found
Details
bedevere/news News entry found in Misc/NEWS.d
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Apr 25, 2021

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

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Apr 25, 2021

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

@serhiy-storchaka serhiy-storchaka deleted the serhiy-storchaka:ast-validator-optimizer-recursion-error branch Apr 25, 2021
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Apr 25, 2021

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

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Apr 26, 2021
…izer (pythonGH-23744).

(cherry picked from commit face87c)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Apr 26, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment