X Tutup
Skip to content

gh-105987: Fix reference counting issue in _asyncio._swap_current_task#105989

Merged
kumaraditya303 merged 5 commits intopython:mainfrom
chgnrdv:_asyncio-swap_current_task-fix-refcount
Jun 24, 2023
Merged

gh-105987: Fix reference counting issue in _asyncio._swap_current_task#105989
kumaraditya303 merged 5 commits intopython:mainfrom
chgnrdv:_asyncio-swap_current_task-fix-refcount

Conversation

@chgnrdv
Copy link
Contributor

@chgnrdv chgnrdv commented Jun 22, 2023 •
edited by bedevere-bot
Loading

Fixes #105987

_PyDict_GetItem_KnownHash returns borrowed reference to previous task object, so call to _PyDict_DelItem_KnownHash/_PyDict_SetItem_KnownHash can deallocate it before it will be returned from swap_current_task function

@chgnrdv
gh-105987: Fix reference counting issue in '_asyncio._swap_current_task'
586cd0d 
'_PyDict_GetItem_KnownHash' returns borrowed reference to previous task object, so consequent calls to '_PyDict_DelItem_KnownHash'/'_PyDict_SetItem_KnownHash' can deallocate it before it will be returned from 'swap_current_task' function
@bedevere-bot bedevere-bot mentioned this pull request Jun 22, 2023
Closed
carljm
carljm reviewed Jun 22, 2023
View reviewed changes
Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Code LGTM. Thanks for finding and fixing! Please add a NEWS entry.

@gvanrossum
Copy link
Member

gvanrossum commented Jun 22, 2023

@carljm If you're uncomfortable merging this, can you ask @kumaraditya303 for a review?

kumaraditya303
kumaraditya303 reviewed Jun 22, 2023
View reviewed changes
Lib/test/test_asyncio/test_eager_task_factory.py
Task = getattr(tasks, '_CTask', None)

def test_issue105987(self):
code = """if 1:
Copy link
Contributor

@kumaraditya303 kumaraditya303 Jun 22, 2023

Choose a reason for hiding this comment

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

You can just copy paste the code here itself no need to run in separate python process right?

Copy link
Contributor Author

@chgnrdv chgnrdv Jun 22, 2023

Choose a reason for hiding this comment

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

This gives false positive result, at least on my machine. Also I mentioned that it is common idiom to run code that used to crash in subprocess, e. g. to prevent killing of entire test suite in case of regression.

@carljm
Copy link
Member

carljm commented Jun 22, 2023

uncomfortable merging this

I wasn't, was just waiting for CI signal. But glad to have @kumaraditya303 's review!

@kumaraditya303 kumaraditya303 merged commit d2cbb6e into python:main Jun 24, 2023
@miss-islington
Copy link
Contributor

miss-islington commented Jun 24, 2023

Thanks @chgnrdv for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington
Copy link
Contributor

miss-islington commented Jun 24, 2023

Sorry, @chgnrdv and @kumaraditya303, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker d2cbb6e918d9ea39f0dd44acb53270f2dac07454 3.12

@kumaraditya303 kumaraditya303 mentioned this pull request Jun 26, 2023
Merged
@bedevere-bot
Copy link

bedevere-bot commented Jun 26, 2023

GH-106099 is a backport of this pull request to the 3.12 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 only security fixes label Jun 26, 2023
kumaraditya303 pushed a commit to kumaraditya303/cpython that referenced this pull request Jun 26, 2023
@chgnrdv @kumaraditya303
[3.12] pythongh-105987: Fix reference counting issue in `_asyncio._sw…
8f4c71c 
…ap_current_task` (pythonGH-105989).

(cherry picked from commit d2cbb6e)

Co-authored-by: chgnrdv <52372310+chgnrdv@users.noreply.github.com>
kumaraditya303 added a commit that referenced this pull request Jun 26, 2023
@kumaraditya303 @chgnrdv
[3.12] gh-105987: Fix reference counting issue in `_asyncio._swap_cur… (
9e6f8d4 
#106099)

[3.12] gh-105987: Fix reference counting issue in `_asyncio._swap_current_task` (GH-105989).
(cherry picked from commit d2cbb6e)

Co-authored-by: chgnrdv <52372310+chgnrdv@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm left review comments

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

@gvanrossum gvanrossum Awaiting requested review from gvanrossum

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

Assignees

@kumaraditya303 kumaraditya303

Labels

topic-asyncio

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Crash in _asyncio._swap_current_task due to improper reference counting

6 participants

@chgnrdv @gvanrossum @carljm @miss-islington @bedevere-bot @kumaraditya303
X Tutup