classification
Title: infinite loop when running inspect.unwrap over unittest.mock.call
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.7, Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Patrick Grafe, The Compiler, cjw296, michael.foord, ncoghlan, pstch, serhiy.storchaka, takluyver, yselivanov
Priority: normal Keywords: 3.5regression, patch

Created on 2015-11-01 23:50 by cjw296, last changed 2017-09-27 09:40 by ncoghlan. This issue is now closed.

Files
File name Uploaded Description Edit
blacklist-wrapped-in-mock-call.patch pstch, 2016-08-21 00:33 Patch blacklisting __wrapped__ as method name in `mock.call` review
Pull Requests
URL Status Linked Edit
PR 1717 merged takluyver, 2017-05-22 12:16
PR 3778 merged serhiy.storchaka, 2017-09-27 05:13
Messages (26)
msg253885 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2015-11-01 23:50
The following results in an infinite loop:

>>> from inspect import unwrap
>>> from unittest.mock import call
>>> unwrap(call)

This can occur when you use doctest.DocTestSuite to load doc tests from a module where unittest.mock.call has been imported.
msg253887 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2015-11-01 23:54
A naive solution is to chat unittest.mock._Call's __getattr__ to be as follows:

def __getattr__(self, attr):
    if attr == '__wrapped__':
        raise AttributeError('__wrapped__')
    if self.name is None:
        return _Call(name=attr, from_kall=False)
    name = '%s.%s' % (self.name, attr)
    return _Call(name=name, parent=self, from_kall=False)

...but I'm not sure that's the right thing to do.

inspect.unwrap is trying to catch this loop, but the implementation fails for cases where f.__wrapped__ is generative, as in the case of Mock and _Call.

I suspect both modules need a fix, but not sure what best to suggest.
msg253903 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2015-11-02 10:31
For mock I think your proposed fix is fine. call is particularly magic as merely importing it into modules can cause introspection problems like this (venusian is a third party library that has a similar issue), so working around these problems as they arise is worthwhile.
msg253954 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2015-11-03 00:47
Ah yes, I can see how Venusian would get confused.

How about making the check a more generic:

    if attr.startswith('__'):
        raise AttributeError(attr)

?

That said, why does call have a __getattr__? Where is that intended to be used?
msg253975 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2015-11-03 09:26
Have you read the docs for call? :-)

call.foo("foo") generates a call object representing a method call to the method foo. So you can do.

    m = Mock()
    m.foo("foo")

    self.assertEqual(m.mock_calls, call.foo("foo"))

(etc)

See also the call.call_list (I believe) method for assertions on chained calls.
msg253976 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2015-11-03 09:28
Preventing the use of "__" attributes would limit the usefulness of call with MagicMock and the use of magic methods. That might be an acceptable trade-off, but would break existing tests for people. (i.e. it's a backwards incompatible change.)
msg253986 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2015-11-03 12:56
Indeed, I guess Venusian will get confused, but not sure thats solvable as there will be obvious bugs indicating that call shouldn't be imported at module-level.

This does feel like the problem is actually with inspect.unwrap: there's evidence of an attempt to catch infinite loops like this and blow up with a ValueError, it just doesn't appear those checks are good enough. How many levels of unwrapping are reasonable? 1? 5? 100? It feels like the loop detection should be count, not set-of-ids based...

The other optional we be for _Call instances to be generative only in the right context, or only to a certain depth (10? how deep can one set of calls realistically be?)

I suspect both should probably happen... Thoughts?
msg254726 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2015-11-16 11:48
I'm happy to blacklist __wrapped__ in call. I don't see how call can detect that kind of introspection in the general case though. Limiting call depth would be one option, but any limit is going to be arbitrary and I don't "like" that as a solution.
msg254839 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2015-11-18 08:39
Cool, what needs to happen for __wrapped__ in to be blacklisted in call?

Separately, inspect.unwrap probably needs to use something less fragile than a set of ids to track whether it's stuck in a loop. What is the actual usecase for __wrapped__ how deeply nested can those realistically expected to be?
msg260458 - (view) Author: Florian Bruhin (The Compiler) Date: 2016-02-18 14:23
FWIW, this also affects pytest users: https://github.com/pytest-dev/pytest/issues/1217
msg273254 - (view) Author: Hugo Geoffroy (pstch) * Date: 2016-08-21 00:33
This patch blacklists `__wrapped__` (using the same form as the first comment, with a more explicit exception message) in `unittest.mock._Call.__getattr__`. 

I also documented the change and added a tests that checks `assertFalse(hasattr(call, '__wrapped__'))`.

I did not make the same change in the `Mock` class, as its instances are not usually set at module level (which is what triggers this bug in doctests, as they run `inspect.unwrap` on module attributes).

I'd like to note that this regression can be nasty for some CI systems : it makes the Python interpreter infinitely allocate memory (as it's not a recursion error) and crashes any host that doesn't limit virtual memory allocation.
msg273265 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-08-21 04:35
An alternative approach would be to change inspect.unwrap() to use getattr_static() rather than the usual getattr().

