bpo-42183: Fix a stack overflow error for asyncio Task or Future repr() #23020

asvetlov · 2020-10-29T09:22:08Z

The overflow occurs under some circumstances when a task or future
recursively returns itself.

https://bugs.python.org/issue42183

The overflow occures under some circumstances when a task or future recursively returns itself.

aeros

Other than a few minor comments, LGTM.

For comparison, I primarily referred to the implementation of reprlib.recursive_repr(). Based on my local testing, it addresses the issue of recursive reprs and fully prevents the stack overflow.

aeros · 2020-10-29T18:33:16Z

Lib/asyncio/base_futures.py

@@ -41,6 +42,9 @@ def format_cb(callback):
    return f'cb=[{cb}]'


+_repr_running = set()


Might be worth adding a brief comment here to clarify the purpose; e.g. "Used for guarding against recursive reprs from futures" or something along those lines. The name _repr_running isn't overly clear by itself without knowing the context of the internal set in reprlib.recursive_repr().

Sure, I'll add. Plus, I want to describe why reprlib.recursive_repr decorator is not applicable (mine first approach was just applying it).

I would say that adding _asyncio.Task.__module__ allows us to use reprlib.recursive_repr but it is a more invasive change.

aeros · 2020-10-29T18:42:12Z

Lib/test/test_asyncio/test_futures2.py

+        async def func():
+            return asyncio.all_tasks()
+
+        self.assertIn('...', repr(await asyncio.wait_for(func(), timeout=10)))


Should this not more strictly test result=...? I think they would be effectively the same in most cases, but during local testing of the PR branch, I noticed some issues with regards to the ellipses colliding/cutting off parts of the repr when using asyncio.gather():

>>> await asyncio.gather( ... asyncio.wait_for(func(), timeout=10), ... asyncio.wait_for(func(), timeout=10)) [{<Task finished name='Task-18' coro=<wait_for() done, defined at /home/aeros/repos/cpython/Lib/asyncio/tasks.py:419> result={<Task finishe...9> result=...>, <Task finishe...res.py:391]>}>, <Task finishe...res.py:391]>}>, <Task pending...tures.py:391]>, <Task finishe... result=...>}>}>, <Task finished name='Task-21' coro=<func() done, defined at <console>:1> result={<Task finishe...res.py:391]>}>, <Task finishe...esult=...>}>}>, <Task finishe...1> result=...>, <Task pending...tures.py:391]>}>, <Task finished name='Task-19' coro=<wait_for() done, defined at /home/aeros/repos/cpython/Lib/asyncio/tasks.py:419> result={<Task finishe...9> result=...>, <Task finishe...esult=...>}>}>, <Task finishe...res.py:391]>}>, <Task pending...tures.py:391]>}>, <Task pending name='Task-17' coro=<<module>() running at <console>:1> cb=[_chain_future.<locals>._call_set_state() at /home/aeros/repos/cpython/Lib/asyncio/futures.py:391]>, <Task finished name='Task-20' coro=<func() done, defined at <console>:1> result={<Task finishe... result=...>}>, <Task finishe...res.py:391]>}>, <Task finishe...res.py:391]>}>, <Task pending...tures.py:391]>, <Task finishe...1> result=...>}>}, {<Task finished name='Task-19' coro=<wait_for() done, defined at /home/aeros/repos/cpython/Lib/asyncio/tasks.py:419> result={<Task finishe...9> result=...>, <Task finishe...esult=...>}>}>, <Task finishe...res.py:391]>}>, <Task pending...tures.py:391]>}>, <Task finished name='Task-18' coro=<wait_for() done, defined at /home/aeros/repos/cpython/Lib/asyncio/tasks.py:419> result={<Task finishe...9> result=...>, <Task finishe...res.py:391]>}>, <Task finishe...res.py:391]>}>, <Task pending...tures.py:391]>, <Task finishe... result=...>}>}>, <Task finished name='Task-21' coro=<func() done, defined at <console>:1> result={<Task finishe...res.py:391]>}>, <Task finishe...esult=...>}>}>, <Task finishe...1> result=...>, <Task pending...tures.py:391]>}>, <Task pending name='Task-17' coro=<<module>() running at <console>:1> cb=[_chain_future.<locals>._call_set_state() at /home/aeros/repos/cpython/Lib/asyncio/futures.py:391]>}]

