gh-117657: TSAN Fix race in `PyMember_Get` and `PyMember_Set`, for `Py_T_OBJECT_EX` type #119368

dpdani · 2024-05-21T21:52:31Z

Fix one data race and use-after-free in PyMember.

I've just started working on this, there are more races to be solved, but this is the only one visible from python code, AFAIU.

More PRs are required for making these functions thread-safe.
I've added some commented code to show what the follow-up tests would look like.

Issue: Make TSAN tests pass with the GIL disabled in free-threaded builds #117657

bedevere-app · 2024-05-21T21:52:35Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

bedevere-app · 2024-05-22T14:48:47Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

bedevere-app · 2024-05-22T16:31:29Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

dpdani · 2024-05-22T18:31:56Z

@DinoV this is ready for review now

colesbury

I don't think we can rely on _Py_REF_MAYBE_WEAKREF (i.e., _PyObject_SetMaybeWeakref) being set on the field's value. The underlying field may be modified by a C API user outside of PyMember_SetOne, such as if it's defined with a PyMemberDef.

I think the setter needs to use a critical section. The reader can optimistically try _Py_TryIncrefCompare, but needs to fall back to a read inside a critical section if that fails.

bedevere-app · 2024-05-31T20:36:54Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

dpdani · 2024-06-04T13:24:38Z

I see your point, but there would be a problem that I cannot see how to solve, though.
The signature PyMember_SetOne(char *addr, ...) implies that not only PyObjects can be passed in.
Also the comment above PyMemberDef implies the same (emphasis mine):

An array of PyMemberDef structures defines the name, type and offset
of selected members of a C structure. ...

So, what lock would I use for the critical section?
Do I just assume that char *addr always points to a python object?

colesbury · 2024-06-04T13:32:08Z

Use the original address (i.e., obj_addr in PyMember_GetOne), not the field.

We already assume it's a PyObject:

cpython/Python/structmember.c

Line 80 in bd8c1f9

PyObject *obj = (PyObject *)obj_addr;

bedevere-app · 2024-06-04T16:30:20Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

dpdani · 2024-06-04T16:31:23Z

Include/internal/pycore_pyatomic_ft_wrappers.h

@@ -59,6 +59,8 @@ extern "C" {
    _Py_atomic_store_uint16_relaxed(&value, new_value)
 #define FT_ATOMIC_STORE_UINT32_RELAXED(value, new_value) \
    _Py_atomic_store_uint32_relaxed(&value, new_value)
+#define FT_ATOMIC_EXCHANGE_PTR(value, new_value) \


I'm not using this now, should I leave it in?

Let's remove it then.

bedevere-app · 2024-06-04T16:35:48Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Python/structmember.c

Tools/tsan/suppressions_free_threading.txt

Python/structmember.c

Co-authored-by: Erlend E. Aasland <[email protected]>

bedevere-app · 2024-06-07T15:25:35Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

dpdani · 2024-06-07T15:27:18Z

@colesbury I can just remove the commented code, it isn't relevant yet.
Or did you want me to change it?

bedevere-app · 2024-06-07T16:31:07Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Python/structmember.c

Co-authored-by: Erlend E. Aasland <[email protected]>

dpdani · 2024-06-12T16:09:41Z

@colesbury let me know if you want me to make more changes.

colesbury

Sorry for the delay - I was on vacation last week.

This LGTM with a minor edit to the test. I'll make the change directly and merge this.

Lib/test/test_free_threading/test_slots.py

miss-islington-app · 2024-06-17T18:44:57Z

Thanks @dpdani for the PR, and @colesbury for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

…pythonGH-119368) Fix a race in `PyMember_GetOne` and `PyMember_SetOne` for `Py_T_OBJECT_EX`. These functions implement `__slots__` accesses for Python objects. (cherry picked from commit 362cd26) Co-authored-by: Daniele Parmeggiani <[email protected]>

bedevere-app · 2024-06-17T18:45:13Z

GH-120655 is a backport of this pull request to the 3.13 branch.

