Skip to content

gh-129185: Fix PyTraceMalloc_Untrack() at Python exit #129191

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jan 23, 2025
Merged

gh-129185: Fix PyTraceMalloc_Untrack() at Python exit #129191

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a1ea2fe
gh-129185: Fix PyTraceMalloc_Untrack() at Python exit
vstinner Jan 22, 2025
4172b91
Test also PyTraceMalloc_Untrack() without the GIL
vstinner Jan 23, 2025
f3ec095
Call _PyTraceMalloc_Fini() after finalize_interp_clear()
vstinner Jan 23, 2025
d510567
PyTraceMalloc_Track() checks tracing with the GIL
vstinner Jan 23, 2025
31bfe5b
Test also Untrack in test_tracemalloc_track_race()
vstinner Jan 23, 2025
555c70d
Get the GIL in Untrack
vstinner Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions Lib/test/test_tracemalloc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import contextlib
import os
import sys
import textwrap
import tracemalloc
import unittest
from unittest.mock import patch
Expand All @@ -19,6 +20,7 @@
_testinternalcapi = None


DEFAULT_DOMAIN = 0
EMPTY_STRING_SIZE = sys.getsizeof(b'')
INVALID_NFRAME = (-1, 2**30)

Expand Down Expand Up @@ -1110,6 +1112,29 @@ def test_tracemalloc_track_race(self):
# gh-128679: Test fix for tracemalloc.stop() race condition
_testcapi.tracemalloc_track_race()

def test_late_untrack(self):
code = textwrap.dedent(f"""
from test import support
import tracemalloc
import _testcapi

class Tracked:
def __init__(self, domain, size):
self.domain = domain
self.ptr = id(self)
self.size = size
_testcapi.tracemalloc_track(self.domain, self.ptr, self.size)

def __del__(self, untrack=_testcapi.tracemalloc_untrack):
untrack(self.domain, self.ptr)

domain = {DEFAULT_DOMAIN}
tracemalloc.start()
obj = Tracked(domain, 1024 * 1024)
support.late_deletion(obj)
""")
assert_python_ok("-c", code)


if __name__ == "__main__":
unittest.main()
10 changes: 10 additions & 0 deletions Python/tracemalloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1255,6 +1255,11 @@ int
PyTraceMalloc_Track(unsigned int domain, uintptr_t ptr,
size_t size)
{
// gh-129185: Pre-check to support calls after _PyTraceMalloc_Fini()
if (!tracemalloc_config.tracing) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to hold the lock or GIL, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this "pre-check" is to avoid the GIL and TABLES_LOCK().

The flag is tested again below with TABLES_LOCK().

PyTraceMalloc_Untrack() doesn't lock the GIL, only TABLES_LOCK(), when tracing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change restores the check that we had before recent refactoring. For example, Python 3.14 alpha3 uses:

int
PyTraceMalloc_Track(unsigned int domain, uintptr_t ptr,
                    size_t size)
{
    int res;
    PyGILState_STATE gil_state;

    if (!tracemalloc_config.tracing) {
        /* tracemalloc is not tracing: do nothing */
        return -2;
    }

    gil_state = PyGILState_Ensure();

    TABLES_LOCK();
    res = tracemalloc_add_trace(domain, ptr, size);
    TABLES_UNLOCK();

    PyGILState_Release(gil_state);
    return res;
}

The check is done without holding the GIL.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then we lose the thread safety: if one thread that holds the GIL were to write to tracing, then a thread that calls one of these without the GIL would race.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the easiest solution would be to grab the tables lock for the read, and then unlock it before calling PyGILState_Ensure to prevent lock-ordering deadlocks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The race is right here. Thread A might check tracing at the exact same time as thread B, which is a data race.

What is the problem of "continue execution" of thread A, since thread A checks again tracing with the tables lock?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it might not reach the second tracing read at all--if the first one races, then we're probably going to get a crash.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're talking about thread B calling _PyTraceMalloc_Fini(), not tracemalloc.stop(). In this case, thread A can call TABLES_LOCK() after thread B deleted the lock, and yes, we can get a crash.

Sadly, PyTraceMalloc_Track() and PyTraceMalloc_Untrack() API doesn't require the caller to hold the GIL.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That can happen too, but I meant that we'll get a crash from the data race on tracing, not a use-after-free on the lock. I think we just hold the GIL (via PyGILState_Ensure) and call without the tables lock held, that should be thread safe enough for 3.12.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified the Untrack() function to get the GIL. It should address your last concern.

return -2;
}

PyGILState_STATE gil_state = PyGILState_Ensure();
TABLES_LOCK();

Expand All @@ -1277,6 +1282,11 @@ PyTraceMalloc_Track(unsigned int domain, uintptr_t ptr,
int
PyTraceMalloc_Untrack(unsigned int domain, uintptr_t ptr)
{
// gh-129185: Pre-check to support calls after _PyTraceMalloc_Fini()
if (!tracemalloc_config.tracing) {
return -2;
}

TABLES_LOCK();

int result;
Expand Down
Loading