bpo-32999: Fix ABC.__subclasscheck__ crash by methane · Pull Request #6002 · python/cpython

methane · 2018-03-06T11:31:46Z

C implementation of ABC.__subclasscheck__(cls, subclass) crashed when subclass is not type object.

This commit makes C implementation more similar to Python implementation.

https://bugs.python.org/issue32999

zhangyangyu · 2018-03-06T11:44:37Z

+     *     return True
+     */
+    _Py_IDENTIFIER(__mro__);
+    if (_PyObject_LookupAttrId(subclass, &PyId___mro__, &mro) < 0) {


mro is leaked.

As well as impl.

ilevkivskyi

Thanks! Just two minor comments

ilevkivskyi · 2018-03-06T11:48:11Z

            self.assertIsInstance(42, A)
            self.assertIsInstance(42, (A,))

+        def test_issubclass(self):


I would rather make the test name more specific, or add few more corner cases here.

ilevkivskyi · 2018-03-06T11:49:28Z

+     */
+    _Py_IDENTIFIER(__mro__);
+    if (_PyObject_LookupAttrId(subclass, &PyId___mro__, &mro) < 0) {
+        return NULL;


Maybe this should be goto end; or similar? (for clean-up)

zhangyangyu · 2018-03-06T12:41:53Z


 end:
+    Py_XDECREF(mro);
    Py_XDECREF(impl);


Looks to me impl can't be NULL here.

izbyshev · 2018-03-06T13:13:29Z

+                __mro__ = 42  # __mro__ is not tuple
+
+            with self.assertRaises(TypeError):
+                self.assertTrue(issubclass(C(), A))


self.assertTrue is not needed.

ilevkivskyi

LGTM now.

zhangyangyu · 2018-03-06T14:43:51Z

-            if (_add_to_weak_set(&impl->_abc_cache, subclass) < 0) {
+        for (pos = 0; pos < PyTuple_GET_SIZE(mro); pos++) {
+            PyObject *mro_item = PyTuple_GET_ITEM(mro, pos);
+            if (mro_item == NULL) {


Is this check needed? If it is a must, we need to set an exception in this branch, otherwise we will return NULL while no exception set.

1st1 · 2018-03-06T16:32:15Z

 static int
 compute_abstract_methods(PyObject *self)
 {
+    _Py_IDENTIFIER(__abstractmethods__);


Why don't we define these identifiers at the top level?

I prefer local variable when it's used only in one function.
If you prefer top level, I'll revert unnecessary changes.

I usually move them to the top level as it's hard to track if a variable like this is used in one or in many functions. Really up to you though, I'm OK about this change either way.

This reverts commit e15fc35.

zhangyangyu

LGTM. +1 about the unittest name could be more specific, but it's just a preference.

methane · 2018-03-07T05:35:58Z

I'm bad at English, including vocabulary. Would you propose better name, instead of just saying "specific"?

zhangyangyu · 2018-03-07T05:44:13Z

I mean something like test_issubclass_bad_arguments since the test only tests for exception cases.

miss-islington · 2018-03-07T07:27:03Z

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

(cherry picked from commit fc7df0e) Co-authored-by: INADA Naoki <methane@users.noreply.github.com>

bedevere-bot · 2018-03-07T07:28:12Z

GH-6014 is a backport of this pull request to the 3.7 branch.

(cherry picked from commit fc7df0e) Co-authored-by: INADA Naoki <methane@users.noreply.github.com>

bpo-33018 (pythonGH-5944) fixed bpo-32999 too. So fc7df0e is not required anymore. Revert it except test case.

bpo-33018 (pythonGH-5944) fixed bpo-32999 too. So fc7df0e is not required anymore. Revert it except test case. (cherry picked from commit f757b72) Co-authored-by: INADA Naoki <methane@users.noreply.github.com>

bpo-33018 (GH-5944) fixed bpo-32999 too. So fc7df0e is not required anymore. Revert it except test case.

bpo-33018 (GH-5944) fixed bpo-32999 too. So fc7df0e is not required anymore. Revert it except test case. (cherry picked from commit f757b72) Co-authored-by: INADA Naoki <methane@users.noreply.github.com>

bpo-33018 (pythonGH-5944) fixed bpo-32999 too. So fc7df0e is not required anymore. Revert it except test case.

methane added 4 commits March 6, 2018 20:12

Move identifiers to function local

e15fc35

add test

868fa3a

fix the bug

97ec4e7

Add NEWS

59998e7

the-knights-who-say-ni added the CLA signed label Mar 6, 2018

bedevere-bot added the awaiting merge label Mar 6, 2018

methane requested review from 1st1 and ilevkivskyi March 6, 2018 11:31

methane added type-bug An unexpected behavior, bug, or error needs backport to 3.7 labels Mar 6, 2018

zhangyangyu reviewed Mar 6, 2018

View reviewed changes

ilevkivskyi reviewed Mar 6, 2018

View reviewed changes

Fix leak

b901e1e

zhangyangyu reviewed Mar 6, 2018

View reviewed changes

methane added 2 commits March 6, 2018 21:52

More corner cases

c06fcae

impl must not be NULL

7767ef7

izbyshev reviewed Mar 6, 2018

View reviewed changes

oops

e2aa5ca

ilevkivskyi approved these changes Mar 6, 2018

View reviewed changes

zhangyangyu reviewed Mar 6, 2018

View reviewed changes

Remove redundant NULL check

23f555f

1st1 reviewed Mar 6, 2018

View reviewed changes

methane added 2 commits March 7, 2018 02:09

Revert "Move identifiers to function local"

8bcb74c

This reverts commit e15fc35.

Move __mro__ identifier to top of the file

697abd2

zhangyangyu approved these changes Mar 7, 2018

View reviewed changes

rename test func

fa4af5f

methane merged commit fc7df0e into python:master Mar 7, 2018

bedevere-bot removed the awaiting merge label Mar 7, 2018

methane deleted the fix-abc-issubclass-bpo32999 branch March 7, 2018 07:27

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 7, 2018

bpo-32999: Fix ABC.__subclasscheck__ crash (pythonGH-6002)

96ef3c1

(cherry picked from commit fc7df0e) Co-authored-by: INADA Naoki <methane@users.noreply.github.com>

bedevere-bot removed the needs backport to 3.7 label Mar 7, 2018

miss-islington added a commit that referenced this pull request Mar 7, 2018

bpo-32999: Fix ABC.__subclasscheck__ crash (GH-6002)

d824b4e

(cherry picked from commit fc7df0e) Co-authored-by: INADA Naoki <methane@users.noreply.github.com>

methane added a commit to methane/cpython that referenced this pull request Mar 22, 2018

bpo-32999: Revert pythonGH-6002 (fc7df0e)

cd7583e

bpo-33018 (pythonGH-5944) fixed bpo-32999 too. So fc7df0e is not required anymore. Revert it except test case.

methane added a commit that referenced this pull request Mar 22, 2018

bpo-32999: Revert GH-6002 (fc7df0e) (GH-6189)

f757b72

bpo-33018 (GH-5944) fixed bpo-32999 too. So fc7df0e is not required anymore. Revert it except test case.

jo2y pushed a commit to jo2y/cpython that referenced this pull request Mar 23, 2018

bpo-32999: Fix ABC.__subclasscheck__ crash (pythonGH-6002)

79536ea

jo2y pushed a commit to jo2y/cpython that referenced this pull request Mar 23, 2018

bpo-32999: Revert pythonGH-6002 (fc7df0e) (pythonGH-6189)

fc8d6e5

bpo-33018 (pythonGH-5944) fixed bpo-32999 too. So fc7df0e is not required anymore. Revert it except test case.

Uh oh!

Conversation

methane commented Mar 6, 2018 • 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!

ilevkivskyi 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!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

zhangyangyu Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

zhangyangyu left a comment

Choose a reason for hiding this comment

Uh oh!

methane commented Mar 7, 2018

Uh oh!

zhangyangyu commented Mar 7, 2018

Uh oh!

miss-islington commented Mar 7, 2018

Uh oh!

bedevere-bot commented Mar 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

methane commented Mar 6, 2018 •

edited by bedevere-bot

Loading

zhangyangyu Mar 6, 2018 •

edited

Loading