gh-128679: Fix tracemalloc.stop() race condition

[vstinner]

Check again 'tracemalloc_config.tracing' once the GIL is held in tracemalloc_raw_alloc() and PyTraceMalloc_Track(), since another thread can call tracemalloc.stop() during PyGILState_Ensure() call.

• Issue: Race condition in tracemalloc causes segfaults #128679

[ZeroIntensity]

Should we include a test based off the reproducer in the issue?

[vstinner]

My fix was incomplete, the PyMem_RawFree() hook also had the bug:

    Fix the race condition in the PyMem_RawFree() hook
    
    * Hold the table lock while calling _PyTraceMalloc_Stop().
    * Add tracemalloc_raw_free().

Should we include a test based off the reproducer in the issue?

Ok, I added a test. It was a good idea, I found PyMem_RawFree() bug with it :-)

[vstinner]

@ZeroIntensity: Would you mind to review the PR which now has a test?

[ZeroIntensity]

Will get to it a little later today :)

[ZeroIntensity]

I'm still not too comfortable relying on incidental lock ordering with the tables lock for synchronization here. Everywhere tracemalloc_config.tracing is used the GIL is held, right? We should be able to rely on the GIL for synchronization. Oherwise, let's either use a dedicated lock for it or one of the _Py_atomic APIs.

[ZeroIntensity]

It's a lot easier to use a PyMutex, considering you don't have to heap allocate it. I think it works without a thread state if you use _PyMutex_LockFlags(mutex, _Py_LOCK_DONT_DETACH)

[ZeroIntensity]

I think we can remove the else:

Suggested change

	else {
	// gh-128679: tracemalloc.stop() was called by another thread
	}
	// gh-128679: If we're not tracing, then tracemalloc.stop() was called by another thread

[ZeroIntensity]

As I said in the other PR, relying on lock ordering like this is really error prone. If we hold the GIL, then we should be able to use tracemalloc_config.tracing without a lock, right?

[vstinner]

As I said in the other PR, relying on lock ordering like this is really error prone.

Would you mind to elaborate?

[ZeroIntensity]

See my comment: #128710 (comment)

[vstinner]

Everywhere tracemalloc_config.tracing is used the GIL is held, right? We should be able to rely on the GIL for synchronization.

This PR adds tracemalloc_raw_free() which is called without holding the GIL and cannot acquire the GIL:

     /* GIL cannot be locked in PyMem_RawFree() because it would introduce
        a deadlock in _PyThreadState_DeleteCurrent(). */

So I decided to reuse TABLES_LOCK() to access tracemalloc_config.tracing in tracemalloc_raw_free(). For that, I also modified _PyTraceMalloc_Stop() to use TABLES_LOCK() while modifying tracemalloc_config.tracing (and the different tables).

tracemalloc_free() is called with the GIL held. I added tracemalloc_raw_free() to add the logic to check tracemalloc_config.tracing to handle tracemalloc.stop() race condition.

[ZeroIntensity]

So I decided to reuse TABLES_LOCK() to access tracemalloc_config.tracing in tracemalloc_raw_free(). For that, I also modified _PyTraceMalloc_Stop() to use TABLES_LOCK() while modifying tracemalloc_config.tracing (and the different tables).

Right, so this is only safe because of lock-ordering. This approach with the tables lock only works for this case because we assume the only thread that can modify tracemalloc_config.tracing during shutdown also holds the tables lock. But, that's not always true, because not all reads and writes are locked. For example, _PyTraceMalloc_IsTracing and _PyTraceMalloc_ClearTraces are both called without the tables lock held, so that will race. You could fix that by wrapping them with the tables lock too, but I think the much cleaner solution would be to use a dedicated lock (or atomics) for tracemalloc_config.tracing.

My other concern is re-entrancy and lock-ordering deadlocks. There are two cases here, depending on what PyThread_acquire_lock does when waiting:

• If PyThread_acquire_lock releases the GIL while waiting, then we need to add an extra check for tracemalloc_config.tracing every time we use TABLES_LOCK(), because the field could have changed while the GIL was released.
• If PyThread_acquire_lock doesn't release the GIL, then that's even worse. Imagine thread 1 is waiting on the GIL, and thread 2 holds the GIL. Thread 2 calls into _PyTraceMalloc_Stop, grabs the table lock, and then releases the GIL via an object's finalizer when clearing. Thread 2 is now waiting to re-acquire the GIL. Then, thread 1 takes the GIL and starts waiting on the tables lock. Now we're in a deadlock! Alternatively, an object's finalizer could attempt to acquire the tables lock, which will also deadlock.

Either way, it's problematic.

[tom-pytel]

• If PyThread_acquire_lock releases the GIL while waiting

PyThread_acquire_lock() can release the GIL? I only see that explicitly in PyThread_acquire_lock_timed_with_retries() which is not called by TABLES_LOCK(), what am I missing?

[vstinner]

If PyThread_acquire_lock releases the GIL while waiting

It doesn't release the GIL.

If PyThread_acquire_lock doesn't release the GIL, then that's even worse. Imagine thread 1 is waiting on the GIL, and thread 2 holds the GIL. Thread 2 calls into _PyTraceMalloc_Stop, grabs the table lock, and then releases the GIL via an object's finalizer when clearing.

_PyTraceMalloc_Stop() cannot release the GIL. It calls _Py_hashtable_clear() on these tables:

• tracemalloc_traces: value_destroy_func=raw_free().
• tracemalloc_domains: value_destroy_func=_Py_hashtable_destroy().
• tracemalloc_tracebacks: key_destroy_func=raw_free().
• tracemalloc_filenames: key_destroy_func=tracemalloc_clear_filename() which calls Py_DECREF() (filenames are Python str objects).

Py_DECREF(filename) cannot release the GIL, it's a simple Python str object.

tracemalloc is designed around "low-level" functions like the Py_hashtable API which should not release the GIL. Only the PyMem_RawMalloc() hook and PyTraceMalloc_Track() function explicitly acquire/release the GIL.

---

The tricky part is to remove a trace in PyMem_RawFree() hook which is called without holding the GIL, and it cannot acquire the GIL temporarily.

[ZeroIntensity]

Generally, things release the GIL when they're waiting on something, like a lock. PyMutex releases it by default, but I'm not sure if PyThread_acquire_lock does the same. But, if it doesn't, then that's significantly worse because we're prone to deadlocks.

_PyTraceMalloc_Stop() cannot release the GIL.

Ok, good to know. I was under the impression that any Py_DECREF could release the GIL because it could trigger a garbage collection, which will in turn call other finalizers, but I guess not.

Anyways, the fix here would be a lot simpler (and less ambiguous) if we went with a dedicated lock for tracing, but if we're really opposed to doing that, I think we could setup a pending call with Py_AddPendingCall that will eventually deallocate it on the main thread.

[vstinner]

Otherwise, let's either use a dedicated lock for it or one of the _Py_atomic APIs.

There are multiple constraints.

• (A) Only a single thread must access tracemalloc tables at the same time. TABLES_LOCK() is used for ordering operations on tables.
• (B) tracemalloc must no longer add traces as soon as _PyTraceMalloc_Stop() is called: as soon as tracemalloc_config.tracing is zero.

(B) means that tracemalloc_config.tracing must not be set while tables are modified.

My fix for the PyMem_RawFree() hook combines (A) and (B) constraints by using TABLES_LOCK() for both, since the GIL cannot be used.

[vstinner]

PyThread_acquire_lock() is a thin wrapper to pthread API which is not aware of the GIL.

[tom-pytel]

Generally, things release the GIL when they're waiting on something, like a lock. PyMutex releases it by default, but I'm not sure if PyThread_acquire_lock does the same. But, if it doesn't, then that's significantly worse because we're prone to deadlocks.

Are you sure about PyMutex? Worst thing I see in _PyMutex_LockTimed() is a call to Py_MakePendingCalls() which I thought was not supposed to release GIL and only actually execute in main thread?

EDIT: Never mind, found it.

[ZeroIntensity]

TABLES_LOCK() is used for ordering operations on tables.

Well, sort of. It's used for protecting the operations from multithreaded races, not necessarily synchronizing the threads in a certain order.

Anyways--the fix works, but only for the specific test. I'd rather dedicated locks or atomics because:

• This will still race with PyTraceMalloc_Track if it's called without a thread state, because the initial tracemalloc_config.tracing read is done without the GIL or tables lock held.
• This will likely end up being a maintenance burden; if we ever add a case where the GIL is acquired or re-acquired while holding the tables lock, then we're going to be cloudy with a chance of deadlocks.

So, I'm willing to approve this as a bandaid for 3.12, but if we go with this let's try to get a better fix in 3.13 and main (where _Py_atomic and PyMutex are available).

[vstinner]

Here is something different: I wrote PR gh-128888 to redesign tracemalloc locking. It holds TABLES_LOCK() longer and uses it to protect tracemalloc_config.tracing. It doesn't fix #128679 but prepares the code to be able to fix the issue in a reliable way.

[ZeroIntensity]

Let's either lock the read above this or remove it entirely, then LGTM for 3.12.

[vstinner]

I wrote #128897 to fix Python 3.12 and 3.13.

[vstinner]

I wrote a different fix based on my refactoring that I just merged: #128893.