-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
gh-126080: fix UAF on task->task_context
in task_call_step_soon
due to an evil loop.__getattribute__
#126120
base: main
Are you sure you want to change the base?
Conversation
Modules/_asynciomodule.c
Outdated
PyObject *task_context = Py_NewRef(task->task_context); | ||
int ret = call_soon(state, task->task_loop, cb, NULL, task->task_context); | ||
Py_DECREF(task_context); |
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.
Why not increment the context in call_soon
to make this a more global change?
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.
- it might not always be needed
- we transfer ownership when possible to avoid an INCREF/DECREF pair
- logic should be separate IMO (same reason why we do not do it in rich comparison)
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.
INCREF
and DECREF
aren't expensive operations, I would rather get rid of future maintenance if we can.
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.
Well, it's still not a universal solution because you might have recursive calls and references that could disappear. We had those kind of issues in OrderedDict.__eq__
and there was already a discussion on doing it automatically: #119004 (comment).
Similarly, #82769 (comment) did not consider doing it automatically. Finally, Kumar wanted to reduce the number of Py_INCREF/Py_DECREF
in general (that was the reason why we used ownership transfer instead).
cc @kumaraditya303 What do you think of doing it only for call_soon though?
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.
Yeah, reducing the number of reference count operations is good, but I'm worried that we'll actually end up adding more by doing this on a case-by-case basis.
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.
Well, the thing is that each call may or may not be subject to a UAF. Some parts of the code are not UAF-vulnerable because the Python code won't be able to mutate the attribute during the call. It's important to check the performances as well in large benchmarks (__eq__
is used quite often and having Py_INCREF/Py_DECREF
by default may have undesired performance impact; this must be benchmarked properly though).
Note: personally, I'm in favor of changing everything in one go but I honestly don't know if it won't make bugs harder to hunt. For instance, this solution was not sufficient in the OrderedDict.__eq__
case!
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.
I really doubt Py_INCREF
and Py_DECREF
will add a noticeable performance impact, they're both extremely fast (especially for DECREF
s that don't run arbitrary Python code).
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.
I'm OK to accept the C code part of patch (after additional quick review), but I'm -1 on submitting the test, which looks extremely complicated and nearly impossible to maintain. Also due to how the test is structured it might be a pain to keep it up to date with future asyncio development.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
@1st1, would it make sense to
|
@encukou @1st1 I agree that these tests are overly complicated and would be better in its own test file. I also think that the tests would benefit from better naming (ridiculous setup isn't very descriptive for future maintainers). I also think a one or two line docstring of what the test is testing would be helpful too. |
I can find a better name for that :)
I assumed that referencing the issue was sufficient but I can add some comments!
Would it be possible to delay that one until we fix all UAFs or make it the priority for now (either way is fine for me). @Nico-Posada has one PR that fixes another UAF and may be working on a second one for an even more contrived crash. If you want it to be now, I can create a PR just to isolate the existing tests (and to move |
I'm really not sure this all is worth the hassle. It's only a matter of time until someone stumbles upon this test and will not understand why it's there :) This PR tests something very abstract and not occurring in real life scenarios, so I personally think it's OK to commit the fix for the sake of having cleaner code, but on the other hand there's no need to overthink it. |
…uaf-126080 # Conflicts: # Lib/test/test_asyncio/test_tasks.py
401335c
to
47e26c8
Compare
This reverts commit 56a50ca.
47e26c8
to
af8614d
Compare
I've removed the contrived test as well as the small refactorization I needed. It's much cleaner and for posterity, people can just look at 02415df to get a test in the future. |
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.
Ok, LGTM. If any future problems come up with wrapping call_soon
, let's move the job of incref'ing there instead of in the caller. For now, this is fine.
I cleaned up the PoC a bit. The pure Python implementation should not be vulnerable to that kind of shenanigans although the interpreter raises an exception upon interpreter's exit (but it doesn't crash).
task_call_step_soon
in_asynciomodule.c
(with admittedly ridiculous setup) #126080