Skip to content

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

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
colesbury merged 20 commits into from
Jun 17, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
066f365
Add test to reproduce data race on PyMember_Get/Set
dpdani May 20, 2024
cc2858d
fix race for case `Py_T_OBJECT_EX`
dpdani May 21, 2024
cff8b44
hide segafulting test
dpdani May 21, 2024
a14f324
test segafults with many iterations
dpdani May 21, 2024
1ee3091
retry path for free threading builds
dpdani May 21, 2024
7aa0396
comment
dpdani May 21, 2024
419929d
self-review
dpdani May 21, 2024
ce49aea
revert change in default build
dpdani May 22, 2024
a4147f1
Merge branch 'refs/heads/main' into tsan-pymember
dpdani May 22, 2024
8639eae
missing NULL check
dpdani May 22, 2024
f30cb20
use critical section instead of relying on weakref flag
dpdani Jun 4, 2024
79396ce
Merge branch 'main' into tsan-pymember
dpdani Jun 4, 2024
99b1bce
review
dpdani Jun 5, 2024
8f273f7
move up
dpdani Jun 5, 2024
798c2f0
suggested new formatting syntax
dpdani Jun 7, 2024
cb698b3
fix test
dpdani Jun 7, 2024
737eb09
remove commented code
dpdani Jun 7, 2024
15fdffe
good nit
dpdani Jun 11, 2024
8baaa45
Merge branch 'main' into tsan-pymember
dpdani Jun 11, 2024
1cd2bb0
Update Lib/test/test_free_threading/test_slots.py
colesbury Jun 17, 2024
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
Prev Previous commit
Next Next commit
review
  • Loading branch information
@dpdani
dpdani committed Jun 5, 2024
commit 99b1bce542cbc2892e1540de9d38947478c51c87
13 changes: 0 additions & 13 deletions Include/internal/pycore_pyatomic_ft_wrappers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,6 @@ 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) \
_Py_atomic_exchange_ptr(value, new_value)

#else
#define FT_ATOMIC_LOAD_PTR(value) value
Expand All @@ -85,17 +83,6 @@ extern "C" {
#define FT_ATOMIC_STORE_UINT16_RELAXED(value, new_value) value = new_value
#define FT_ATOMIC_STORE_UINT32_RELAXED(value, new_value) value = new_value

static inline void *
_Py_non_atomic_exchange_ptr(void **value, void *new_value)
{
void *current = *value;
*value = new_value;
return current;
}

#define FT_ATOMIC_EXCHANGE_PTR(value, new_value) \
_Py_non_atomic_exchange_ptr(value, new_value)

#endif

#ifdef __cplusplus
Expand Down
32 changes: 14 additions & 18 deletions Python/structmember.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@
#include "Python.h"
#include "pycore_abstract.h" // _PyNumber_Index()
#include "pycore_long.h" // _PyLong_IsNegative()
#include "pycore_pyatomic_ft_wrappers.h"
#include "pycore_object.h"
#include "pycore_object.h" // _Py_TryIncrefCompare(), FT_ATOMIC_*()
#include "pycore_critical_section.h"


static inline PyObject *
_PyMember_GetOneObject(const char *addr, const char *obj_addr, PyMemberDef *l)
member_get_object(const char *addr, const char *obj_addr, PyMemberDef *l)
{
PyObject *v = FT_ATOMIC_LOAD_PTR(*(PyObject **) addr);
if (v == NULL) {
Expand All @@ -23,9 +22,6 @@ _PyMember_GetOneObject(const char *addr, const char *obj_addr, PyMemberDef *l)
return v;
}




PyObject *
PyMember_GetOne(const char *obj_addr, PyMemberDef *l)
{
Expand Down Expand Up @@ -96,16 +92,18 @@ PyMember_GetOne(const char *obj_addr, PyMemberDef *l)
break;
case Py_T_OBJECT_EX:
#ifndef Py_GIL_DISABLED
v = _PyMember_GetOneObject(addr, obj_addr, l);
v = member_get_object(addr, obj_addr, l);
Py_XINCREF(v);
#else
v = _PyMember_GetOneObject(addr, obj_addr, l);
if (v != NULL && !_Py_TryIncrefCompare((PyObject **) addr, v)) {
Py_BEGIN_CRITICAL_SECTION((PyObject *)obj_addr);
v = _PyMember_GetOneObject(addr, obj_addr, l);
Py_XINCREF(v);
v = member_get_object(addr, obj_addr, l);
if (v != NULL) {
if (!_Py_TryIncrefCompare((PyObject **) addr, v)) {
Py_BEGIN_CRITICAL_SECTION((PyObject *) obj_addr);
v = member_get_object(addr, obj_addr, l);
Py_XINCREF(v);
Py_END_CRITICAL_SECTION();
}
}
#endif
break;
case Py_T_LONGLONG:
Expand Down Expand Up @@ -142,7 +140,9 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v)
return -1;
}

#ifdef Py_GIL_DISABLED
PyObject *obj = (PyObject *) addr;
#endif
addr += l->offset;

if ((l->flags & Py_READONLY))
Expand Down Expand Up @@ -306,14 +306,10 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v)
break;
case _Py_T_OBJECT:
case Py_T_OBJECT_EX:
#ifdef Py_GIL_DISABLED
Py_BEGIN_CRITICAL_SECTION(obj);
#endif
oldv = FT_ATOMIC_LOAD_PTR(*(PyObject **)addr);
FT_ATOMIC_STORE_PTR(*(PyObject **)addr, Py_XNewRef(v));
#ifdef Py_GIL_DISABLED
oldv = *(PyObject **)addr;
FT_ATOMIC_STORE_PTR_RELEASE(*(PyObject **)addr, Py_XNewRef(v));
Py_END_CRITICAL_SECTION();
#endif
Py_XDECREF(oldv);
break;
case Py_T_CHAR: {
Expand Down
1 change: 0 additions & 1 deletion Tools/tsan/suppressions_free_threading.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ race_top:_PyType_HasFeature
race_top:assign_version_tag
race_top:insertdict
race_top:lookup_tp_dict
race_top:mi_heap_visit_pages
race_top:new_reference
race_top:set_contains_key
# https://gist.github.com/colesbury/d13d033f413b4ad07929d044bed86c35
Expand Down