Skip to content

gh-157415: Fix data race in pthread wrapper - #157430

Open
vstinner wants to merge 2 commits into
python:mainfrom
vstinner:pythread_wrapper
Open

vstinner wants to merge 2 commits into
python:mainfrom
vstinner:pythread_wrapper

Conversation

@vstinner

@vstinner vstinner commented Sep 13, 2026

Copy link
Copy Markdown
Member

Use free() instead of PyMem_RawFree() in pythread_wrapper() to avoid a data race if another thread calls PyMem_SetAllocator() in parallel.

Use free() instead of PyMem_RawFree() in pythread_wrapper() to avoid
a data race if another thread calls PyMem_SetAllocator() in parallel.

Co-Authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
@vstinner

Copy link
Copy Markdown
Member Author

@kumaraditya303: So do you agree that this change is useful or not?

IMO it's worth it to fix pythread_wrapper() (avoid using PyMem_RawFree()), because the Python test suite is always run with a hidden faulthandler thread. This thread doesn't make other memory allocations.

But yes, you're right that it doesn't fix the issue in the general case.

@ngoldbaum added:

It also looks like the faulthandler thread doesn't allocate or free after bootstrap either.

cc @ngoldbaum @kumaraditya303

@kumaraditya303

Copy link
Copy Markdown
Contributor

So do you agree that this change is useful or not?

Do you plan to re-enable the no memory tests under TSAN after this?

@vstinner

Copy link
Copy Markdown
Member Author

I updated my PR to reenable "no memory" tests on TSAN.

So far, I failed to reproduce the issue locally (on Fedora 44 using GCC 16.2.1). We can rely on the TSAN job on GitHub Actions.

Do you plan to re-enable the no memory tests under TSAN after this?

I didn't think about this when I wrote the PR, but you're right, we should/can reenable "no memory" tests on TSAN.

If I understood correctly, the data race reported by TSAN (issue gh-157415) is between a test changing Python memory allocators and the "hidden" faulthandler which uses the memory allocator. With this change, the faulthandler should no longer use the Python memory allocator, and so we should be good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants