Skip to content

gh-95808: Add missing early returns in _asynciomodule.c #95809

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
Merged
merged 2 commits into from
Aug 15, 2022
Merged

gh-95808: Add missing early returns in _asynciomodule.c #95809

merged 2 commits into from
Aug 15, 2022

Conversation

1st1
Copy link
Member

@1st1 1st1 commented Aug 9, 2022 •
edited by bedevere-bot
Loading

@1st1 1st1 added skip news 3.11 only security fixes 3.12 only security fixes labels Aug 9, 2022
@1st1 1st1 requested review from ambv and pablogsal August 9, 2022 05:29
@1st1 1st1 requested a review from asvetlov as a code owner August 9, 2022 05:29
@1st1
Copy link
Member Author

1st1 commented Aug 9, 2022

cc @gvanrossum Stumbled upon this while debugging TaskGroups.

1st1
1st1 commented Aug 9, 2022
View reviewed changes
Modules/_asynciomodule.c
@@ -640,6 +643,9 @@ static void
future_set_cancelled_error(FutureObj *fut)
{
PyObject *exc = create_cancelled_error(fut);
if (exc == NULL) {
return;
}
PyErr_SetObject(asyncio_CancelledError, exc);
Copy link
Member Author

Choose a reason for hiding this comment

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

Without the check we're masking the error.

1st1
1st1 commented Aug 9, 2022
View reviewed changes
Modules/_asynciomodule.c Outdated
@@ -631,6 +631,9 @@ create_cancelled_error(FutureObj *fut)
} else {
exc = PyObject_CallOneArg(asyncio_CancelledError, msg);
}
if (exc == NULL) {
return NULL;
}
PyException_SetContext(exc, fut->fut_cancelled_exc);
Copy link
Member Author

Choose a reason for hiding this comment

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

Without the check we're...segfaulting in PyException_SetContext.

@kumaraditya303 kumaraditya303 added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Aug 9, 2022
@graingert graingert self-requested a review August 9, 2022 17:13
ronaldoussoren
ronaldoussoren reviewed Aug 11, 2022
View reviewed changes
Modules/_asynciomodule.c Outdated
@@ -631,6 +631,9 @@ create_cancelled_error(FutureObj *fut)
} else {
exc = PyObject_CallOneArg(asyncio_CancelledError, msg);
}
if (exc == NULL) {
return NULL;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

PyException_SetContext steals a reference to its second argument (according to the docs), which implies that the refcount of fut->fut_cancelled_exc should be adjusted before returning (and also that the Py_CLEAR call below is dodgy).

That said, the call to PyException_SetContext appears to be a no-op because the block at line 622 is used when that fut_cancelled_exc is not NULL and that block always returns. That means fut_cancelled_exc should be NULL if execution gets here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Ronald, you're 100% right. I've pushed an update.

@graingert
Copy link
Contributor

graingert commented Aug 12, 2022 •
edited
Loading

Can you add a test for this? Does monkey patching the __init__ of CancelledError trigger this issue? Or does it have to be a subclass?

@1st1
Copy link
Member Author

1st1 commented Aug 15, 2022

Can you add a test for this? Does monkey patching the init of CancelledError trigger this issue? Or does it have to be a subclass?

I think it's hard to add a test for this. I only stumbled upon this because I was modifying the asyncio source. Even if it's possible to add a test for this fringe case, I don't think it's worth the time now, or later when somebody would be reviewing it or running it.

@1st1 1st1 merged commit b2afe48 into python:main Aug 15, 2022
@miss-islington
Copy link
Contributor

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

@1st1 1st1 deleted the fixsf branch August 15, 2022 23:32
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 15, 2022
@1st1 @miss-islington
pythongh-95808: Add missing early returns in _asynciomodule.c (python…
ede18d9 
…GH-95809)

(cherry picked from commit b2afe48)

Co-authored-by: Yury Selivanov <yury@edgedb.com>
@miss-islington
Copy link
Contributor

Sorry, @1st1, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker b2afe482f21b826d53886a69ea2c99d0d940c59a 3.10

@bedevere-bot
Copy link

GH-96013 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 Aug 15, 2022
@graingert
Copy link
Contributor

@1st1 sorry I don't understand what you mean, I can't unpack this sentence

don't think it's worth the time now, or later when somebody would be reviewing it or running it.

miss-islington added a commit that referenced this pull request Aug 16, 2022
@miss-islington @1st1
gh-95808: Add missing early returns in _asynciomodule.c (GH-95809)
af3e491 
(cherry picked from commit b2afe48)

Co-authored-by: Yury Selivanov <yury@edgedb.com>
@ZeroIntensity ZeroIntensity removed the needs backport to 3.10 only security fixes label Feb 17, 2025
@ZeroIntensity ZeroIntensity removed 3.11 only security fixes 3.12 only security fixes labels Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronaldoussoren ronaldoussoren ronaldoussoren left review comments

@ambv ambv Awaiting requested review from ambv

@pablogsal pablogsal Awaiting requested review from pablogsal

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

@graingert graingert Awaiting requested review from graingert

Assignees

@1st1 1st1

Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@1st1 @graingert @miss-islington @bedevere-bot @ronaldoussoren @ZeroIntensity @kumaraditya303