ignore property errors when parsing fixure factories #2235

bluetech · 2017-02-05T22:05:25Z

Previously, properties such as in the added test case were triggered
during collection, possibly executing unintended code. Let's skip them
instead.

We should probably skip all custom descriptors, however I am not sure
how to do this cleanly, so for now just skip properties.

Fixes #2234.

coveralls · 2017-02-05T22:35:41Z

Coverage increased (+0.002%) to 92.835% when pulling 0138d0b on bluetech:dont-execute-properties into ccf9877 on pytest-dev:master.

The-Compiler · 2017-02-06T06:47:50Z

I wonder if this should go to the features branch - while it can be seen as a bugfix, it also seems to me like there's a possibility of breaking something which worked before with it.

The-Compiler · 2017-02-06T06:48:44Z

_pytest/fixtures.py

@@ -1068,6 +1068,9 @@ def parsefactories(self, node_or_obj, nodeid=NOTSET, unittest=False):
        self._holderobjseen.add(holderobj)
        autousenames = []
        for name in dir(holderobj):
+            # Skip properties before the following getattr executes them.
+            if isinstance(getattr(type(holderobj), name, None), property):


hmm, maybe this should use safe_getattr from _pytest.compat? Probably a getattr on a type will never raise something, but you never know with metaclasses and stuff I guess 😆

It is technically possible to raise any exception from __getattr__ implemented on metaclass. Eg:

>>> getattr(fields.Fields.a, 'a', None) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/usr/local/lib/python2.7/dist-packages/fields/__init__.py", line 301, in __getattr__ raise TypeError("Field %r is already specified as required." % name) TypeError: Field 'a' is already specified as required.

Perhaps a better way to look at the object is to iterate on holderobj.__dict__ (getattr will involve the descriptor protocol, even if it's a class).

the real problem is, that avoiding the descriptor protocol correctly is tricky

Hehe. By far this is my „favourite” python quirk:

>>> class Desc(object): ... def __get__(self, inst, owner): ... print 'Surprise!', inst, owner ... >>> class Foo(object): ... d = Desc() ... >>> Foo.d Surprise! None <class '__main__.Foo'>

I will switch from getattr(type(holderobj), name, None) to type(holderobj).__dict__.get(name), which is hopefully safer. Here I assume that type(X) has a __dict__ for all X. I think this is true - even if I try something like this:

>>> class FooMeta(type): ... __slots__ = () ... >>> class Foo(metaclass=FooMeta): ... pass

it still has a __dict__:

>>> type(Foo()) <class '__main__.Foo'> >>> type(Foo()).__dict__ mappingproxy({...stuff...})

(Indeed the third argument to type(...) must be a dict, None or other types are not accepted)

bluetech · 2017-02-06T10:01:12Z

I wonder if this should go to the features branch - while it can be seen as a bugfix, it also seems to me like there's a possibility of breaking something which worked before with it.

I agree there is some slight breakage potential here. If you want me to change the target branch, let me know (the maintainers can also edit the PR).

The-Compiler · 2017-02-06T10:02:33Z

Let's wait what others think - no strong feelings either way, but history shows it's usually better to err on the side of caution 😉

RonnyPfannschmidt

btw, with just the switch to safe_getattr i believe its fine to go in to a patch release

RonnyPfannschmidt · 2017-02-06T10:36:18Z

_pytest/fixtures.py

@@ -1068,6 +1068,9 @@ def parsefactories(self, node_or_obj, nodeid=NOTSET, unittest=False):
        self._holderobjseen.add(holderobj)
        autousenames = []
        for name in dir(holderobj):
+            # Skip properties before the following getattr executes them.
+            if isinstance(type(holderobj).__dict__.get(name), property):


this really should use safe_getattr instead
else this protects only against one specific case of error while staying open for dozens of others

What sort of cases are you thinking of? As I understand it, this line cannot throw, since:

type(holderobj) always has a __dict__ for all holderobj, and that __dict__ is indeed a dict (as I mentioned in a comment above).

Accessing the __dict__ and checking isinstance can't trigger any Python voodoo.

On the other hand, using safe_getattr will still trigger voodoo, though it will continue if it throws.

That said, having properties on the class type is really getting farfetched, so using safe_getattr (or just getattr) is fine by me as well. Just let me know if you still want me to change it.

@bluetech basically any implementation of the descriptor protocol that does something at the class level can trigger

on the other hand i also think its fine to trigger those objects, since well, python mantra is "consenting adults" - we dont need to protect people from their own traps too much

Just an idea: what if instead of explicitly skipping properties we just make sure we only accept function objects? After all fixtures can only be created using functions.

@nicoddemus well, thats more strict in a good way ^^ i like the idea good thought :)