The downside of that would be potentially breaking true object proxies, like the 3rd party wrapt module and weakref.proxy.

However, what if inspect.signature implemented a limit on the number of recursive descents it permitted (e.g. based on sys.getrecursionlimit()), and then halted the descent, the same way it does if it finds a `__signature__` attribute on a wrapped object?

`inspect.unwraps` makes that relatively straightforward:

    unwrap_count = 0
    recursion_limit = sys.getrecursionlimit()
    def stop_unwrapping(wrapped):
        nonlocal unwrap_count
        unwrap_count += 1
        return unwrap_count >= recursion_limit
    innermost_function = inspect.unwrap(outer_function, stop=stop_unwrapping)

Alternatively, that hard limit on the recursive descent could be baked directly into the loop detection logic in inspect.unwrap (raising ValueError rather than returning the innermost function reached), as Chris suggested above. We'd then just need to check inspect.signature was handling that potential failure mode correctly.
msg273270 - (view) Author: Hugo Geoffroy (pstch) * Date: 2016-08-21 06:30
You are right, the fix would be better suited in `unwrap`. 

But, still, shouldn't any `__getattr__` implementation take care of not returning, for the `__wrapped__` attribute, a dynamic wrapper that provides the same attribute ? `__wrapped__` is commonly resolved to the innermost value without `__wrapped__`, which in this case never happens.

This would also avoid problems with introspection tools that resolve `__wrapped__` without the help of `unwrap` (before Python 3.4 IIRC).
msg273272 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-08-21 06:44
The infinite loop reported here is an inspect.signature bug - it's supposed to prevent infinite loops, but we didn't account for cases where "obj.__wrapped__" implicitly generates a fresh object every time.

unittest.mock.Mock is the only case of that in the standard library, but it's far from the only Python mocking library out there, and we should give a clear exception in such cases, rather than eating up all the memory on the machine.

This further means that while I agree the idea of blacklisting certain attributes from auto-generation in unittest.mock.Mock is a reasonable one, I also think you'll end up with a better design for *unittest.mock* by approaching that from the perspective of:

- having a way to prevent certain attributes being auto-generated is clearly useful (e.g. the "assert_*" methods, "__wrapped__")
- it's useful to have that default "blocked attribute" list be non-empty
- it may be useful to have that default list be configurable by tests and test frameworks without needing to update mock itself

That would make more sense as its own RFE, with a reference back to this bug as part of the motivation, rather than pursuing it as the *fix* for this bug.
msg273273 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-08-21 07:00
My current inclination is that the right fix here is to have a hard limit in inspect.unwrap() itself, on the basis of "we won't eat all the memory on your machine, even when other code produces an infinite chain of distinct __wrapped__ attributes" is a guarantee that function should aim to be offering.

Since the limit is arbitrary anyway, sys.getrecursionlimit() seems like a reasonable choice. The current id-based checked then just becomes a way to bail out early if there's an actual cycle in the wrapper chain, with the hard limit only being reached in the case of introspection on recursively generated attributes.

Looking at the current inspect.signature and pydoc handling, inspect.signature lets the ValueError escape, while pydoc intercepts it and handles it as "no signature information available".

The inspect.signature case seems reasonable, but in pydoc, it may be worth trying inspect.signature a second time, only with "follow_wrapped=False" set in the call.

