bpo-32176: Set CO_NOFREE in the code object constructor #4675

ncoghlan · 2017-12-02T11:40:04Z

Previously, CO_NOFREE was set in the compiler, which meant
it could end up being set incorrectly when code objects
were created directly. Setting it in the constructor based
on freevars and cellvars ensures it is always accurate,
regardless of how the code object is defined.

https://bugs.python.org/issue32176

Previously, CO_NOFREE was set in the compiler, which meant it could end up being set incorrectly when code objects were created directly. Setting it in the constructor based on freevars and cellvars ensures it is always accurate, regardless of how the code object is defined.

serhiy-storchaka · 2017-12-02T13:49:54Z

Lib/test/test_code.py

+
+        # Ensure the implicit super() call actually works
+        obj = List([1,2,3])
+        self.assertEqual(obj[0], 1)


Does it print to stdout? stdout shouldn't be polluted by tests in non-verbose mode.

How do you know that the injected method actually works instead of the original list method? I suggest to transform the value in __getitem__. For example by calling str().

Done - replaced the print() with the use of an f-string as the getitem result.

serhiy-storchaka · 2017-12-02T13:50:09Z

Lib/test/test_code.py

+        self.assertFalse(function.__code__.co_flags & inspect.CO_NOFREE)
+
+        # Ensure the implicit super() call actually works
+        obj = List([1,2,3])


Add spaces after commas.

serhiy-storchaka · 2017-12-02T13:51:10Z

Lib/test/test_code.py

+            setattr(cls, name, FunctionType(code, globals(), name, defaults, closure))
+
+        class List(list):
+            def append(self, elem):


Are these methods related to the test?

Just copied from the original reproducer in the bug report - replaced with pass.

serhiy-storchaka · 2017-12-02T13:54:46Z

Lib/test/test_code.py

+        def new_code(c_or_f):
+            '''A new code object with a __class__ cell added to freevars'''
+            c = c_or_f.__code__ if isinstance(c_or_f, FunctionType) else c_or_f
+            return CodeType(


Seems there are no direct tests for CodeType. I think we need to add them (in different issue).

Yeah, that would probably be a good idea. I did a brief review for other unhandled edge cases in direct calls to the constructor and didn't see any, but I could easily have missed something.

serhiy-storchaka · 2017-12-02T13:56:05Z

Lib/test/test_code.py

+
+        def new_code(c_or_f):
+            '''A new code object with a __class__ cell added to freevars'''
+            c = c_or_f.__code__ if isinstance(c_or_f, FunctionType) else c_or_f


In this test new_code() is called only with the code object.

I refactored both this and add_foreign_method to only handle the cases needed for the test (the original more general versions came from the reported reproducer for the issue)

serhiy-storchaka · 2017-12-02T13:58:55Z

Lib/test/test_code.py

+        self.assertIs(class_ref, List)
+
+        # Ensure the code correctly indicates it accesses a free variable
+        self.assertFalse(function.__code__.co_flags & inspect.CO_NOFREE)


Add the second argument hex(function.__code__.co_flags).

- use a non-dunder name for the injected function - remove some code branches not actually needed for the test - transform the value to ensure the injected method is running - don't print anything as a side effect of running the test - print the code flags as hex if the test assertion fails

ncoghlan

Thanks for the review! All suggestions accepted, and I spotted a few other opportunities for cleanups while I was editing.

ncoghlan · 2017-12-02T23:43:21Z

Lib/test/test_code.py

+
+        # Ensure the implicit super() call actually works
+        obj = List([1,2,3])
+        self.assertEqual(obj[0], 1)


Done - replaced the print() with the use of an f-string as the getitem result.

ncoghlan · 2017-12-02T23:43:28Z

Lib/test/test_code.py

+        self.assertFalse(function.__code__.co_flags & inspect.CO_NOFREE)
+
+        # Ensure the implicit super() call actually works
+        obj = List([1,2,3])


ncoghlan · 2017-12-02T23:43:34Z

Lib/test/test_code.py

+        self.assertIs(class_ref, List)
+
+        # Ensure the code correctly indicates it accesses a free variable
+        self.assertFalse(function.__code__.co_flags & inspect.CO_NOFREE)


ncoghlan · 2017-12-02T23:44:02Z

Lib/test/test_code.py

+            setattr(cls, name, FunctionType(code, globals(), name, defaults, closure))
+
+        class List(list):
+            def append(self, elem):


Just copied from the original reproducer in the bug report - replaced with pass.

ncoghlan · 2017-12-02T23:45:27Z

Lib/test/test_code.py

+        def new_code(c_or_f):
+            '''A new code object with a __class__ cell added to freevars'''
+            c = c_or_f.__code__ if isinstance(c_or_f, FunctionType) else c_or_f
+            return CodeType(


Yeah, that would probably be a good idea. I did a brief review for other unhandled edge cases in direct calls to the constructor and didn't see any, but I could easily have missed something.

ncoghlan · 2017-12-02T23:46:24Z

Lib/test/test_code.py

+
+        def new_code(c_or_f):
+            '''A new code object with a __class__ cell added to freevars'''
+            c = c_or_f.__code__ if isinstance(c_or_f, FunctionType) else c_or_f


I refactored both this and add_foreign_method to only handle the cases needed for the test (the original more general versions came from the reported reproducer for the issue)

miss-islington · 2017-12-03T01:12:22Z

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

miss-islington · 2017-12-03T01:12:33Z

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

ncoghlan · 2017-12-03T01:12:34Z

@serhiy-storchaka Thanks for the review!

…GH-4675) Previously, CO_NOFREE was set in the compiler, which meant it could end up being set incorrectly when code objects were created directly. Setting it in the constructor based on freevars and cellvars ensures it is always accurate, regardless of how the code object is defined.. (cherry picked from commit 078f181)

bedevere-bot · 2017-12-03T01:22:07Z

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

the-knights-who-say-ni added the CLA signed label Dec 2, 2017

bedevere-bot added the awaiting merge label Dec 2, 2017

ncoghlan added the needs backport to 3.6 label Dec 2, 2017

Remove debugging line

62d5895

ncoghlan requested a review from serhiy-storchaka December 2, 2017 12:43

serhiy-storchaka reviewed Dec 2, 2017

View reviewed changes

ncoghlan commented Dec 2, 2017

View reviewed changes

serhiy-storchaka approved these changes Dec 3, 2017

View reviewed changes

ncoghlan merged commit 078f181 into python:master Dec 3, 2017

bedevere-bot removed the awaiting merge label Dec 3, 2017

bedevere-bot removed the needs backport to 3.6 label Dec 3, 2017

ncoghlan deleted the issue-32176-class-cell-injection branch March 30, 2018 07:52

Uh oh!

bpo-32176: Set CO_NOFREE in the code object constructor #4675

bpo-32176: Set CO_NOFREE in the code object constructor #4675

Uh oh!

Conversation

ncoghlan commented Dec 2, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Dec 3, 2017

Uh oh!

miss-islington commented Dec 3, 2017

Uh oh!

ncoghlan commented Dec 3, 2017

Uh oh!

bedevere-bot commented Dec 3, 2017

Uh oh!

Uh oh!

ncoghlan commented Dec 2, 2017 •

edited by bedevere-bot

Loading