gh-120289: Do not free memory in disable() to prevent use-after-free

[gaogaotiantian]

• Issue: gh-120289

[markshannon]

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. It looks like you are already adding a NULL check here anyway.

[gaogaotiantian]

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?

[bedevere-app[bot]]

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

[gaogaotiantian]

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..

This approach sounds complicated. Can't we modify timer.disable() to raise an exception if the timer is "not supposed to be disabled"?

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, initContext in ptrace_enter_call is also not friendly with exceptions. It's not possible to do that, but it's definitely not trivial. _lsprof.c is old and not quite well-maintained. To be honest cProfile is too old for a modern profiler.

[vstinner]

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.

You can emit a warning and/or log an "unraisable exception". Or simply ignore the attempt to disable the profiler silently.

[gaogaotiantian]

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.

[gaogaotiantian]

Hey @markshannon , do you still have comments on this PR? Thanks!

[gaogaotiantian]

The original reproducer in the second issue won't be quite applicable because the actual trigger (clear()) won't be executed - disable() will raise an exception and stop clear() from running. What we can do is to do a pure clear() - I'm not sure if that would repro before the patch.

[miss-islington-app[bot]]

Thanks @gaogaotiantian for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13. 🐍🍒⛏🤖

[bedevere-app[bot]]

GH-121984 is a backport of this pull request to the 3.13 branch.

[miss-islington-app[bot]]

Thanks @gaogaotiantian for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12. 🐍🍒⛏🤖

[bedevere-app[bot]]

GH-121989 is a backport of this pull request to the 3.12 branch.

[colesbury]

@gaogaotiantian, it looks like the refleaks buildbots are failing after this PR:

https://buildbot.python.org/#/builders/259/builds/1190/steps/6/logs/stdio https://buildbot.python.org/#/builders/1287/builds/289/steps/6/logs/stdio

[gaogaotiantian]

I'll take a look at it soon, thanks for the notice!

[gaogaotiantian]

It turned out that the new code did not cause the issue, it triggered the issue. The root cause is that the profiler did not check the external timer, which could be a container, in traverse. I had the fix in https://github.com/python/cpython/pull/121998 and ./python -m test -R3:3 test_cprofile passed after the fix.