Skip to content

bpo-25532: Protect against infinite loops in inspect.unwrap()#1717

Merged
ncoghlan merged 5 commits intopython:masterfrom
takluyver:unwrap-finite
May 23, 2017
Merged

bpo-25532: Protect against infinite loops in inspect.unwrap()#1717
ncoghlan merged 5 commits intopython:masterfrom
takluyver:unwrap-finite

Conversation

@takluyver
Copy link
Copy Markdown
Contributor

Some objects, such as unittest.mock.call, generate a new similar object every time you access obj.__wrapped__. This isn't caught by the memoise loop protection, because each object has a different ID. So a call like inspect.getsourcelines(unittest.mock.call) goes into an infinite loop, using ever more memory as it creates new objects.

This implements @ncoghlan's suggestion in issue 22532: if we've unwrapped something sys.getrecursionlimit() times, assume it's an infinite chain and give up. Things like getsourcelines() will fail, but an exception is better than using up all the memory.

IPython's inspection machinery already has a similar check, with an arbitrary limit of 100 unwraps. That returns the original object if the limit is exceeded, rather than throwing an error.
https://github.com/ipython/ipython/blob/6.0.0/IPython/core/oinspect.py#L279

@mention-bot
Copy link
Copy Markdown

@takluyver, thanks for your PR! By analyzing the history of the files in this pull request, we identified @akuchling, @1st1 and @zestyping to be potential reviewers.

@ncoghlan
Copy link
Copy Markdown
Contributor

+1 from me, so this just needs a Misc/NEWS entry noting the fix.

@takluyver
Copy link
Copy Markdown
Contributor Author

Added a NEWS entry :-)

@ncoghlan
Copy link
Copy Markdown
Contributor

Hah, that'll teach me to review patches late at night - I missed a more important omission, which is to add a test case that triggers the new check :)

@takluyver
Copy link
Copy Markdown
Contributor Author

I take it you don't want the test to use unittest.mock and go into an infinite loop if this is ever broken? ;-)

@takluyver
Copy link
Copy Markdown
Contributor Author

How does that test look?

Lib/inspect.py Outdated
@@ -506,10 +506,13 @@ def _is_wrapper(f):
return hasattr(f, '__wrapped__') and not stop(f)
f = func # remember the original func for error reporting
memo = {id(f)} # Memoise by id to tolerate non-hashable objects
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are here: we should switch to use a dictionary {id(o): o}. The same approach that deepcopy uses to ensure that objects stay alive long enough.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@pppery
Copy link
Copy Markdown

pppery commented May 22, 2017

Why can't you use len(memo) instead of creating a seperate unwrap_count variable?

@1st1
Copy link
Copy Markdown
Member

1st1 commented May 22, 2017

+1 to the len(memo) idea.

@ncoghlan ncoghlan merged commit f9169ce into python:master May 23, 2017
@ncoghlan
Copy link
Copy Markdown
Contributor

Thanks for the patch @takluyver, and thanks for the suggested improvements @1st1 and @pppery :)

@miss-islington
Copy link
Copy Markdown
Contributor

🐍🍒⛏🤖 Thanks @takluyver for the PR, and @ncoghlan for merging it 🌮🎉.I'm working now to backport this PR to: 3.6.

@miss-islington
Copy link
Copy Markdown
Contributor

Sorry, @takluyver and @ncoghlan, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.

serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this pull request Sep 27, 2017
…ythonGH-1717)

Some objects (like test mocks) auto-generate new objects on
attribute access, which can lead to an infinite loop in
inspect.unwrap().

Ensuring references are retained to otherwise temporary objects
and capping the size of the memo dict turns this case into a
conventional exception instead..
(cherry picked from commit f9169ce)
@bedevere-bot
Copy link
Copy Markdown

GH-3778 is a backport of this pull request to the 3.6 branch.

serhiy-storchaka added a commit that referenced this pull request Sep 27, 2017
…H-1717) (#3778)

Some objects (like test mocks) auto-generate new objects on
attribute access, which can lead to an infinite loop in
inspect.unwrap().

Ensuring references are retained to otherwise temporary objects
and capping the size of the memo dict turns this case into a
conventional exception instead..
(cherry picked from commit f9169ce)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants