[3.13] gh-128679: Fix tracemalloc.stop() race conditions #128897

vstinner · 2025-01-15T21:29:53Z

tracemalloc_alloc(), tracemalloc_realloc(), PyTraceMalloc_Track(), PyTraceMalloc_Untrack() and _PyTraceMalloc_TraceRef() now check tracemalloc_config.tracing after calling TABLES_LOCK().

_PyTraceMalloc_Stop() now protects more code with TABLES_LOCK(), especially setting tracemalloc_config.tracing to 1.

Add a test using PyTraceMalloc_Track() to test tracemalloc.stop() race condition.

Issue: Race condition in tracemalloc causes segfaults #128679

tracemalloc_alloc(), tracemalloc_realloc(), PyTraceMalloc_Track(), PyTraceMalloc_Untrack() and _PyTraceMalloc_TraceRef() now check tracemalloc_config.tracing after calling TABLES_LOCK(). _PyTraceMalloc_Stop() now protects more code with TABLES_LOCK(), especially setting tracemalloc_config.tracing to 1. Add a test using PyTraceMalloc_Track() to test tracemalloc.stop() race condition.

vstinner · 2025-01-16T12:53:55Z

@ZeroIntensity: Here is a simplified change for the 3.12 and 3.13 branch. Would you mind to review it?

ZeroIntensity

There's a couple other (unmodified) functions where I'm seeing lockless reads:

tracemalloc_get_traceback
_PyMem_DumpTraceback
_PyTraceMalloc_GetTraces
_PyTraceMalloc_GetTracedMemory
_PyTraceMalloc_ResetPeak

Python/tracemalloc.c

ZeroIntensity · 2025-01-16T13:17:42Z

Python/tracemalloc.c

@@ -1376,16 +1395,21 @@ _PyTraceMalloc_TraceRef(PyObject *op, PyRefTracerEvent event, void* Py_UNUSED(ig
    int res = -1;

    TABLES_LOCK();


We also read tracing without a lock before this.

Checking tracing without the lock is a workaround for the Python finalization. TABLES_LOCK() can no longer be called after _PyTraceMalloc_Fini() (the lock is destroyed), whereas _PyTraceMalloc_TraceRef() is still called after _PyTraceMalloc_Fini().

tracing is checked again once we acquired the lock.

The issue was fixed properly in the main branch by calling (void)PyRefTracer_SetTracer(NULL, NULL); in _PyTraceMalloc_Stop(). Since this change is designed to be backported to Python 3.12, I didn't include this fix.

Once this change is merged in 3.13 and 3.12, we can remove the unprotected tracing read in 3.13 in _PyTraceMalloc_TraceRef() by adding (void)PyRefTracer_SetTracer(NULL, NULL);.

Python/tracemalloc.c

vstinner · 2025-01-16T14:36:58Z

Ok, I completed my PR to cover most (almost all?) functions, instead of focusing on alloc/realloc/free and Track/Untrack functions.

@ZeroIntensity: Please review the updated PR.

vstinner · 2025-01-16T14:41:25Z

_PyTraceMalloc_GetTraces

Fixing properly _PyTraceMalloc_GetTraces() requires to redesign tracemalloc! For example, currently, the PyMem_Malloc() hook uses TABLES_LOCK() on a reentrant call. In the main branch, the whole _PyTraceMalloc_GetTraces() function is run with TABLES_LOCK(). If we do the same in 3.13, the PyMem_Malloc() hook must be modified to no longer use TABLES_LOCK() on a reentrant call. It would change the behavior (for correctness!), I'm not sure that it's a good thing in a stable branch.

In the main branch, I redesigned the code to be able to hold the lock for all tables operations. For that, I had to modify the behavior:

tracemalloc_realloc() and tracemalloc_free() no longer remove the trace on reentrant call.

I'm open to consider fixing _PyTraceMalloc_GetTraces() by redesigning tracemalloc in 3.13+3.12, but I'm not sure if it's worth it. Calling get_traces() is a deliberate action from the user, it's different from the tracemalloc hooks which called implicitly by running regular Python code.

ZeroIntensity · 2025-01-16T14:43:54Z

It looks like tests somehow were broken.

So TABLES_LOCK() can be called in _PyMem_DumpTraceback() even if tracemalloc is not tracing.

vstinner · 2025-01-16T14:54:08Z

It looks like tests somehow were broken.

Fixed.

Well... Fixing more functions require to backport more changes. I backported the _PyTraceMalloc_Init() change to fix test_capi and test_gc.

ZeroIntensity

LGTM. With the exception of a few special cases that you've mentioned, everything is properly locked! Due to the changes to the lifecycle and the size of the change, I think we should run buildbots before merging.

bedevere-bot · 2025-01-16T14:58:55Z

🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit 54c8f9e 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

vstinner · 2025-01-16T16:32:32Z

buildbot/AMD64 FreeBSD Refleaks PR failed

test_tracemalloc_track_race (test.test_tracemalloc.TestCAPI.test_tracemalloc_track_race) ...

Debug memory block at address p=0x82647c840: API 'r'
    16 bytes originally requested
    The 7 pad bytes at p-7 are FORBIDDENBYTE, as expected.
    The 8 pad bytes at tail=0x82647c850 are FORBIDDENBYTE, as expected.
    Data at p: 10 e3 a6 42 08 00 00 00 60 c9 47 26 08 00 00 00

Enable tracemalloc to get the memory block allocation traceback

Fatal Python error: _PyMem_DebugRawFree: bad ID: Allocated using API 'r', verified using API 'P'
Python runtime state: initialized

Thread 0x0000000831196500 (most recent call first):
  <no Python frame>

(...)

vstinner · 2025-01-16T17:00:49Z

I mark this PR as a draft because of the FreeBSD crash.

Oh, I can also reproduce the test_tracemalloc_track_race() crash on the main branch on FreeBSD.

vstinner · 2025-01-16T17:07:42Z

So far, I cannot reproduce the crash on Linux.

vstinner · 2025-01-16T17:13:57Z

I can trigger a crash if I insert a delay in PyMem_SetAllocator():

diff --git a/Objects/obmalloc.c b/Objects/obmalloc.c
index b103deb01ca..c20eb47e8a8 100644
--- a/Objects/obmalloc.c
+++ b/Objects/obmalloc.c
@@ -859,7 +859,14 @@ set_allocator_unlocked(PyMemAllocatorDomain domain, PyMemAllocatorEx *allocator)
 {
     switch(domain)
     {
-    case PYMEM_DOMAIN_RAW: _PyMem_Raw = *allocator; break;
+    case PYMEM_DOMAIN_RAW:
+        _PyMem_Raw.ctx = allocator->ctx;
+        (void)sched_yield();
+        _PyMem_Raw.malloc = allocator->malloc;
+        _PyMem_Raw.calloc = allocator->calloc;
+        _PyMem_Raw.realloc = allocator->realloc;
+        _PyMem_Raw.free = allocator->free;
+        break;
     case PYMEM_DOMAIN_MEM: _PyMem = *allocator; break;
     case PYMEM_DOMAIN_OBJ: _PyObject = *allocator; break;
     /* ignore unknown domain */

The write is protected by a lock (write), but PyMem_RawMalloc() and PyMem_RawFree() don't use the lock (read).

vstinner · 2025-01-18T14:48:37Z

Only debug build are affected, not release build. I modified the test in the main branch to skip it on debug build: #128988

…ython#128988) There is a race condition between PyMem_SetAllocator() and PyMem_RawMalloc()/PyMem_RawFree(). While PyMem_SetAllocator() write is protected by a lock, PyMem_RawMalloc()/PyMem_RawFree() reads are not protected by a lock. PyMem_RawMalloc()/PyMem_RawFree() can be called with an old context and the new function pointer. On a release build, it's not an issue since the context is not used. On a debug build, the debug hooks use the context and so can crash. (cherry picked from commit 9bc1964)

miss-islington-app · 2025-01-18T23:39:11Z

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

miss-islington-app · 2025-01-18T23:39:14Z

Sorry, @vstinner, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 6b47499510e47c0401d1f6cca2660fc12c496e39 3.12

bedevere-app · 2025-01-19T13:07:53Z

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

…129022) [3.13] gh-128679: Fix tracemalloc.stop() race conditions (#128897) tracemalloc_alloc(), tracemalloc_realloc(), PyTraceMalloc_Track(), PyTraceMalloc_Untrack() and _PyTraceMalloc_TraceRef() now check tracemalloc_config.tracing after calling TABLES_LOCK(). _PyTraceMalloc_Stop() now protects more code with TABLES_LOCK(), especially setting tracemalloc_config.tracing to 1. Add a test using PyTraceMalloc_Track() to test tracemalloc.stop() race condition. Call _PyTraceMalloc_Init() at Python startup. (cherry picked from commit 6b47499)

vstinner added the needs backport to 3.12 only security fixes label Jan 15, 2025

bedevere-app bot mentioned this pull request Jan 15, 2025

Race condition in tracemalloc causes segfaults #128679

Closed

vstinner mentioned this pull request Jan 15, 2025

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

Closed

vstinner force-pushed the tracemalloc_stop13 branch from a92f15b to 545053f Compare January 16, 2025 10:51

vstinner marked this pull request as ready for review January 16, 2025 12:53

bedevere-app bot added the awaiting core review label Jan 16, 2025

ZeroIntensity reviewed Jan 16, 2025

View reviewed changes

vstinner added 3 commits January 16, 2025 15:24

Fix the race condition in more functions

be6f0e2

Lock also in _PyTraceMalloc_IsTracing()

ef1718b

_PyTraceMalloc_Start(): lock before reading tracing

99d27fe

Call _PyTraceMalloc_Init() at Python startup

54c8f9e

So TABLES_LOCK() can be called in _PyMem_DumpTraceback() even if tracemalloc is not tracing.

vstinner requested a review from ericsnowcurrently as a code owner January 16, 2025 14:52

ZeroIntensity approved these changes Jan 16, 2025

View reviewed changes

ZeroIntensity added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 16, 2025

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 16, 2025

vstinner marked this pull request as draft January 16, 2025 17:00

bedevere-app bot removed the awaiting core review label Jan 16, 2025

vstinner mentioned this pull request Jan 18, 2025

gh-128679: Skip test_tracemalloc_track_race() on debug build #128988

Merged

vstinner marked this pull request as ready for review January 18, 2025 23:19

bedevere-app bot added the awaiting core review label Jan 18, 2025

vstinner enabled auto-merge (squash) January 18, 2025 23:20

vstinner merged commit 6b47499 into python:3.13 Jan 18, 2025
39 checks passed

vstinner deleted the tracemalloc_stop13 branch January 18, 2025 23:39

bedevere-app bot removed the awaiting core review label Jan 18, 2025

miss-islington-app bot assigned vstinner Jan 18, 2025

bedevere-app bot removed the needs backport to 3.12 only security fixes label Jan 19, 2025

		@@ -1376,16 +1395,21 @@ _PyTraceMalloc_TraceRef(PyObject op, PyRefTracerEvent event, void Py_UNUSED(ig
		int res = -1;

		TABLES_LOCK();

Uh oh!

[3.13] gh-128679: Fix tracemalloc.stop() race conditions #128897

[3.13] gh-128679: Fix tracemalloc.stop() race conditions #128897

Uh oh!

Conversation

vstinner commented Jan 15, 2025 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Jan 16, 2025

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ZeroIntensity Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

vstinner Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner commented Jan 16, 2025

Uh oh!

vstinner commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZeroIntensity commented Jan 16, 2025

Uh oh!

vstinner commented Jan 16, 2025

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 16, 2025

Uh oh!

vstinner commented Jan 16, 2025

Uh oh!

vstinner commented Jan 16, 2025

Uh oh!

vstinner commented Jan 16, 2025

Uh oh!

vstinner commented Jan 16, 2025

Uh oh!

vstinner commented Jan 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

miss-islington-app bot commented Jan 18, 2025

Uh oh!

miss-islington-app bot commented Jan 18, 2025

Uh oh!

bedevere-app bot commented Jan 19, 2025

Uh oh!

Uh oh!

vstinner commented Jan 15, 2025 •

edited by bedevere-app bot

Loading

vstinner commented Jan 16, 2025 •

edited

Loading

vstinner commented Jan 18, 2025 •

edited

Loading