Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-125789: fix side-effects inasyncio callback scheduling methods#125833

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
Show file tree
Hide file tree
Changes from1 commit
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
PrevPrevious commit
NextNext commit
Address Guido's and Yury's reviews.
  • Loading branch information
@picnixz
picnixz committedOct 23, 2024
commita05cde237e43b99a47cad807b22cd6aeba020d6b
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
Mitigate interpreter's crashes and state corruption due to side-effects in
:meth:`asyncio.Future.remove_done_callback` orothers callback scheduling
Mitigate interpreter crashes and state corruption due to side-effects in
:meth:`asyncio.Future.remove_done_callback` orother callback scheduling
methods. Patch by Bénédikt Tran.
40 changes: 21 additions & 19 deletionsModules/_asynciomodule.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -434,8 +434,8 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut)
return 0;
}

if (!PyList_Check(fut->fut_callbacks)) {
PyErr_SetString(PyExc_RuntimeError, "corruptedfuture state");
if (!PyList_CheckExact(fut->fut_callbacks)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a test for this? Is there a way to assign a non-list to fut._callbacks?

Copy link
MemberAuthor

@picnixzpicnixzOct 24, 2024•
edited
Loading

Choose a reason for hiding this comment

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

Mmh, actually no. The property is read-only and even with subclassing it's not possible. I'll replace the list checks by assertions.

EDIT: NVM, the macros PyList_GET* macros already take care of the assertion.

PyErr_SetString(PyExc_RuntimeError, "corruptedcallbacks list");
return -1;
}

Expand All@@ -446,19 +446,19 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut)
return 0;
}

// Beware: 'call_soon'below may change fut_callbacks or its items
// Beware:An evil'call_soon'could change fut_callbacks or its items
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have a test case for an evil 'call_soon'?

willingc reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

No, you're right. I didn't test but I think we can just callfut._callbacks.clear() inside the callback that is being called and that would be it but I'd need to confirm tomorrow.

willingc reacted with thumbs up emoji
Copy link
Contributor

@graingertgraingertOct 23, 2024•
edited
Loading

Choose a reason for hiding this comment

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

The callback being called won't run until the loop here is finished (and whatever is running future_schedule_callbacks returns to _run_once)

You do need an evil call_soon that either calls the callback immediately or mutates fut._callbacks itself

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, I mentioned it can be abused in my initial report but never actually followed up on it. Here's a simple crash POC

importasynciodefcatch(*args,**kwargs):fut._callbacks.clear()loop=lambda: ...loop.call_soon=catchloop.get_debug=lambda:Falsefut=asyncio.Future(loop=loop)pad=lambda: ...fut.add_done_callback(pad)fut.add_done_callback(None)fut.add_done_callback(None)fut.remove_done_callback(pad)fut.set_result("bonk")

picnixz reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Thanks for that Nico; I've also added a test for when the callback tuple is changed in the evilcall_soon.

// (see https://github.com/python/cpython/issues/125789 for details).
for (i = 0; fut->fut_callbacks != NULL; i++) {
if (!PyList_Check(fut->fut_callbacks)) {
PyErr_SetString(PyExc_RuntimeError, "corruptedfuture state");
if (!PyList_CheckExact(fut->fut_callbacks)) {
PyErr_SetString(PyExc_RuntimeError, "corruptedcallbacks list");
return -1;
}
if (i >= PyList_GET_SIZE(fut->fut_callbacks)) {
break; // done
}
PyObject *cb_tup = PyList_GET_ITEM(fut->fut_callbacks, i);
if (!PyTuple_Check(cb_tup) || PyTuple_GET_SIZE(cb_tup) < 2) {
PyErr_SetString(PyExc_RuntimeError, "corruptedfuture state");
if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) < 2) {
PyErr_SetString(PyExc_RuntimeError, "corruptedcallback tuple");
return -1;
}
PyObject *cb = PyTuple_GET_ITEM(cb_tup, 0);
Expand DownExpand Up@@ -1035,6 +1035,8 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
ENSURE_FUTURE_ALIVE(state, self)

if (self->fut_callback0 != NULL) {
// Beware: An evil PyObject_RichCompareBool could change fut_callback0
// (see https://github.com/python/cpython/issues/125789 for details).
int cmp = PyObject_RichCompareBool(self->fut_callback0, fn, Py_EQ);
if (cmp == -1) {
return NULL;
Expand All@@ -1051,8 +1053,8 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
return PyLong_FromSsize_t(cleared_callback0);
}

if (!PyList_Check(self->fut_callbacks)) {
PyErr_SetString(PyExc_RuntimeError, "corruptedfuture state");
if (!PyList_CheckExact(self->fut_callbacks)) {
PyErr_SetString(PyExc_RuntimeError, "corruptedcallbacks list");
return NULL;
}

Expand All@@ -1064,11 +1066,11 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,

if (len == 1) {
PyObject *cb_tup = PyList_GET_ITEM(self->fut_callbacks, 0);
// Beware:PyObject_RichCompareBool below maychange fut_callbacks or
// its items (see https://github.com/python/cpython/issues/97592and
// Beware:An evil PyObject_RichCompareBool couldchange fut_callbacks
//orits items (see https://github.com/python/cpython/issues/97592or
// https://github.com/python/cpython/issues/125789 for details).
if (!PyTuple_Check(cb_tup) || PyTuple_GET_SIZE(cb_tup) < 1) {
PyErr_SetString(PyExc_RuntimeError, "corruptedfuture state");
if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) < 1) {
PyErr_SetString(PyExc_RuntimeError, "corruptedcallback tuple");
return NULL;
}
Py_INCREF(cb_tup);
Expand All@@ -1092,20 +1094,20 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
return NULL;
}

// Beware:PyObject_RichCompareBool below maychange fut_callbacks or
// its items (see https://github.com/python/cpython/issues/97592and
// Beware:An evil PyObject_RichCompareBool couldchange fut_callbacks
//orits items (see https://github.com/python/cpython/issues/97592or
// https://github.com/python/cpython/issues/125789 for details).
for (i = 0; self->fut_callbacks != NULL; i++) {
if (!PyList_Check(self->fut_callbacks)) {
PyErr_SetString(PyExc_RuntimeError, "corruptedfuture state");
if (!PyList_CheckExact(self->fut_callbacks)) {
PyErr_SetString(PyExc_RuntimeError, "corruptedcallbacks list");
goto fail;
}
if (i >= PyList_GET_SIZE(self->fut_callbacks)) {
break; // done
}
PyObject *cb_tup = PyList_GET_ITEM(self->fut_callbacks, i);
if (!PyTuple_Check(cb_tup) || PyTuple_GET_SIZE(cb_tup) < 1) {
PyErr_SetString(PyExc_RuntimeError, "corruptedfuture state");
if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) < 1) {
PyErr_SetString(PyExc_RuntimeError, "corruptedcallback tuple");
goto fail;
}
Py_INCREF(cb_tup);
Expand Down
[8]ページ先頭
©2009-2025 Movatter.jp