If it's a bit hard to read, this was the problematic line:

<Task finished name='Task-21' coro=<func() done, defined at <console>:1> result={<Task finishe...res.py:391]>}>, <Task finishe...esult=...>

That might just be an issue with asyncio.gather() reprs though, so I'm not sure exactly what the fix would be.

The main thing that the test check is the code doesn't raise RecursionError.
Probably I should mention it in a comment for the call.

I'm open to suggestions but not I don't see a better test check.
A comparison of the whole repr() string is weak.
It will be broken on any repr() change, e.g. providing info about an additional task's internal state.

I agree that the formatting is not always tidy. From my understanding, it is the result of two strategies: preventing recursion calls and limiting the repr string size.
Maybe we can improve this but I consider it as a separate issue.

My idea was to use self.assertIn('result=...', repr(await asyncio.wait_for(func(), timeout=10))) rather than matchiing the whole repr() string, but I agree that the repr formatting is more of a separate issue (and lower priority than what the PR mainly addresses).

aeros · 2020-10-29T18:45:55Z

Lib/test/test_asyncio/test_futures2.py

@@ -0,0 +1,15 @@
+# IsolatedAsyncioTestCase based tests


Is the motivation here to start a new set of tests using IsolatedAsyncioTestCase for simplification? If so, is there going to be some effort to gradually migrate the existing test_futures or is it just for new ones?

Yes, the gradual migration of the asyncio test suite would be nice.
I believe IsolatedAsyncioTestCase based tests are much more readable.
On another hand, don't fix if not broken :) Plus tests rewriting takes time with very little effort.
For a new test, I prefer the new approach, definitely.

Misc/NEWS.d/next/Library/2020-10-29-11-17-35.bpo-42183.50ZcIi.rst

Co-authored-by: Kyle Stanley <aeros167@gmail.com>

aeros · 2020-10-30T09:23:08Z

With the 3.7 branch now being security only, I believe the needs backport to 3.7 label should probably be removed.

asvetlov · 2020-10-30T09:43:46Z

I'm not sure if RecursionError a security-critical bug or not.
My plan is: prepare PR for 3.7 and let @ned-deily decide to accept it or reject.

asvetlov · 2020-10-30T09:58:15Z

Comments are added

ned-deily · 2020-10-31T20:27:01Z

I'm not sure if RecursionError a security-critical bug or not.
My plan is: prepare PR for 3.7 and let @ned-deily decide to accept it or reject.

It doesn't sound like something that a third-party could use to cause a DoS but, If you and @1st1 think it should be fixed, I would likely not object to backporting to 3.7.

miss-islington · 2020-11-10T13:58:35Z

Thanks @asvetlov for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8, 3.9.
🐍🍒⛏🤖

…() (pythonGH-23020) The overflow occurs under some circumstances when a task or future recursively returns itself. Co-authored-by: Kyle Stanley <aeros167@gmail.com> (cherry picked from commit 42d873c) Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>

bedevere-bot · 2020-11-10T13:58:48Z

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

…() (pythonGH-23020) The overflow occurs under some circumstances when a task or future recursively returns itself. Co-authored-by: Kyle Stanley <aeros167@gmail.com> (cherry picked from commit 42d873c) Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>

bedevere-bot · 2020-11-10T13:58:56Z

GH-23222 is a backport of this pull request to the 3.8 branch.

bedevere-bot · 2020-11-10T13:59:04Z

GH-23223 is a backport of this pull request to the 3.7 branch.

…() (pythonGH-23020) The overflow occurs under some circumstances when a task or future recursively returns itself. Co-authored-by: Kyle Stanley <aeros167@gmail.com> (cherry picked from commit 42d873c) Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>

…() (GH-23020) The overflow occurs under some circumstances when a task or future recursively returns itself. Co-authored-by: Kyle Stanley <aeros167@gmail.com> (cherry picked from commit 42d873c) Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>

…() (pythonGH-23020) The overflow occurs under some circumstances when a task or future recursively returns itself. Co-authored-by: Kyle Stanley <aeros167@gmail.com>