coveralls · 2017-02-06T10:38:27Z

Coverage increased (+0.002%) to 92.835% when pulling 34718c6 on bluetech:dont-execute-properties into ccf9877 on pytest-dev:master.

KeyboardInterrupt is a subclass of BaseException, but not of Exception. Hence if we remove this except, KeyboardInterrupts will still be raised so the behavior stays the same.

Descriptors (e.g. properties) such as in the added test case are triggered during collection, executing arbitrary code which can raise. Previously, such exceptions were propagated and failed the collection. Now these exceptions are caught and the corresponding attributes are silently ignored. A better solution would be to completely skip access to all custom descriptors, such that the offending code doesn't even trigger. However I think this requires manually going through the instance and all of its MRO for each and every attribute checking if it might be a proper fixture before accessing it. So I took the easy route here. In other words, putting something like this in your test class is still a bad idea...: @Property def innocent(self): os.system('rm -rf /') Fixes pytest-dev#2234.

bluetech · 2017-02-07T12:34:41Z

Thanks for the comment. I've updated it again to just replace the original getattr to safe_getattr. I rewrote the commit message, please have a look.

This patch fixes my immediate problem; hopefully someone will fix the problem properly, but doing so is a bit over my head as a passerby (haven't even had a chance to use pytest fixtures yet).

I also added another tiny change I noticed, you can drop that commit if you want.

RonnyPfannschmidt · 2017-02-07T12:43:38Z

yup, the current code is fine to merge - additional enhancements can be done as part of a new pr

coveralls · 2017-02-07T12:44:02Z

Coverage increased (+0.01%) to 92.843% when pulling 3a0a0c2 on bluetech:dont-execute-properties into ccf9877 on pytest-dev:master.

nicoddemus · 2017-02-08T01:41:30Z

yup, the current code is fine to merge - additional enhancements can be done as part of a new pr

Agreed, thanks again @bluetech!

The-Compiler reviewed Feb 6, 2017

View reviewed changes

bluetech force-pushed the dont-execute-properties branch from 0138d0b to 34718c6 Compare February 6, 2017 10:25

RonnyPfannschmidt approved these changes Feb 6, 2017

View reviewed changes

bluetech added 2 commits February 7, 2017 14:12

Remove an unneeded except KeyboardInterrupt

87fb689

KeyboardInterrupt is a subclass of BaseException, but not of Exception. Hence if we remove this except, KeyboardInterrupts will still be raised so the behavior stays the same.

bluetech force-pushed the dont-execute-properties branch from 34718c6 to 3a0a0c2 Compare February 7, 2017 12:30

RonnyPfannschmidt changed the title ~~Don't execute class properties when collecting Python test classes~~ ignore property errors when parsing fixure factories Feb 7, 2017

nicoddemus merged commit a4fb971 into pytest-dev:master Feb 8, 2017

bluetech deleted the dont-execute-properties branch February 28, 2020 12:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ignore property errors when parsing fixure factories #2235

ignore property errors when parsing fixure factories #2235

bluetech commented Feb 5, 2017

coveralls commented Feb 5, 2017

The-Compiler commented Feb 6, 2017

The-Compiler Feb 6, 2017

ionelmc Feb 6, 2017

ionelmc Feb 6, 2017

RonnyPfannschmidt Feb 6, 2017

ionelmc Feb 6, 2017 •

edited

Loading

bluetech Feb 6, 2017

bluetech Feb 6, 2017

bluetech commented Feb 6, 2017

The-Compiler commented Feb 6, 2017

RonnyPfannschmidt left a comment

RonnyPfannschmidt Feb 6, 2017

bluetech Feb 6, 2017

RonnyPfannschmidt Feb 6, 2017

nicoddemus Feb 7, 2017

RonnyPfannschmidt Feb 7, 2017

coveralls commented Feb 6, 2017

bluetech commented Feb 7, 2017

RonnyPfannschmidt commented Feb 7, 2017

coveralls commented Feb 7, 2017

nicoddemus commented Feb 8, 2017

ignore property errors when parsing fixure factories #2235

ignore property errors when parsing fixure factories #2235

Conversation

bluetech commented Feb 5, 2017

coveralls commented Feb 5, 2017

The-Compiler commented Feb 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ionelmc Feb 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluetech commented Feb 6, 2017

The-Compiler commented Feb 6, 2017

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 6, 2017

bluetech commented Feb 7, 2017

RonnyPfannschmidt commented Feb 7, 2017

coveralls commented Feb 7, 2017

nicoddemus commented Feb 8, 2017

ionelmc Feb 6, 2017 •

edited

Loading