GH-119368) (#120655) Fix a race in `PyMember_GetOne` and `PyMember_SetOne` for `Py_T_OBJECT_EX`. These functions implement `__slots__` accesses for Python objects. (cherry picked from commit 362cd26) Co-authored-by: Daniele Parmeggiani <[email protected]>

dpdani · 2024-06-17T20:40:04Z

thank you @colesbury, celebrated with a good beer and good vibes 1️⃣🍻🎉

…python#119368) Fix a race in `PyMember_GetOne` and `PyMember_SetOne` for `Py_T_OBJECT_EX`. These functions implement `__slots__` accesses for Python objects.

dpdani added 7 commits May 21, 2024 00:11

Add test to reproduce data race on PyMember_Get/Set

066f365

fix race for case Py_T_OBJECT_EX

cc2858d

hide segafulting test

cff8b44

test segafults with many iterations

a14f324

retry path for free threading builds

1ee3091

comment

7aa0396

self-review

419929d

bedevere-app bot added the awaiting review label May 21, 2024

bedevere-app bot mentioned this pull request May 21, 2024

Make TSAN tests pass with the GIL disabled in free-threaded builds #117657

Open

revert change in default build

ce49aea

dpdani added 2 commits May 22, 2024 18:21

Merge branch 'refs/heads/main' into tsan-pymember

a4147f1

missing NULL check

8639eae

colesbury changed the title ~~gh-117657 TSAN Fix race in PyMember_Get and PyMember_Set, for Py_T_OBJECT_EX type~~ gh-117657: TSAN Fix race in PyMember_Get and PyMember_Set, for Py_T_OBJECT_EX type May 31, 2024

colesbury requested changes May 31, 2024

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels May 31, 2024

use critical section instead of relying on weakref flag

f30cb20

dpdani commented Jun 4, 2024

View reviewed changes

Merge branch 'main' into tsan-pymember

79396ce

colesbury added the topic-free-threading label Jun 4, 2024

colesbury reviewed Jun 4, 2024

View reviewed changes

suggested new formatting syntax

798c2f0

Co-authored-by: Erlend E. Aasland <[email protected]>

dpdani added 2 commits June 7, 2024 18:29

fix test

cb698b3

remove commented code

737eb09

erlend-aasland reviewed Jun 7, 2024

View reviewed changes

Python/structmember.c Outdated Show resolved Hide resolved

dpdani and others added 2 commits June 11, 2024 16:21

good nit

15fdffe

Co-authored-by: Erlend E. Aasland <[email protected]>

Merge branch 'main' into tsan-pymember

8baaa45

colesbury added the skip news label Jun 17, 2024

colesbury approved these changes Jun 17, 2024

View reviewed changes

Lib/test/test_free_threading/test_slots.py Outdated Show resolved Hide resolved

bedevere-app bot added awaiting merge and removed awaiting change review labels Jun 17, 2024

Update Lib/test/test_free_threading/test_slots.py

1cd2bb0

colesbury added the needs backport to 3.13 bugs and security fixes label Jun 17, 2024

colesbury enabled auto-merge (squash) June 17, 2024 18:43

colesbury merged commit 362cd26 into python:main Jun 17, 2024
35 checks passed

bedevere-app bot removed the awaiting merge label Jun 17, 2024

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jun 17, 2024

dpdani deleted the tsan-pymember branch August 21, 2024 13:53

dpdani mentioned this pull request Aug 21, 2024

gh-117657: TSAN Fix races in PyMember_Get and PyMember_Set, for C extensions #123211

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-117657: TSAN Fix race in `PyMember_Get` and `PyMember_Set`, for `Py_T_OBJECT_EX` type #119368

gh-117657: TSAN Fix race in `PyMember_Get` and `PyMember_Set`, for `Py_T_OBJECT_EX` type #119368

dpdani commented May 21, 2024 •

edited by bedevere-app bot

Loading

bedevere-app bot commented May 21, 2024

bedevere-app bot commented May 22, 2024

bedevere-app bot commented May 22, 2024

dpdani commented May 22, 2024

colesbury left a comment

bedevere-app bot commented May 31, 2024

dpdani commented Jun 4, 2024

colesbury commented Jun 4, 2024

bedevere-app bot commented Jun 4, 2024

dpdani Jun 4, 2024

colesbury Jun 4, 2024

bedevere-app bot commented Jun 4, 2024

bedevere-app bot commented Jun 7, 2024

dpdani commented Jun 7, 2024

bedevere-app bot commented Jun 7, 2024

dpdani commented Jun 12, 2024

colesbury left a comment

miss-islington-app bot commented Jun 17, 2024

bedevere-app bot commented Jun 17, 2024

dpdani commented Jun 17, 2024

gh-117657: TSAN Fix race in PyMember_Get and PyMember_Set, for Py_T_OBJECT_EX type #119368

gh-117657: TSAN Fix race in PyMember_Get and PyMember_Set, for Py_T_OBJECT_EX type #119368

Conversation

dpdani commented May 21, 2024 • edited by bedevere-app bot Loading

bedevere-app bot commented May 21, 2024

bedevere-app bot commented May 22, 2024

bedevere-app bot commented May 22, 2024

dpdani commented May 22, 2024

colesbury left a comment

Choose a reason for hiding this comment

bedevere-app bot commented May 31, 2024

dpdani commented Jun 4, 2024

colesbury commented Jun 4, 2024

bedevere-app bot commented Jun 4, 2024

dpdani Jun 4, 2024

Choose a reason for hiding this comment

colesbury Jun 4, 2024

Choose a reason for hiding this comment

bedevere-app bot commented Jun 4, 2024

bedevere-app bot commented Jun 7, 2024

dpdani commented Jun 7, 2024

bedevere-app bot commented Jun 7, 2024

dpdani commented Jun 12, 2024

colesbury left a comment

Choose a reason for hiding this comment

miss-islington-app bot commented Jun 17, 2024

bedevere-app bot commented Jun 17, 2024

dpdani commented Jun 17, 2024

gh-117657: TSAN Fix race in `PyMember_Get` and `PyMember_Set`, for `Py_T_OBJECT_EX` type #119368

gh-117657: TSAN Fix race in `PyMember_Get` and `PyMember_Set`, for `Py_T_OBJECT_EX` type #119368

dpdani commented May 21, 2024 •

edited by bedevere-app bot

Loading