bpo-31608: Fix a crash in methods of a subclass of _collections.deque with a bad new() #3788

orenmn · 2017-09-27T11:17:48Z

in _collectionsmodule.c: add a check whether __new__() returned a deque.

https://bugs.python.org/issue31608

serhiy-storchaka · 2017-10-09T20:44:20Z

Modules/_collectionsmodule.c

+                                       deque, old_deque->maxlen, NULL);
+    }
+    if (result != NULL &&
+        !PyObject_IsInstance(result, (PyObject *)&deque_type))


PyObject_IsInstance() can raise an exception.

Even if PyObject_IsInstance() returns 1, the result can have the binary layout different from dequeobject (because __instancecheck__), and the following concatenation or repeating will crash. Use PyObject_TypeCheck() instead.

serhiy-storchaka · 2017-10-09T20:47:44Z

Modules/_collectionsmodule.c

+        !PyObject_IsInstance(result, (PyObject *)&deque_type))
+    {
+        PyErr_Format(PyExc_TypeError,
+                     "%.200s.__new__() must return a deque, not %.200s",


Bad __new__() is not the only possible cause. The type can be an instance of a metaclass with overridden __call__.

bedevere-bot · 2017-10-09T20:48:11Z

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.

serhiy-storchaka · 2017-10-14T11:31:03Z

@rhettinger, please take a look at this patch.

rhettinger

Please add a test.

rhettinger · 2017-10-29T00:52:51Z

Modules/_collectionsmodule.c

+    else {
+        result = PyObject_CallFunction((PyObject *)(Py_TYPE(deque)), "Oi",
+                                       deque, old_deque->maxlen, NULL);
+    }


Please restore 541-548 to their former state and limit the patch to just the issue at hand.

rhettinger · 2017-10-29T00:53:53Z

Modules/_collectionsmodule.c

+        Py_DECREF(result);
+        return NULL;
+    }
+    return result;


This looks fine but it needs a test.

bedevere-bot · 2017-10-29T00:54:04Z

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 have made the requested changes; please review again. 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.

* added a test. * added a NEWS.d item.

serhiy-storchaka · 2017-10-29T10:30:18Z

Lib/test/test_deque.py

+            pass
+        d = X()
+        def bad___new__(cls, *args, **kwargs):
+            return 42


I think returning a list is more realistic example.

serhiy-storchaka · 2017-10-29T10:33:19Z

Lib/test/test_deque.py

+            return 42
+        X.__new__ = bad___new__
+        with self.assertRaises(TypeError):
+            d * 42  # shouldn't crash


What about concatenation?

The C changes are only to deque_copy(), so i thought that testing also the concatenation is redundant.
Should i add it to the test anyway?

serhiy-storchaka · 2017-10-29T10:33:51Z

Misc/NEWS.d/next/Library/2017-10-29-10-37-55.bpo-31608.wkp8Nw.rst

@@ -0,0 +1,2 @@
+Raise a ``TypeError`` instead of crashing, in specific cases where a
+``collections.deque`` subclass returns a non-deque. Patch by Oren Milman.


I would mention a copy.

I think that mentioning the copy won't help much to a user that isn't familiar with the internal implementation.
If you think that i should be more specific, i could mention that the crashes were in repetition and concatenation.
What do you think?

orenmn · 2017-10-29T15:29:18Z

I have made the requested changes; please review again.

bedevere-bot · 2017-10-29T15:29:19Z

Thanks for making the requested changes!

@serhiy-storchaka, @rhettinger: please review the changes made to this pull request.

miss-islington · 2018-09-11T18:46:58Z

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

…s.deque with a bad __new__(). (pythonGH-3788) (cherry picked from commit 24bd50b) Co-authored-by: Oren Milman <[email protected]>

bedevere-bot · 2018-09-11T18:47:08Z

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

miss-islington · 2018-09-11T18:47:10Z

Sorry, @orenmn and @benjaminp, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 24bd50bdcc97d65130c07d6cd26085fd06c3e972 3.6

miss-islington · 2018-09-11T18:47:15Z

Sorry, @orenmn and @benjaminp, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 24bd50bdcc97d65130c07d6cd26085fd06c3e972 2.7

…ections.deque with a bad __new__(). (GH-3788). (cherry picked from commit 24bd50b) Co-authored-by: Oren Milman <[email protected]>

…s.deque with a bad __new__(). (GH-3788) (cherry picked from commit 24bd50b) Co-authored-by: Oren Milman <[email protected]>

init commit

41cea9c

orenmn requested a review from rhettinger as a code owner September 27, 2017 11:17

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

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

serhiy-storchaka requested changes Oct 9, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Oct 9, 2017

orenmn added 2 commits October 14, 2017 13:17

Merge branch 'master' into bpo31608-fix-crash

57609f3

switched to use PyObject_TypeCheck, and fixed the error message.

2f34cb3

serhiy-storchaka approved these changes Oct 14, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Oct 14, 2017

rhettinger requested changes Oct 29, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Oct 29, 2017

orenmn added 2 commits October 29, 2017 09:58

Merge branch 'master' into bpo31608-fix-crash

a9c5806

* minimized the changes to the C code.

07bcc04

* added a test. * added a NEWS.d item.

serhiy-storchaka reviewed Oct 29, 2017

View reviewed changes

make the test more realistic, and also test concatenation.

2a690e8

bedevere-bot added awaiting change review and removed awaiting changes labels Oct 29, 2017

benjaminp added 2 commits September 11, 2018 11:24

improve NEWS

5c56c76

add space

ce1cb63

benjaminp added needs backport to 3.6 labels Sep 11, 2018

benjaminp added the needs backport to 3.7 label Sep 11, 2018

benjaminp merged commit 24bd50b into python:master Sep 11, 2018

bedevere-bot removed the awaiting change review label Sep 11, 2018

bedevere-bot removed the needs backport to 3.7 label Sep 11, 2018

miss-islington assigned benjaminp Sep 11, 2018

benjaminp pushed a commit that referenced this pull request Sep 11, 2018

[3.6] closes bpo-31608: Fix a crash in methods of a subclass of _coll…

a7c1f94

…ections.deque with a bad __new__(). (GH-3788). (cherry picked from commit 24bd50b) Co-authored-by: Oren Milman <[email protected]>

benjaminp pushed a commit that referenced this pull request Sep 11, 2018

[2.7] closes bpo-31608: Fix a crash in methods of a subclass of _coll…

b9ced63

…ections.deque with a bad __new__(). (GH-3788). (cherry picked from commit 24bd50b) Co-authored-by: Oren Milman <[email protected]>

miss-islington added a commit that referenced this pull request Sep 11, 2018

closes bpo-31608: Fix a crash in methods of a subclass of _collection…

536e45a

…s.deque with a bad __new__(). (GH-3788) (cherry picked from commit 24bd50b) Co-authored-by: Oren Milman <[email protected]>

		@@ -0,0 +1,2 @@
		Raise a ``TypeError`` instead of crashing, in specific cases where a
		``collections.deque`` subclass returns a non-deque. Patch by Oren Milman.

Uh oh!

bpo-31608: Fix a crash in methods of a subclass of _collections.deque with a bad __new__() #3788

bpo-31608: Fix a crash in methods of a subclass of _collections.deque with a bad __new__() #3788

Uh oh!

Conversation

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

bedevere-bot commented Oct 9, 2017

Uh oh!

serhiy-storchaka commented Oct 14, 2017

Uh oh!

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

bedevere-bot commented Oct 29, 2017

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!

orenmn Oct 29, 2017 • 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!

orenmn commented Oct 29, 2017

Uh oh!

bedevere-bot commented Oct 29, 2017

Uh oh!

miss-islington commented Sep 11, 2018

Uh oh!

bedevere-bot commented Sep 11, 2018

Uh oh!

miss-islington commented Sep 11, 2018

Uh oh!

miss-islington commented Sep 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bpo-31608: Fix a crash in methods of a subclass of _collections.deque with a bad new() #3788

bpo-31608: Fix a crash in methods of a subclass of _collections.deque with a bad new() #3788

orenmn commented Sep 27, 2017 •

edited by bedevere-bot

Loading

orenmn Oct 29, 2017 •

edited

Loading