Skip to content

Commit 396f8b0

Browse files
miss-islingtondpdani
miss-islington
and
dpdani
authored
[3.13] gh-117657: Fix __slots__ thread safety in free-threaded build (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 <8658291+dpdani@users.noreply.github.com>
1 parent 0dbb86c commit 396f8b0

4 files changed

Lines changed: 77 additions & 12 deletions

File tree

‎Lib/test/test_descr.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1314,7 +1314,7 @@ class X(object):
13141314
# Inherit from object on purpose to check some backwards compatibility paths
13151315
class X(object):
13161316
__slots__ = "a"
1317-
with self.assertRaisesRegex(AttributeError, "'X' object has no attribute 'a'"):
1317+
with self.assertRaisesRegex(AttributeError, "'test.test_descr.ClassPropertiesAndMethods.test_slots.<locals>.X' object has no attribute 'a'"):
13181318
X().a
13191319

13201320
# Test string subclass in `__slots__`, see gh-98783

‎Lib/test/test_free_threading/test_slots.py‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import threading
2+
from test.support import threading_helper
3+
from unittest import TestCase
4+
5+
6+
def run_in_threads(targets):
7+
"""Run `targets` in separate threads"""
8+
threads = [
9+
threading.Thread(target=target)
10+
for target in targets
11+
]
12+
for thread in threads:
13+
thread.start()
14+
for thread in threads:
15+
thread.join()
16+
17+
18+
@threading_helper.requires_working_threading()
19+
class TestSlots(TestCase):
20+
21+
def test_object(self):
22+
class Spam:
23+
__slots__ = [
24+
"eggs",
25+
]
26+
27+
def __init__(self, initial_value):
28+
self.eggs = initial_value
29+
30+
spam = Spam(0)
31+
iters = 20_000
32+
33+
def writer():
34+
for _ in range(iters):
35+
spam.eggs += 1
36+
37+
def reader():
38+
for _ in range(iters):
39+
eggs = spam.eggs
40+
assert type(eggs) is int
41+
assert 0 <= eggs <= iters
42+
43+
run_in_threads([writer, reader, reader, reader])

‎Python/structmember.c‎

Lines changed: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,22 @@
44
#include "Python.h"
55
#include "pycore_abstract.h" // _PyNumber_Index()
66
#include "pycore_long.h" // _PyLong_IsNegative()
7+
#include "pycore_object.h" // _Py_TryIncrefCompare(), FT_ATOMIC_*()
8+
#include "pycore_critical_section.h"
79

810

11+
static inline PyObject *
12+
member_get_object(const char *addr, const char *obj_addr, PyMemberDef *l)
13+
{
14+
PyObject *v = FT_ATOMIC_LOAD_PTR(*(PyObject **) addr);
15+
if (v == NULL) {
16+
PyErr_Format(PyExc_AttributeError,
17+
"'%T' object has no attribute '%s'",
18+
(PyObject *)obj_addr, l->name);
19+
}
20+
return v;
21+
}
22+
923
PyObject *
1024
PyMember_GetOne(const char *obj_addr, PyMemberDef *l)
1125
{
@@ -75,15 +89,19 @@ PyMember_GetOne(const char *obj_addr, PyMemberDef *l)
7589
Py_INCREF(v);
7690
break;
7791
case Py_T_OBJECT_EX:
78-
v = *(PyObject **)addr;
79-
if (v == NULL) {
80-
PyObject *obj = (PyObject *)obj_addr;
81-
PyTypeObject *tp = Py_TYPE(obj);
82-
PyErr_Format(PyExc_AttributeError,
83-
"'%.200s' object has no attribute '%s'",
84-
tp->tp_name, l->name);
85-
}
92+
v = member_get_object(addr, obj_addr, l);
93+
#ifndef Py_GIL_DISABLED
8694
Py_XINCREF(v);
95+
#else
96+
if (v != NULL) {
97+
if (!_Py_TryIncrefCompare((PyObject **) addr, v)) {
98+
Py_BEGIN_CRITICAL_SECTION((PyObject *) obj_addr);
99+
v = member_get_object(addr, obj_addr, l);
100+
Py_XINCREF(v);
101+
Py_END_CRITICAL_SECTION();
102+
}
103+
}
104+
#endif
87105
break;
88106
case Py_T_LONGLONG:
89107
v = PyLong_FromLongLong(*(long long *)addr);
@@ -92,6 +110,7 @@ PyMember_GetOne(const char *obj_addr, PyMemberDef *l)
92110
v = PyLong_FromUnsignedLongLong(*(unsigned long long *)addr);
93111
break;
94112
case _Py_T_NONE:
113+
// doesn't require free-threading code path
95114
v = Py_NewRef(Py_None);
96115
break;
97116
default:
@@ -118,6 +137,9 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v)
118137
return -1;
119138
}
120139

140+
#ifdef Py_GIL_DISABLED
141+
PyObject *obj = (PyObject *) addr;
142+
#endif
121143
addr += l->offset;
122144

123145
if ((l->flags & Py_READONLY))
@@ -281,8 +303,10 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v)
281303
break;
282304
case _Py_T_OBJECT:
283305
case Py_T_OBJECT_EX:
306+
Py_BEGIN_CRITICAL_SECTION(obj);
284307
oldv = *(PyObject **)addr;
285-
*(PyObject **)addr = Py_XNewRef(v);
308+
FT_ATOMIC_STORE_PTR_RELEASE(*(PyObject **)addr, Py_XNewRef(v));
309+
Py_END_CRITICAL_SECTION();
286310
Py_XDECREF(oldv);
287311
break;
288312
case Py_T_CHAR: {

‎Tools/tsan/suppressions_free_threading.txt‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,6 @@ race_top:_PyEval_EvalFrameDefault
2929
race_top:assign_version_tag
3030
race_top:insertdict
3131
race_top:lookup_tp_dict
32-
race_top:PyMember_GetOne
33-
race_top:PyMember_SetOne
3432
race_top:new_reference
3533
race_top:set_contains_key
3634
# https://gist.github.com/colesbury/d13d033f413b4ad07929d044bed86c35

0 commit comments

Comments
 (0)