[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Nick Coghlan]

Nick Coghlan  added the comment:

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.

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:


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


--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
pull_requests: +3765
stage: backport needed -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Patrick Grafe]

Patrick Grafe added the comment:

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.

--
nosy: +Patrick Grafe

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Yury Selivanov]

Yury Selivanov added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Nick Coghlan]

Changes by Nick Coghlan :


--
stage: needs patch -> backport needed
versions: +Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Nick Coghlan]

Nick Coghlan added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Thomas Kluyver]

Thomas Kluyver added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Yury Selivanov]

Yury Selivanov added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Thomas Kluyver]

Thomas Kluyver added the comment:

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

--
nosy: +takluyver

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Thomas Kluyver]

Changes by Thomas Kluyver :


--
pull_requests: +1806

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Hugo Geoffroy]

Hugo Geoffroy added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Nick Coghlan]

Nick Coghlan added the comment:

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)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Hugo Geoffroy]

Hugo Geoffroy added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Hugo Geoffroy]

Hugo Geoffroy added the comment:

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.

--
keywords: +patch
nosy: +pstch
Added file: 
http://bugs.python.org/file44178/blacklist-wrapped-in-mock-call.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Berker Peksag]

Changes by Berker Peksag :


--
stage:  -> needs patch
type:  -> behavior

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Florian Bruhin]

Florian Bruhin added the comment:

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

--
nosy: +The Compiler

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Chris Withers]

Chris Withers added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Michael Foord]

Michael Foord added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Michael Foord]

Michael Foord added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Michael Foord]

Michael Foord added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Chris Withers]

Chris Withers added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Michael Foord]

Michael Foord added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Chris Withers]

Chris Withers added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Chris Withers]

New submission from Chris Withers:

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.

--
components: Library (Lib)
keywords: 3.5regression
messages: 253885
nosy: cjw296, michael.foord, ncoghlan, yselivanov
priority: normal
severity: normal
status: open
title: infinite loop when running inspect.unwrap over unittest.mock.call
versions: Python 3.5, Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25532] infinite loop when running inspect.unwrap over unittest.mock.call

[Chris Withers]

Chris Withers added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com