bpo-41654: Fix deallocator of MemoryError to account for subclasses #22020

pablogsal · 2020-08-30T19:40:31Z

When allocating MemoryError classes, there is some logic to use
pre-allocated instances in a freelist only if the type that is being
allocated is not a subclass of MemoryError. Unfortunately in the
destructor this logic is not present so the freelist is altered even
with subclasses of MemoryError.

https://bugs.python.org/issue41654

Objects/exceptions.c

Lib/test/test_exceptions.py

Objects/exceptions.c

Lib/test/test_exceptions.py

When allocating MemoryError classes, there is some logic to use pre-allocated instances in a freelist only if the type that is being allocated is not a subclass of MemoryError. Unfortunately in the destructor this logic is not present so the freelist is altered even with subclasses of MemoryError.

Lib/test/test_exceptions.py

vstinner · 2020-09-01T09:30:05Z

Objects/exceptions.c

+        return Py_TYPE(self)->tp_free((PyObject *)self);
+    }
+
+    _PyObject_GC_UNTRACK(self);


This is a bugfix. Once this PR is merged, you may propose a PR to fix it in 3.9 and 3.8 as well. I'm surprised that _PyObject_GC_UNTRACK() doesn't crash (assert failure) on subclasses.

This PR will be backported to all supported branches....do you want a separate PR for them?

"This PR will be backported to all supported branches" oh ok, in this case, there is no need for a separated change ;-)

This bug has been here since forever! 😨

Lib/test/test_exceptions.py

vstinner

LGTM. Thanks for the fix @pablogsal!

I tested manually bug.py script attached to https://bugs.python.org/issue41654 : it crashs without the fix, it pass with the fix. I tested Python built in debug mode.

shihai1991 · 2020-09-01T17:20:23Z

Objects/exceptions.c

-    if (type != (PyTypeObject *) PyExc_MemoryError)
+    /* If this is a subclass of MemoryError, don't use the freelist
+     * and just return a fresh object */
+    if (type != (PyTypeObject *) PyExc_MemoryError) {


nitpick: Can you update this line same like line 2323 after you updated this line ;)

nitpick: Can you update this line same like line 2323 after you updated this line ;)

Here it does not make sense because we don't have an instance yet.

oh, sorry~. Forget my comment.

shihai1991

LGTM.

shihai1991 · 2020-09-01T18:02:48Z

Objects/exceptions.c

-    if (type != (PyTypeObject *) PyExc_MemoryError)
+    /* If this is a subclass of MemoryError, don't use the freelist
+     * and just return a fresh object */
+    if (type != (PyTypeObject *) PyExc_MemoryError) {


oh, sorry~. Forget my comment.

…asses

miss-islington · 2020-09-01T18:39:49Z

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7, 3.8, 3.9.
🐍🍒⛏🤖

miss-islington · 2020-09-01T18:39:54Z

Sorry @pablogsal, I had trouble checking out the 3.9 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 9b648a95ccb4c3b14f1e87158f5c9f5dbb2f62c0 3.9

miss-islington · 2020-09-01T18:39:59Z

Sorry, @pablogsal, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 9b648a95ccb4c3b14f1e87158f5c9f5dbb2f62c0 3.8

miss-islington · 2020-09-01T18:40:01Z

Sorry @pablogsal, I had trouble checking out the 3.7 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 9b648a95ccb4c3b14f1e87158f5c9f5dbb2f62c0 3.7

miss-islington · 2020-09-01T18:40:07Z

Sorry, @pablogsal, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 9b648a95ccb4c3b14f1e87158f5c9f5dbb2f62c0 3.6

vstinner · 2020-09-01T18:59:24Z

@pablogsal: I don't think that this change is related to security. Why do you want to backport it to 3.6 and 3.7? https://bugs.python.org/issue41654 type is "crash", not "security".

pablogsal · 2020-09-01T19:00:56Z

@pablogsal: I don't think that this change is related to security. Why do you want to backport it to 3.6 and 3.7? https://bugs.python.org/issue41654 type is "crash", not "security".

I normally qualify segfaults/buffer overflow as security as they can be maliciously used, but it may be a stretch. Let's backport to 3.8 and 3.9 only.

…sses (pythonGH-22020) When allocating MemoryError classes, there is some logic to use pre-allocated instances in a freelist only if the type that is being allocated is not a subclass of MemoryError. Unfortunately in the destructor this logic is not present so the freelist is altered even with subclasses of MemoryError.. (cherry picked from commit 9b648a9) Co-authored-by: Pablo Galindo <[email protected]>

bedevere-bot · 2020-09-01T19:06:50Z

GH-22045 is a backport of this pull request to the 3.9 branch.

bedevere-bot · 2020-09-01T19:07:11Z

GH-22046 is a backport of this pull request to the 3.8 branch.

…subclasses (pythonGH-22020) When allocating MemoryError classes, there is some logic to use pre-allocated instances in a freelist only if the type that is being allocated is not a subclass of MemoryError. Unfortunately in the destructor this logic is not present so the freelist is altered even with subclasses of MemoryError.. (cherry picked from commit 9b648a9) Co-authored-by: Pablo Galindo <[email protected]>. (cherry picked from commit 87e91ae) Co-authored-by: Pablo Galindo <[email protected]>

…subclasses (GH-22020) (GH-22046) When allocating MemoryError classes, there is some logic to use pre-allocated instances in a freelist only if the type that is being allocated is not a subclass of MemoryError. Unfortunately in the destructor this logic is not present so the freelist is altered even with subclasses of MemoryError.. (cherry picked from commit 9b648a9) Co-authored-by: Pablo Galindo <[email protected]>. (cherry picked from commit 87e91ae) Co-authored-by: Pablo Galindo <[email protected]>

…sses (GH-22020) (GH-22045) When allocating MemoryError classes, there is some logic to use pre-allocated instances in a freelist only if the type that is being allocated is not a subclass of MemoryError. Unfortunately in the destructor this logic is not present so the freelist is altered even with subclasses of MemoryError.. (cherry picked from commit 9b648a9) Co-authored-by: Pablo Galindo <[email protected]>

…ythonGH-22020) When allocating MemoryError classes, there is some logic to use pre-allocated instances in a freelist only if the type that is being allocated is not a subclass of MemoryError. Unfortunately in the destructor this logic is not present so the freelist is altered even with subclasses of MemoryError.

