-
-
Notifications
You must be signed in to change notification settings - Fork 29.4k
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-120289: Do not free memory in disable() to prevent use-after-free #120297
base: main
Are you sure you want to change the base?
Conversation
gaogaotiantian
commented
Jun 9, 2024
•
edited by bedevere-app
bot
Loading
edited by bedevere-app
bot
- Issue: Use After Free in initContext(_lsprof.c) #120289
I'm not familiar with this code, so I might be mistaken, but... Needing a flag to determine if something is in a freelist seems risky. What if it is both in the freelist and in use, can't it end up being used twice? The safer (if maybe a tad slower) approach would be to NULL out the context whenever it is freed and check before use. |
Yeah that's one possible way to go. I thought of that but I also thought I can avoid the NULL checking with the free list solution. I was wrong and the ultimate solution was not as clean as I expected. I changed the solution to NULL the context when something goes wrong. Do you think it's better? |
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.
The news entry needs to be fixed. Should be good to merge once that's done.
@@ -0,0 +1 @@ | |||
Generate an unraisable exception if the external timer :mod:`cProfile` uses changes the profile 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.
I don't think this is correct any more.
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 changed that. I did not put implementation detail in it because I remember that we are not supposed to do that. So I just said I fixed the issue and mentioned the context.
When you're done making the requested changes, leave the comment: |
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.
This approach sounds complicated. Can't we modify timer.disable() to raise an exception if the timer is "not supposed to be disabled"?
I just realized when I tried to click "quote reply", I accidentally clicked "edit" .. So here's my reply, and I'll try to restore your reply..
That's not the issue. The issue is that the profiler is disabled in the timer, which frees the context memory and caused the UAF. It's possible to enforce that the profiler should not be disabled in the timer, but the code of the timer does not propagate exceptions. static inline PyTime_t
call_timer(ProfilerObject *pObj)
{
if (pObj->externalTimer != NULL) {
return CallExternalTimer(pObj);
}
else {
PyTime_t t;
(void)PyTime_PerfCounterRaw(&t);
return t;
}
} More than that, |
You can emit a warning and/or log an "unraisable exception". Or simply ignore the attempt to disable the profiler silently. |
Attempting to ignore the disable is very difficult - it requires the code to know whether it's in a timer which would make the code even more complicated. Yes emitting a warning/unraisable exception is an options, but do you mean in addition to the current code? Because this is a UAF, you need to deal with it. Simply sending a warning does not fix anything. |
Hey @markshannon , do you still have comments on this PR? Thanks! |
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.
LGTM.
I have one question.
Lib/test/test_cprofile.py
Outdated
@@ -30,6 +30,38 @@ def test_bad_counter_during_dealloc(self): | |||
|
|||
self.assertEqual(cm.unraisable.exc_type, TypeError) | |||
|
|||
@unittest.skipUnless(support.check_sanitizer(address=True), "only used to fail with ASAN") |
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.
Is this particularly slow?
I was wondering why not test it for all builds?
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.
Not really. I added this just because that was the place where it failed. I can remove this so we can test it in all environments for regression.
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.
Can you copy the second's issue reproducer to the tests please?
The original reproducer in the second issue won't be quite applicable because the actual trigger ( |