GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — GH-97592: Fix crash in C remove_done_callback due to evil code by gvanrossum · Pull Request #97660 · python/cpython

Conversation

gvanrossum commented Sep 29, 2022 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Issue: Bugs in asyncio.Future.remove_done_callback() cause segfault. #97592

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added the awaiting core review label

Sep 29, 2022

gvanrossum marked this pull request as ready for review

September 29, 2022 23:10

gvanrossum requested review from 1st1 and asvetlov as code owners

September 29, 2022 23:10

gvanrossum force-pushed the fix-fut-callbacks branch from 10daef4 to d407a0b Compare

September 29, 2022 23:16

gvanrossum added 2 commits

September 29, 2022 16:20


          pythonGH-97592: Anticipate fut_callbacks being cleared by PyObject_Ri…

bc55838

…chCompareBool


          Add test

00b9536

gvanrossum force-pushed the fix-fut-callbacks branch from 2ee888c to 00b9536 Compare

September 29, 2022 23:20


          📜🤖 Added by blurb_it.

648a617

gvanrossum requested a review from kumaraditya303

September 29, 2022 23:23

gvanrossum commented Sep 29, 2022

Copy link

Copy Markdown

Member Author

@xiaxinmeng Feel free to add your review.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra approved these changes

Sep 30, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels

Sep 30, 2022

kumaraditya303 reviewed

Sep 30, 2022

View reviewed changes

Comment thread

Misc/NEWS.d/next/Library/2022-09-29-23-22-24.gh-issue-97592.tpJg_J.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

kumaraditya303 approved these changes

Sep 30, 2022

View reviewed changes

kumaraditya303 left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

kumaraditya303 added topic-asyncio needs backport to 3.10 needs backport to 3.11 only security fixes labels

Sep 30, 2022

CharlieZhao95 suggested changes

Sep 30, 2022

View reviewed changes

Comment thread

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())

CharlieZhao95 Sep 30, 2022

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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())

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_asyncio/test_futures.py

		def __eq__(self, other):
		fut.remove_done_callback(other)

		fut.remove_done_callback(evil())

CharlieZhao95 Sep 30, 2022

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Add a link to the news

a18d215

Co-authored-by: Kumar Aditya <[email protected]>

gvanrossum merged commit 63780f4 into python:main

Sep 30, 2022

miss-islington commented Sep 30, 2022

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the awaiting merge label

Sep 30, 2022

gvanrossum deleted the fix-fut-callbacks branch

September 30, 2022 19:57

bedevere-bot commented Sep 30, 2022

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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


          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 <[email protected]>

bedevere-bot commented Sep 30, 2022

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the needs backport to 3.10 label

Sep 30, 2022

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Sep 30, 2022


          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 <[email protected]>

miss-islington added a commit that referenced this pull request

Sep 30, 2022


          GH-97592: Fix crash in C remove_done_callback due to evil code (GH-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 <[email protected]>

miss-islington added a commit that referenced this pull request

Sep 30, 2022


          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 <[email protected]>

serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this pull request

Oct 2, 2022


          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.

Reviewers

JelleZijlstra

JelleZijlstra approved these changes

kumaraditya303

kumaraditya303 approved these changes

1st1

Awaiting requested review from 1st1

1st1 is a code owner

asvetlov

Awaiting requested review from asvetlov

asvetlov is a code owner

+1 more reviewer

CharlieZhao95

CharlieZhao95 requested changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

topic-asyncio

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

gvanrossum commented Sep 29, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Sep 29, 2022

Uh oh!

Uh oh!

kumaraditya303 left a comment

Choose a reason for hiding this comment

Uh oh!

CharlieZhao95 Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

CharlieZhao95 Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Sep 30, 2022

Uh oh!

bedevere-bot commented Sep 30, 2022

Uh oh!

bedevere-bot commented Sep 30, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gvanrossum commented Sep 29, 2022 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.