pablogsal added needs backport to 3.6 needs backport to 3.9 only security fixes labels Aug 30, 2020

the-knights-who-say-ni added the CLA signed label Aug 30, 2020

bedevere-bot added the awaiting core review label Aug 30, 2020

pablogsal force-pushed the bpo-41654 branch from 590a46d to f2db4f8 Compare August 30, 2020 19:41

pablogsal requested a review from vstinner August 30, 2020 19:41

pablogsal force-pushed the bpo-41654 branch from f2db4f8 to 10774a0 Compare August 30, 2020 19:44

shihai1991 reviewed Aug 31, 2020

View reviewed changes

Objects/exceptions.c Outdated Show resolved Hide resolved

vstinner reviewed Aug 31, 2020

View reviewed changes

Lib/test/test_exceptions.py Show resolved Hide resolved

Objects/exceptions.c Outdated Show resolved Hide resolved

pablogsal force-pushed the bpo-41654 branch 4 times, most recently from e204706 to faefcf6 Compare August 31, 2020 11:39

sweeneyde requested changes Aug 31, 2020

View reviewed changes

Lib/test/test_exceptions.py Outdated Show resolved Hide resolved

pablogsal force-pushed the bpo-41654 branch 2 times, most recently from 0e25645 to 3941f94 Compare August 31, 2020 20:31

pablogsal force-pushed the bpo-41654 branch from 3941f94 to 7def4bb Compare August 31, 2020 20:41

vstinner reviewed Sep 1, 2020

View reviewed changes

vstinner approved these changes Sep 1, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Sep 1, 2020

shihai1991 reviewed Sep 1, 2020

View reviewed changes

shihai1991 approved these changes Sep 1, 2020

View reviewed changes

fixup! bpo-41654: Fix deallocator of MemoryError to account for subcl…

8665c17

…asses

pablogsal force-pushed the bpo-41654 branch from deaeaab to 8665c17 Compare September 1, 2020 18:08

pablogsal merged commit 9b648a9 into python:master Sep 1, 2020

bedevere-bot removed the awaiting merge label Sep 1, 2020

pablogsal deleted the bpo-41654 branch September 1, 2020 18:39

miss-islington assigned pablogsal Sep 1, 2020

pablogsal removed needs backport to 3.6 labels Sep 1, 2020

bedevere-bot removed the needs backport to 3.9 only security fixes label Sep 1, 2020

Uh oh!

bpo-41654: Fix deallocator of MemoryError to account for subclasses #22020

bpo-41654: Fix deallocator of MemoryError to account for subclasses #22020

Uh oh!

Conversation

pablogsal commented Aug 30, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

pablogsal Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

vstinner Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

pablogsal Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

shihai1991 Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

pablogsal Sep 1, 2020 • 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!

shihai1991 Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

shihai1991 left a comment

Choose a reason for hiding this comment

Uh oh!

shihai1991 Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Sep 1, 2020

Uh oh!

miss-islington commented Sep 1, 2020

Uh oh!

miss-islington commented Sep 1, 2020

Uh oh!

miss-islington commented Sep 1, 2020

Uh oh!

miss-islington commented Sep 1, 2020

Uh oh!

vstinner commented Sep 1, 2020

Uh oh!

pablogsal commented Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Sep 1, 2020

Uh oh!

bedevere-bot commented Sep 1, 2020

Uh oh!

Uh oh!

pablogsal commented Aug 30, 2020 •

edited by bedevere-bot

Loading

pablogsal Sep 1, 2020 •

edited

Loading

pablogsal commented Sep 1, 2020 •

edited

Loading