gh-140798: Fix memory leak with threading.local when finalizer resurrects threading_local_key

[efimov-mikhail]

• Issue: Memory leak with threading.local when tracing is active #140798

[sergey-miryanov]

Tracer handles Python calls. If a thread's locals contain an object with a Python callback (e.g., a __del__ finalizer), then tracer will handle the call.

When our thread is finalizing, the locals are cleared, as are all objects within them. In this case, the finalizer of our object's __del__ will be called and handled by the tracer. This leads to the recreation of thread locals (threading_local_key and threading_local_sentinel), which leak because we mistakenly believe that the locals were cleared and will not be recreated.

In this PR, we clear the locals after clearing the tracers, so any callbacks from finalizers will not be handled, and the locals cannot be recreated.

[efimov-mikhail]

Thanks for the review, Sergey!

But I have to say that I've discovered one even more difficult case without tracing at all:

import _thread
import threading

local = _thread._local()

class ClassWithDel:
    def __del__(self):
        getattr(local, 'c', None)

def thread_func():
    local.c = ClassWithDel()

t = threading.Thread(target=thread_func)
t.start()
t.join()

There's a leak with ASAN since finalizer of local.c object recreates localsdict for this thread.

CC @mpage @colesbury @ambv as author and reviewers of #121655.

Btw, we can merge this as is and create another issue for this case if this will be clearer.

[colesbury]

This does not look like the right approach to me. I think you will end up playing whac-a-mole where each time you "fix" one leak you will introduce other bugs. You need a robust approach to dealing with finalizers like the GC has.

[efimov-mikhail]

Thanks for the comment, @colesbury .
I'd like to stress that this PR doesn't introduce new bugs, it just doesn't fix all related issues in the current state. There's a leak with this code snippet on main.

And I'll be grateful for some piece of advice, if possible.

[colesbury]

I think you just haven't yet found the new bugs that this PR introduces. The original bug report was only found via fuzzing. I suspect that if you keep fuzzing you will find different ones with this PR.

The problems is that we have code in the form:

Py_CLEAR(tstate->var1);
Py_CLEAR(tstate->var2);
...
Py_CLEAR(tstate->varN);

The underlying problem is that when you clear tstate->var2 you may execute Python code that re-initializes tstate->var2 or variables cleared earlier like tstate->var1. If you move around the order of clearing, you change the possible bugs.

For example, if you move tstate->threading_local_key to the end, you now have potential leaks where thread local variable destructors set things like tstate->asyncio_running_loop

[efimov-mikhail]

In general, I agree with you, swapping clear function calls isn't good way of fixing anything.
I have some new thoughts on this topic. I'll continue to find better solution.

It seems that we should find new solution.