bpo-31588: Prevent raising a SystemError in class creation in case of a metaclass with a bad prepare() method #3764

orenmn · 2017-09-26T11:15:43Z

in bltinmodule.c: add a check whether __prepare__() returned a mapping.
in test_types.py: add tests to verify that the SystemError is no more.

https://bugs.python.org/issue31588

serhiy-storchaka · 2017-09-26T12:52:15Z

Lib/test/test_types.py

+        # __prepare__() must return a mapping.
+        class BadMeta(type):
+            def __prepare__(*args):
+                pass


Perhaps it would look better if add explicit return None. __prepare__() must return a value.

serhiy-storchaka · 2017-09-26T13:17:43Z

Python/bltinmodule.c

+    if (!PyMapping_Check(ns)) {
+        PyErr_Format(PyExc_TypeError,
+                     "%.200s.__prepare__() must return a mapping, not %.200s",
+                     isclass ? ((PyTypeObject *)meta)->tp_name : "<metaclass>",


Why this code is as it is?

I thought that printing the name of the problematic metaclass would be helpful to the user.
Now ISTM that I should have checked the __name__ attribute.
Do you think I should add that? Or should we just not include the name of the metaclass in the error message?

Why not use ((PyTypeObject *)meta)->tp_name in all cases?

IIUC, if meta is not a type object, then ((PyTypeObject *)meta)->tp_name might contain arbitrary data, according to the type of meta, which might cause a crash (as indeed happens when running my second test).

In what circumstances meta is not a type object?

This code is an example:

class BadMetaclass: def __prepare__(*args): pass class Bar(metaclass=BadMetaclass()): pass

I don't know about usecases for that (though I am certainly not a metaclasses expert), but IIUC, the C code explicitly handles such cases.

Maybe use meta->ob_type->tp_name in this case?

When metaclass isn't a type, it can be an arbitrary callable, so I think it's reasonable to just hardcode in <metaclass> for that case.

serhiy-storchaka · 2017-09-26T13:20:19Z

Misc/NEWS.d/next/Core and Builtins/2017-09-26-13-03-16.bpo-31588.wT9Iy7.rst

@@ -0,0 +1,2 @@
+Prevent raising a SystemError in class creation in case of a metaclass with


It is not clear what is the new behavior. Is a class created successfully?

Document that a TypeError is now raised instead of a SystemError.

serhiy-storchaka · 2017-09-27T09:37:39Z

Python/bltinmodule.c

+    if (!PyMapping_Check(ns)) {
+        PyErr_Format(PyExc_TypeError,
+                     "%.200s.__prepare__() must return a mapping, not %.200s",
+                     isclass ? ((PyTypeObject *)meta)->tp_name : "<metaclass>",


Maybe use meta->ob_type->tp_name in this case?

serhiy-storchaka · 2017-09-27T09:46:17Z

Lib/test/test_types.py

+    def test_bad___prepare__(self):
+        # __prepare__() must return a mapping.
+        class BadMeta(type):
+            def __prepare__(*args):


__prepare__ is called as a class method here. Decorate it with classmethod.

ncoghlan

The actual patch looks good to me, but I think the test case needs some refinement to make sure we're actually getting the desired behaviour.

ncoghlan · 2017-09-27T10:07:23Z

Lib/test/test_types.py

+        class BadMeta(type):
+            def __prepare__(*args):
+                return None
+        with self.assertRaises(TypeError):


This should use self.assertRaisesRegex to check we're getting the error message we expect rather than a different TypeError ( like "NoneType is not subscriptable")

ncoghlan · 2017-09-27T10:08:52Z

Misc/NEWS.d/next/Core and Builtins/2017-09-26-13-03-16.bpo-31588.wT9Iy7.rst

@@ -0,0 +1,2 @@
+Raise a `TypeError` instead of `SystemError` in case class creation failed due


I can't reproduce the reported SystemError myself - I get an unhelpfully cryptic error message, but it's a TypeError instance.

I got the same message you got when I compiled in release mode. this is because checks that are in debug mode only, as I mentioned in https://bugs.python.org/issue31588#msg303122.

So without the patch we have a TypeError in release mode and a SystemError in debug mode.
I would change the phrasing of the NEWS.d item accordingly.

ncoghlan · 2017-09-27T10:10:00Z

Python/bltinmodule.c

+    if (!PyMapping_Check(ns)) {
+        PyErr_Format(PyExc_TypeError,
+                     "%.200s.__prepare__() must return a mapping, not %.200s",
+                     isclass ? ((PyTypeObject *)meta)->tp_name : "<metaclass>",


When metaclass isn't a type, it can be an arbitrary callable, so I think it's reasonable to just hardcode in <metaclass> for that case.

bedevere-bot · 2017-09-27T10:10:48Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

orenmn · 2017-09-27T12:03:01Z

Misc/NEWS.d/next/Core and Builtins/2017-09-26-13-03-16.bpo-31588.wT9Iy7.rst

@@ -0,0 +1,2 @@
+Raise a `TypeError` with a helpful error message in case class creation failed
+due to a metaclass with a bad ``__prepare__()`` method. Patch by Oren Milman.


Should I be more specific here?
i.e. it could be:
... due to a metaclass whose __prepare__() method returned a non-mapping object.

I think your current wording is fine.

orenmn · 2017-09-27T12:19:36Z

I didn't expect the Spanish Inquisition!

bedevere-bot · 2017-09-27T12:19:37Z

Nobody expects the Spanish Inquisition!

@ncoghlan: please review the changes made to this pull request.

ncoghlan · 2017-09-27T13:24:43Z

Misc/NEWS.d/next/Core and Builtins/2017-09-26-13-03-16.bpo-31588.wT9Iy7.rst

@@ -0,0 +1,2 @@
+Raise a `TypeError` with a helpful error message in case class creation failed
+due to a metaclass with a bad ``__prepare__()`` method. Patch by Oren Milman.


I think your current wording is fine.

miss-islington · 2017-09-27T14:05:23Z

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

bedevere-bot · 2017-09-27T14:05:46Z

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

…onGH-3764) Class execution requires that __prepare__() methods return a proper execution namespace. Check for that immediately after calling __prepare__(), rather than passing it through to the code execution machinery and potentially triggering SystemError (in debug builds) or a cryptic TypeError (in release builds). Patch by Oren Milman. (cherry picked from commit 5837d04)

orenmn added 4 commits September 26, 2017 10:41

init commit

58df081

added a NEWS.d item, and added a test

bff6f25

cosmetic improvements to the test.

6c7ce6b

improve a comment.

bf6f425

the-knights-who-say-ni added the CLA signed label Sep 26, 2017

bedevere-bot added the awaiting review label Sep 26, 2017

serhiy-storchaka reviewed Sep 26, 2017

View reviewed changes

serhiy-storchaka requested a review from ncoghlan September 26, 2017 13:31

improve the test and the NEWS.d item

1571f3b

bedevere-bot added awaiting core review and removed awaiting review labels Sep 26, 2017

serhiy-storchaka reviewed Sep 27, 2017

View reviewed changes

ncoghlan requested changes Sep 27, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Sep 27, 2017

orenmn added 2 commits September 27, 2017 14:40

improve the tests

1462735

fix the NEWS.d item

7f70a1e

orenmn commented Sep 27, 2017

View reviewed changes

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 27, 2017

ncoghlan approved these changes Sep 27, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Sep 27, 2017

Minor grammar fix

dcd5227

ncoghlan merged commit 5837d04 into python:master Sep 27, 2017

ncoghlan added the needs backport to 3.6 label Sep 27, 2017

bedevere-bot removed the needs backport to 3.6 label Sep 27, 2017

ncoghlan removed the awaiting merge label Sep 27, 2017

		@@ -0,0 +1,2 @@
		Prevent raising a SystemError in class creation in case of a metaclass with

		@@ -0,0 +1,2 @@
		Raise a `TypeError` instead of `SystemError` in case class creation failed due

		@@ -0,0 +1,2 @@
		Raise a `TypeError` with a helpful error message in case class creation failed
		due to a metaclass with a bad ``__prepare__()`` method. Patch by Oren Milman.

Uh oh!

bpo-31588: Prevent raising a SystemError in class creation in case of a metaclass with a bad __prepare__() method #3764

bpo-31588: Prevent raising a SystemError in class creation in case of a metaclass with a bad __prepare__() method #3764

Uh oh!

Conversation

orenmn commented Sep 26, 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!

bedevere-bot commented Sep 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orenmn commented Sep 27, 2017

Uh oh!

bedevere-bot commented Sep 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Sep 27, 2017

Uh oh!

bedevere-bot commented Sep 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bpo-31588: Prevent raising a SystemError in class creation in case of a metaclass with a bad prepare() method #3764

bpo-31588: Prevent raising a SystemError in class creation in case of a metaclass with a bad prepare() method #3764

orenmn commented Sep 26, 2017 •

edited by bedevere-bot

Loading