Skip to content

bpo-11105: Do not crash when compiling recursive ASTs #20594

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 3, 2021
Merged

bpo-11105: Do not crash when compiling recursive ASTs #20594

merged 3 commits into from
Jun 3, 2021

Conversation

isidentical
Copy link
Member

@isidentical isidentical commented Jun 2, 2020 •
edited
Loading

When compiling AST objects with direct or indirect reference cycles, the converter (python -> native) used to crash. Now it properly manages all the potential calls that might fall into a recursive state with Py_EnterRecursiveCall/Py_LeaveRecursiveCall.

https://bugs.python.org/issue11105

isidentical added 2 commits May 9, 2021 03:49
@isidentical
bpo-11105: Do not crash when compiling recursive AST
3935d3d 
When compiling an AST object with a direct / indirect reference
cycles, on the conversion phase because of exceeding amount of
calls, a segfault was raised. This patch adds recursion guards to
places for preventing user inputs to not to crash AST but instead
raise a RecursionError.
@isidentical
Add news entry
46635a4
@isidentical isidentical added the needs backport to 3.10 only security fixes label May 9, 2021
@isidentical
Copy link
Member Author

I didn't bother with benchmarking extensively though it seems like when compiling python AST objects (only on compile() route), it is now %2~ slower (debug build, with no pgo/lto). (python -m timeit -s 'import ast; source="foo(bar, baz + quux, eggs[what], lambda: spam ** 2),"*10000; tree=ast.parse(source)' 'compile(tree,"<test>","exec")')

@isidentical isidentical changed the title bpo-11105: Do not crash when compiling recursive AST bpo-11105: Do not crash when compiling recursive ASTs May 9, 2021
@isidentical isidentical added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 9, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @isidentical for commit 9f8c68f 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 9, 2021
@pablogsal
Copy link
Member

@isidentical could you run a quick benchmark to see if this has any effect on the general speed? I expect almost nothing, but is never bad to check

@isidentical
Copy link
Member Author

@isidentical could you run a quick benchmark to see if this has any effect on the general speed? I expect almost nothing, but is never bad to check

What do you mean by general speed? This is the code path taken when the python level AST objects (ast.AST) are getting serialized into C level ones (expr_ty etc). So it won't change anything in regular code execution since there is no intermediate step that happens on the python layer, and will only affect the compile(tree) calls (which I gave the benchmark results above). Are there any other particular ones that you are interested in?

@pablogsal
Copy link
Member

@isidentical could you run a quick benchmark to see if this has any effect on the general speed? I expect almost nothing, but is never bad to check

What do you mean by general speed? This is the code path taken when the python level AST objects (ast.AST) are getting serialized into C level ones (expr_ty etc). So it won't change anything in regular code execution since there is no intermediate step that happens on the python layer, and will only affect the compile(tree) calls (which I gave the benchmark results above). Are there any other particular ones that you are interested in?

Sorry, I was on my phone and I missed #20594 (comment) 🤦

That was what I was searching for :)

@pablogsal pablogsal merged commit f349124 into python:main Jun 3, 2021
@miss-islington
Copy link
Contributor

Thanks @isidentical for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.9.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @isidentical and @pablogsal, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker f3491242e41933aa9529add7102edb68b80a25e9 3.9

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jun 3, 2021
@bedevere-bot
Copy link

GH-26521 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 3, 2021
@isidentical @miss-islington
bpo-11105: Do not crash when compiling recursive ASTs (pythonGH-20594)
5b705f7 
When compiling an AST object with a direct / indirect reference
cycles, on the conversion phase because of exceeding amount of
calls, a segfault was raised. This patch adds recursion guards to
places for preventing user inputs to not to crash AST but instead
raise a RecursionError.
(cherry picked from commit f349124)

Co-authored-by: Batuhan Taskaya <[email protected]>
@pablogsal
Copy link
Member

@isidentical Can you do the manual backport to 3.9?

@isidentical
Copy link
Member Author

@isidentical Can you do the manual backport to 3.9?

Yeah, sure!

miss-islington added a commit that referenced this pull request Jun 3, 2021
@miss-islington @isidentical
bpo-11105: Do not crash when compiling recursive ASTs (GH-20594)
976598d 
When compiling an AST object with a direct / indirect reference
cycles, on the conversion phase because of exceeding amount of
calls, a segfault was raised. This patch adds recursion guards to
places for preventing user inputs to not to crash AST but instead
raise a RecursionError.
(cherry picked from commit f349124)

Co-authored-by: Batuhan Taskaya <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Jun 3, 2021
@bedevere-bot
Copy link

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

isidentical added a commit to isidentical/cpython that referenced this pull request Jun 3, 2021
@isidentical
[3.9] bpo-11105: Do not crash when compiling recursive ASTs (pythonGH…
23f88b5 
…-20594)

When compiling an AST object with a direct / indirect reference
cycles, on the conversion phase because of exceeding amount of
calls, a segfault was raised. This patch adds recursion guards to
places for preventing user inputs to not to crash AST but instead
raise a RecursionError..
(cherry picked from commit f349124)

Co-authored-by: Batuhan Taskaya <[email protected]>
pablogsal pushed a commit that referenced this pull request Jun 3, 2021
@isidentical
[3.9] bpo-11105: Do not crash when compiling recursive ASTs (GH-20594) (
de58b31 
GH-26522)

When compiling an AST object with a direct / indirect reference
cycles, on the conversion phase because of exceeding amount of
calls, a segfault was raised. This patch adds recursion guards to
places for preventing user inputs to not to crash AST but instead
raise a RecursionError..
(cherry picked from commit f349124)

Co-authored-by: Batuhan Taskaya <[email protected]>
@pablogsal pablogsal mentioned this pull request Jul 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pablogsal pablogsal pablogsal approved these changes

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Assignees

@pablogsal pablogsal

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@isidentical @bedevere-bot @pablogsal @miss-islington @ambv @the-knights-who-say-ni