infinite loop when running inspect.unwrap over unittest.mock.call

[cjw296]

BPO	25532
Nosy	@ncoghlan, @cjw296, @voidspace, @takluyver, @serhiy-storchaka, @1st1, @The-Compiler, @pstch
PRs	python/cpython#1717 python/cpython#3778
Files	blacklist-wrapped-in-mock-call.patch: Patch blacklisting wrapped as method name in mock.call

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = created_at = labels = ['3.7', 'type-bug', 'library'] title = 'infinite loop when running inspect.unwrap over unittest.mock.call' updated_at = user = 'https://github.com/cjw296' ``` bugs.python.org fields: ```python activity = actor = 'ncoghlan' assignee = 'none' closed = True closed_date = closer = 'ncoghlan' components = ['Library (Lib)'] creation = creator = 'cjw296' dependencies = [] files = ['44178'] hgrepos = [] issue_num = 25532 keywords = ['patch', '3.5regression'] message_count = 26.0 messages = ['253885', '253887', '253903', '253954', '253975', '253976', '253986', '254726', '254839', '260458', '273254', '273265', '273270', '273272', '273273', '273333', '294142', '294168', '294172', '294215', '294255', '294282', '296947', '303104', '303106', '303117'] nosy_count = 9.0 nosy_names = ['ncoghlan', 'cjw296', 'michael.foord', 'takluyver', 'serhiy.storchaka', 'yselivanov', 'The Compiler', 'pstch', 'Patrick Grafe'] pr_nums = ['1717', '3778'] priority = 'normal' resolution = 'fixed' stage = 'resolved' status = 'closed' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue25532' versions = ['Python 3.5', 'Python 3.6', 'Python 3.7'] ```

[cjw296]

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.

[cjw296]

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.

[voidspace]

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.

[cjw296]

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?

[voidspace]

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.

[voidspace]

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.)

[cjw296]

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?

[voidspace]

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.

[cjw296]

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?

[b8c17aa0-8543-473e-baae-23d368fad117]

FWIW, this also affects pytest users: https://github.com/pytest-dev/pytest/issues/1217

[bef90765-97b4-481c-8867-198befeb5c7b]

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.

[ncoghlan]

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.

[bef90765-97b4-481c-8867-198befeb5c7b]

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).

[ncoghlan]

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.

[ncoghlan]

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)

[bef90765-97b4-481c-8867-198befeb5c7b]

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).

[cd34197f-d4a4-4e30-9fbe-454f267f097e]

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

[1st1]

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.

[cd34197f-d4a4-4e30-9fbe-454f267f097e]

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.

[ncoghlan]

New changeset f9169ce6b48c7cc7cc62d9eb5e4ee1ac7066d14b by Nick Coghlan (Thomas Kluyver) in branch 'master': bpo-25532: Protect against infinite loops in inspect.unwrap() (bpo-1717) https://github.com/python/cpython/commit/f9169ce6b48c7cc7cc62d9eb5e4ee1ac7066d14b

[ncoghlan]

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.

[1st1]

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.

[14f67dc1-74e1-411e-97f2-c006592bd8d5]

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.

[serhiy-storchaka]

New changeset 02c3cdcef84edd8c71e9fe189873dea216acfc1d by Serhiy Storchaka in branch '3.6': [3.6] bpo-25532: Protect against infinite loops in inspect.unwrap() (GH-1717) (bpo-3778) https://github.com/python/cpython/commit/02c3cdcef84edd8c71e9fe189873dea216acfc1d

[serhiy-storchaka]

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.

[ncoghlan]

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.