(I'm somewhat regretting raising ValueError rather than RecursionError for the infinite loop detection case, but I'm not sure it's worth the effort to change it at this point - the main benefit from doing so in 3.6 would be to better enable the "don't follow the wrapper chain when it's broken" fallback in pydoc)
msg273333 - (view) Author: Hugo Geoffroy (pstch) * Date: 2016-08-22 04:40
Another argument for having the fix in `unwrap` rather than `signature` is that this bug does not actually seem to be called by `signature`, as the doctest module calls `unwrap` for "inspect.isroutine(inspect.unwrap(val))".

Also, this call does not even check for `ValueError`, which, if I'm not wrong, is something that should be corrected.

Maybe `unwrap` could be made recursive to make it respect recursion limits directly ? Otherwise, limiting the loop seems like a good idea.

(Temporarily, `from mock import call; call.__wrapped__ = None` seems to be a good workaround to prevent infinite memory allocation).
msg294142 - (view) Author: Thomas Kluyver (takluyver) * Date: 2017-05-22 12:17
This was just reported in IPython as well (https://github.com/ipython/ipython/issues/10578 ).

I've prepared a pull request based on Nick's proposal:
https://github.com/python/cpython/pull/1717
msg294168 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-05-22 18:25
> Since the limit is arbitrary anyway, sys.getrecursionlimit() seems like a reasonable choice. 

The recursion limit can be changed for whatever purposes and I'm not sure that it's a good idea that it will affect unwrap behaviour in such cases.

I'd suggest to pick a sufficiently large number like 500 and go with it.
msg294172 - (view) Author: Thomas Kluyver (takluyver) * Date: 2017-05-22 19:12
I could go either way on that. It's not hard to imagine it as a recursive algorithm, and using the recursion limit provides a simple configuration escape hatch if someone has a desperate need for something wrapped 3000 times for some strange reason. But it may also be somewhat surprising if someone sets a really high recursion limit and suddenly it behaves oddly.

I've made the PR with getrecursionlimit() for now, but I'm happy to change it if people prefer it the other way.
msg294215 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-05-23 03:27
New changeset f9169ce6b48c7cc7cc62d9eb5e4ee1ac7066d14b by Nick Coghlan (Thomas Kluyver) in branch 'master':
bpo-25532: Protect against infinite loops in inspect.unwrap() (#1717)
https://github.com/python/cpython/commit/f9169ce6b48c7cc7cc62d9eb5e4ee1ac7066d14b
msg294255 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-05-23 15:28
Ah, sorry - I merged the PR before seeing the discussion about the limit over here.

I'd be fine with replacing the sys.getrecursionlimit() call with a module level constant like "_UNWRAP_LIMIT = 500".

If someone desperately needed a higher limit for some reason, they could still poke around and change that from outside the module, even if doing so wasn't officially supported.

That wouldn't need a new NEWS entry, since it would just be an amendment to the existing patch.
msg294282 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-05-23 20:18
> I'd be fine with replacing the sys.getrecursionlimit() call with a module level constant like "_UNWRAP_LIMIT = 500".

TBH I'm OK either way.

`sys.getrecursionlimit()` is 1000 (at least on my machine), which might be a bit too much for this specific use-case.

It's also unlikely that someone will set recursion limit to less than 100 (or too many things would break), so we are probably safe here.

So I'm +0.5 on using _UNWRAP_LIMIT; if you feel the same way too we should do that, otherwise let's leave it as is.
msg296947 - (view) Author: Patrick Grafe (Patrick Grafe) Date: 2017-06-26 21:14
Is this ready to get backported to Python 3.5 and 3.6?  I see the tags on the issue, but I'm not clear on the process for actually backporting patches.
msg303104 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-27 06:34
New changeset 02c3cdcef84edd8c71e9fe189873dea216acfc1d by Serhiy Storchaka in branch '3.6':
[3.6] bpo-25532: Protect against infinite loops in inspect.unwrap() (GH-1717) (#3778)
https://github.com/python/cpython/commit/02c3cdcef84edd8c71e9fe189873dea216acfc1d
msg303106 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-27 06:44
Ah, sorry, I backported and merged the PR with the "needs backport to 3.6" label before seeing the discussion about the limit over here.
msg303117 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-09-27 09:40
Let's call it done with the current sys.getrecursionlimit() behaviour, and if that turns out to be problematic in practice, someone will hopefully file a new issue about it.
History
Date User Action Args
2018-03-24 01:26:53terry.reedylinkissue33120 superseder
2017-09-27 09:40:40ncoghlansetstatus: open -> closed
resolution: fixed
messages: + msg303117

stage: patch review -> resolved
2017-09-27 06:44:36serhiy.storchakasetmessages: + msg303106
2017-09-27 06:34:46serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg303104
2017-09-27 05:13:06serhiy.storchakasetstage: backport needed -> patch review
pull_requests: + pull_request3765
2017-06-26 21:14:50Patrick Grafesetnosy: + Patrick Grafe
messages: + msg296947
2017-05-23 20:18:16yselivanovsetmessages: + msg294282
2017-05-23 15:28:46ncoghlansetmessages: + msg294255
2017-05-23 03:29:41ncoghlansetstage: needs patch -> backport needed
versions: + Python 3.7
2017-05-23 03:27:54ncoghlansetmessages: + msg294215
2017-05-22 19:12:23takluyversetmessages: + msg294172
2017-05-22 18:25:36yselivanovsetmessages: + msg294168
2017-05-22 12:17:53takluyversetnosy: + takluyver
messages: + msg294142
2017-05-22 12:16:38takluyversetpull_requests: + pull_request1806
2016-08-22 04:40:38pstchsetmessages: + msg273333
2016-08-21 07:00:06ncoghlansetmessages: + msg273273
2016-08-21 06:44:17ncoghlansetmessages: + msg273272
2016-08-21 06:30:59pstchsetmessages: + msg273270
2016-08-21 04:35:08ncoghlansetmessages: + msg273265
2016-08-21 00:33:16pstchsetfiles: + blacklist-wrapped-in-mock-call.patch

nosy: + pstch
messages: + msg273254

keywords: + patch
2016-02-18 15:57:17berker.peksagsettype: behavior
stage: needs patch
2016-02-18 14:23:41The Compilersetnosy: + The Compiler
messages: + msg260458
2015-11-18 08:39:46cjw296setmessages: + msg254839
2015-11-16 11:48:34michael.foordsetmessages: + msg254726
2015-11-03 12:56:34cjw296setmessages: + msg253986
2015-11-03 09:28:31michael.foordsetmessages: + msg253976
2015-11-03 09:26:58michael.foordsetmessages: + msg253975
2015-11-03 00:47:21cjw296setmessages: + msg253954
2015-11-02 10:31:16michael.foordsetmessages: + msg253903
2015-11-01 23:54:21cjw296setmessages: + msg253887
2015-11-01 23:50:17cjw296create