Skip to content

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

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.

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

Merged
merged 5 commits into from
Jun 24, 2023
Merged

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

merged 5 commits into from
Jun 24, 2023

Conversation

chgnrdv
Copy link
Contributor

@chgnrdv chgnrdv commented Jun 22, 2023

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

'_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
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

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

@@ -222,6 +224,23 @@ class PyEagerTaskFactoryLoopTests(EagerTaskFactoryLoopTests, test_utils.TestCase
class CEagerTaskFactoryLoopTests(EagerTaskFactoryLoopTests, test_utils.TestCase):
Task = getattr(tasks, '_CTask', None)

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

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

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

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

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

@bedevere-bot
Copy link

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
…ap_current_task` (pythonGH-105989).

(cherry picked from commit d2cbb6e)

Co-authored-by: chgnrdv <[email protected]>
kumaraditya303 added a commit that referenced this pull request Jun 26, 2023
#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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash in _asyncio._swap_current_task due to improper reference counting
6 participants