Skip to content

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

Merged
pablogsal merged 3 commits into
python:mainfrom
isidentical:bpo-11105
Jun 3, 2021
Merged

bpo-11105: Do not crash when compiling recursive ASTs#20594
pablogsal merged 3 commits into
python:mainfrom
isidentical:bpo-11105

Conversation

@isidentical

@isidentical isidentical commented Jun 2, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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

isidentical commented May 9, 2021

Copy link
Copy Markdown
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

bedevere-bot commented May 9, 2021

Copy link
Copy Markdown

🤖 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

pablogsal commented Jun 2, 2021

Copy link
Copy Markdown
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

isidentical commented Jun 2, 2021

Copy link
Copy Markdown
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

pablogsal commented Jun 2, 2021

Copy link
Copy Markdown
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

miss-islington commented Jun 3, 2021

Copy link
Copy Markdown
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

miss-islington commented Jun 3, 2021

Copy link
Copy Markdown
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

bedevere-bot commented Jun 3, 2021

Copy link
Copy Markdown

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 <batuhan@python.org>
@pablogsal

pablogsal commented Jun 3, 2021

Copy link
Copy Markdown
Member

@isidentical Can you do the manual backport to 3.9?

@isidentical

isidentical commented Jun 3, 2021

Copy link
Copy Markdown
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 <batuhan@python.org>
@bedevere-bot

bedevere-bot commented Jun 3, 2021

Copy link
Copy Markdown

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 <batuhan@python.org>
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 <batuhan@python.org>
@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