◐ Shell
reader mode source ↗
Skip to content

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

Merged
pablogsal merged 2 commits into
python:masterfrom
pablogsal:bpo-41654
Sep 1, 2020
Merged

bpo-41654: Fix deallocator of MemoryError to account for subclasses#22020
pablogsal merged 2 commits into
python:masterfrom
pablogsal:bpo-41654

Conversation

@pablogsal

@pablogsal pablogsal commented Aug 30, 2020 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

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

@pablogsal pablogsal requested a review from vstinner August 30, 2020 19:41
@pablogsal pablogsal force-pushed the bpo-41654 branch 4 times, most recently from e204706 to faefcf6 Compare August 31, 2020 11:39
@pablogsal pablogsal force-pushed the bpo-41654 branch 2 times, most recently from 0e25645 to 3941f94 Compare August 31, 2020 20:31
@pablogsal
bpo-41654: Fix deallocator of MemoryError to account for subclasses …
7def4bb 
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.
vstinner
vstinner approved these changes Sep 1, 2020
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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
shihai1991 approved these changes Sep 1, 2020
View reviewed changes

@shihai1991 shihai1991 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

LGTM.

@pablogsal pablogsal merged commit 9b648a9 into python:master Sep 1, 2020
@miss-islington

miss-islington commented Sep 1, 2020

Copy link
Copy Markdown
Contributor

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

16 hidden items Load more…
@pablogsal pablogsal deleted the bpo-41654 branch September 1, 2020 18:39
@miss-islington

miss-islington commented Sep 1, 2020

Copy link
Copy Markdown
Contributor

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

miss-islington commented Sep 1, 2020

Copy link
Copy Markdown
Contributor

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

miss-islington commented Sep 1, 2020

Copy link
Copy Markdown
Contributor

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

miss-islington commented Sep 1, 2020

Copy link
Copy Markdown
Contributor

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

vstinner commented Sep 1, 2020

Copy link
Copy Markdown
Member

@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

pablogsal commented Sep 1, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

@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.

pablogsal added a commit to pablogsal/cpython that referenced this pull request Sep 1, 2020
@pablogsal
[3.9] bpo-41654: Fix deallocator of MemoryError to account for subcla… …
87e91ae 
…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 <Pablogsal@gmail.com>
@bedevere-bot

bedevere-bot commented Sep 1, 2020

Copy link
Copy Markdown

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

@bedevere-bot

bedevere-bot commented Sep 1, 2020

Copy link
Copy Markdown

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

pablogsal added a commit to pablogsal/cpython that referenced this pull request Sep 1, 2020
@pablogsal
[3.8] [3.9] bpo-41654: Fix deallocator of MemoryError to account for … …
5307a6d 
…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 <Pablogsal@gmail.com>.
(cherry picked from commit 87e91ae)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
pablogsal added a commit that referenced this pull request Sep 1, 2020
@pablogsal
[3.8] [3.9] bpo-41654: Fix deallocator of MemoryError to account for … …
77f4000 
…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 <Pablogsal@gmail.com>.
(cherry picked from commit 87e91ae)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
pablogsal added a commit that referenced this pull request Sep 1, 2020
@pablogsal
[3.9] bpo-41654: Fix deallocator of MemoryError to account for subcla… …
d14775d 
…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 <Pablogsal@gmail.com>
@python python deleted a comment from bedevere-bot Sep 1, 2020
@python python deleted a comment from bedevere-bot Sep 1, 2020
@python python deleted a comment from bedevere-bot Sep 1, 2020
@python python deleted a comment from bedevere-bot Sep 1, 2020
@python python deleted a comment from bedevere-bot Sep 2, 2020
xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
@pablogsal @xzy3-cdc
bpo-41654: Fix deallocator of MemoryError to account for subclasses (p… …
cb998cc 
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@corona10 corona10 corona10 left review comments

@sweeneyde sweeneyde sweeneyde requested changes

@vstinner vstinner vstinner approved these changes

@shihai1991 shihai1991 shihai1991 approved these changes

Assignees

@pablogsal pablogsal

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@pablogsal @miss-islington @vstinner @bedevere-bot @corona10 @shihai1991 @sweeneyde @the-knights-who-say-ni