Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GH-97592: Fix crash in C remove_done_callback due to evil code #97660

Merged
merged 4 commits into from
Sep 30, 2022
Merged

GH-97592: Fix crash in C remove_done_callback due to evil code #97660

merged 4 commits into from
Sep 30, 2022

Conversation

gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Sep 29, 2022 •
edited by bedevere-bot
Loading

@gvanrossum gvanrossum marked this pull request as ready for review September 29, 2022 23:10
@gvanrossum
Copy link
Member Author

@xiaxinmeng Feel free to add your review.

kumaraditya303
kumaraditya303 approved these changes Sep 30, 2022
View reviewed changes
Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

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

LGTM

CharlieZhao95
CharlieZhao95 suggested changes Sep 30, 2022
View reviewed changes
Lib/test/test_asyncio/test_futures.py
Comment on lines +844 to +848
class evil:
def __eq__(self, other):
fut.remove_done_callback(other)

fut.remove_done_callback(evil())
Copy link
Contributor

Choose a reason for hiding this comment

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

Class names should normally use the CapWords convention.

Suggested change
class evil:
def __eq__(self, other):
fut.remove_done_callback(other)
fut.remove_done_callback(evil())
class Evil:
def __eq__(self, other):
fut.remove_done_callback(other)
fut.remove_done_callback(Evil())

Lib/test/test_asyncio/test_futures.py
def __eq__(self, other):
fut.remove_done_callback(other)

fut.remove_done_callback(evil())
Copy link
Contributor

Choose a reason for hiding this comment

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

All callbacks added by fut.add_done_callback() will be cleared when calling fut.remove_done_callback(Evil()), but the return value will be the number of callbacks added last.

I'm not sure if we need to improve the return value of remove done callback further, the current changes are good enough to fix the crash. :)

@gvanrossum @kumaraditya303
Add a link to the news
a18d215 
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@gvanrossum gvanrossum merged commit 63780f4 into python:main Sep 30, 2022
@miss-islington
Copy link
Contributor

Thanks @gvanrossum for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@gvanrossum gvanrossum deleted the fix-fut-callbacks branch September 30, 2022 19:57
@bedevere-bot
Copy link

GH-97692 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Sep 30, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 30, 2022
@gvanrossum @miss-islington
pythonGH-97592: Fix crash in C remove_done_callback due to evil code (p…
6ffd0e7 
…ythonGH-97660)

Evil code could cause fut_callbacks to be cleared when PyObject_RichCompareBool is called.
(cherry picked from commit 63780f4)

Co-authored-by: Guido van Rossum <guido@python.org>
@bedevere-bot
Copy link

GH-97693 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Sep 30, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 30, 2022
@gvanrossum @miss-islington
pythonGH-97592: Fix crash in C remove_done_callback due to evil code (p…
72e19e4 
…ythonGH-97660)

Evil code could cause fut_callbacks to be cleared when PyObject_RichCompareBool is called.
(cherry picked from commit 63780f4)

Co-authored-by: Guido van Rossum <guido@python.org>
miss-islington added a commit that referenced this pull request Sep 30, 2022
@miss-islington @gvanrossum
GH-97592: Fix crash in C remove_done_callback due to evil code (GH-97660
3124618 
)

Evil code could cause fut_callbacks to be cleared when PyObject_RichCompareBool is called.
(cherry picked from commit 63780f4)

Co-authored-by: Guido van Rossum <guido@python.org>
miss-islington added a commit that referenced this pull request Sep 30, 2022
@miss-islington @gvanrossum
GH-97592: Fix crash in C remove_done_callback due to evil code (GH-97660
54bbb5e 
)

Evil code could cause fut_callbacks to be cleared when PyObject_RichCompareBool is called.
(cherry picked from commit 63780f4)

Co-authored-by: Guido van Rossum <guido@python.org>
serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this pull request Oct 2, 2022
@gvanrossum @serhiy-storchaka
pythonGH-97592: Fix crash in C remove_done_callback due to evil code (p…
67851dc 
…ython#97660)

Evil code could cause fut_callbacks to be cleared when PyObject_RichCompareBool is called.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CharlieZhao95 CharlieZhao95 CharlieZhao95 requested changes

@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Assignees
No one assigned
Labels
topic-asyncio
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gvanrossum @miss-islington @bedevere-bot @JelleZijlstra @kumaraditya303 @CharlieZhao95