Fix a stack overflow error for asyncio Task or Future repr().

ab49120

The overflow occures under some circumstances when a task or future recursively returns itself.

asvetlov requested a review from 1st1 as a code owner October 29, 2020 09:22

the-knights-who-say-ni added the CLA signed label Oct 29, 2020

bedevere-bot added the awaiting core review label Oct 29, 2020

asvetlov added needs backport to 3.7 needs backport to 3.9 only security fixes labels Oct 29, 2020

Update NEWS

cff4fdf

asvetlov requested a review from aeros October 29, 2020 09:24

aeros approved these changes Oct 29, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Oct 29, 2020

Update Misc/NEWS.d/next/Library/2020-10-29-11-17-35.bpo-42183.50ZcIi.rst

1bd6ca5

Co-authored-by: Kyle Stanley <aeros167@gmail.com>

asvetlov added 2 commits October 30, 2020 11:55

Add a note

d9f44ca

Add comment

6845689

asvetlov self-assigned this Oct 30, 2020

asvetlov merged commit 42d873c into python:master Nov 10, 2020

bedevere-bot removed the awaiting merge label Nov 10, 2020

asvetlov deleted the fix-recursive-repr-crash branch November 10, 2020 13:58

bedevere-bot removed the needs backport to 3.9 only security fixes label Nov 10, 2020

bedevere-bot removed the needs backport to 3.8 label Nov 10, 2020

bedevere-bot removed the needs backport to 3.7 label Nov 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-42183: Fix a stack overflow error for asyncio Task or Future repr() #23020

bpo-42183: Fix a stack overflow error for asyncio Task or Future repr() #23020

asvetlov commented Oct 29, 2020 •

edited by bedevere-bot

Loading

aeros left a comment •

edited

Loading

aeros Oct 29, 2020

asvetlov Oct 30, 2020

asvetlov Oct 30, 2020

aeros Oct 29, 2020

asvetlov Oct 30, 2020

aeros Oct 30, 2020 •

edited

Loading

aeros Oct 29, 2020

asvetlov Oct 30, 2020

aeros commented Oct 30, 2020 •

edited

Loading

asvetlov commented Oct 30, 2020

asvetlov commented Oct 30, 2020

ned-deily commented Oct 31, 2020

miss-islington commented Nov 10, 2020

bedevere-bot commented Nov 10, 2020

bedevere-bot commented Nov 10, 2020

bedevere-bot commented Nov 10, 2020

		@@ -41,6 +42,9 @@ def format_cb(callback):
		return f'cb=[{cb}]'


		_repr_running = set()

bpo-42183: Fix a stack overflow error for asyncio Task or Future repr() #23020

bpo-42183: Fix a stack overflow error for asyncio Task or Future repr() #23020

Conversation

asvetlov commented Oct 29, 2020 • edited by bedevere-bot Loading

aeros left a comment • edited Loading

Choose a reason for hiding this comment

aeros Oct 29, 2020

Choose a reason for hiding this comment

asvetlov Oct 30, 2020

Choose a reason for hiding this comment

asvetlov Oct 30, 2020

Choose a reason for hiding this comment

aeros Oct 29, 2020

Choose a reason for hiding this comment

asvetlov Oct 30, 2020

Choose a reason for hiding this comment

aeros Oct 30, 2020 • edited Loading

Choose a reason for hiding this comment

aeros Oct 29, 2020

Choose a reason for hiding this comment

asvetlov Oct 30, 2020

Choose a reason for hiding this comment

aeros commented Oct 30, 2020 • edited Loading

asvetlov commented Oct 30, 2020

asvetlov commented Oct 30, 2020

ned-deily commented Oct 31, 2020

miss-islington commented Nov 10, 2020

bedevere-bot commented Nov 10, 2020

bedevere-bot commented Nov 10, 2020

bedevere-bot commented Nov 10, 2020

asvetlov commented Oct 29, 2020 •

edited by bedevere-bot

Loading

aeros left a comment •

edited

Loading

aeros Oct 30, 2020 •

edited

Loading

aeros commented Oct 30, 2020 •

edited

Loading