Skip to content

Commit 5a1618a

Browse files
DinoVDinoV
authored
gh-118362: Fix thread safety around lookups from the type cache in the face of concurrent mutators (#118454)
Add _PyType_LookupRef and use incref before setting attribute on type Makes setting an attribute on a class and signaling type modified atomic Avoid adding re-entrancy exposing the type cache in an inconsistent state by decrefing after type is updated
1 parent e6b213e commit 5a1618a

File tree

18 files changed

+437
-124
lines changed

18 files changed

+437
-124
lines changed

‎Include/cpython/object.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,7 @@ typedef struct _heaptypeobject {
275275

276276
PyAPI_FUNC(const char *) _PyType_Name(PyTypeObject *);
277277
PyAPI_FUNC(PyObject *) _PyType_Lookup(PyTypeObject *, PyObject *);
278+
PyAPI_FUNC(PyObject *) _PyType_LookupRef(PyTypeObject *, PyObject *);
278279
PyAPI_FUNC(PyObject *) PyType_GetDict(PyTypeObject *);
279280

280281
PyAPI_FUNC(int) PyObject_Print(PyObject *, FILE *, int);

‎Include/cpython/pyatomic.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -484,6 +484,9 @@ _Py_atomic_store_int_release(int *obj, int value);
484484
static inline int
485485
_Py_atomic_load_int_acquire(const int *obj);
486486

487+
static inline void
488+
_Py_atomic_store_uint32_release(uint32_t *obj, uint32_t value);
489+
487490
static inline void
488491
_Py_atomic_store_uint64_release(uint64_t *obj, uint64_t value);
489492

‎Include/cpython/pyatomic_gcc.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -516,6 +516,10 @@ static inline int
516516
_Py_atomic_load_int_acquire(const int *obj)
517517
{ return __atomic_load_n(obj, __ATOMIC_ACQUIRE); }
518518

519+
static inline void
520+
_Py_atomic_store_uint32_release(uint32_t *obj, uint32_t value)
521+
{ __atomic_store_n(obj, value, __ATOMIC_RELEASE); }
522+
519523
static inline void
520524
_Py_atomic_store_uint64_release(uint64_t *obj, uint64_t value)
521525
{ __atomic_store_n(obj, value, __ATOMIC_RELEASE); }

‎Include/cpython/pyatomic_msc.h

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -989,6 +989,19 @@ _Py_atomic_load_int_acquire(const int *obj)
989989
#endif
990990
}
991991

992+
static inline void
993+
_Py_atomic_store_uint32_release(uint32_t *obj, uint32_t value)
994+
{
995+
#if defined(_M_X64) || defined(_M_IX86)
996+
*(uint32_t volatile *)obj = value;
997+
#elif defined(_M_ARM64)
998+
_Py_atomic_ASSERT_ARG_TYPE(unsigned __int32);
999+
__stlr32((unsigned __int32 volatile *)obj, (unsigned __int32)value);
1000+
#else
1001+
# error "no implementation of _Py_atomic_store_uint32_release"
1002+
#endif
1003+
}
1004+
9921005
static inline void
9931006
_Py_atomic_store_uint64_release(uint64_t *obj, uint64_t value)
9941007
{

‎Include/cpython/pyatomic_std.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -911,6 +911,14 @@ _Py_atomic_load_int_acquire(const int *obj)
911911
memory_order_acquire);
912912
}
913913

914+
static inline void
915+
_Py_atomic_store_uint32_release(uint32_t *obj, uint32_t value)
916+
{
917+
_Py_USING_STD;
918+
atomic_store_explicit((_Atomic(uint32_t)*)obj, value,
919+
memory_order_release);
920+
}
921+
914922
static inline void
915923
_Py_atomic_store_uint64_release(uint64_t *obj, uint64_t value)
916924
{

‎Include/internal/pycore_dict.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,9 @@ PyAPI_FUNC(PyObject *)_PyDict_LoadGlobal(PyDictObject *, PyDictObject *, PyObjec
106106
/* Consumes references to key and value */
107107
PyAPI_FUNC(int) _PyDict_SetItem_Take2(PyDictObject *op, PyObject *key, PyObject *value);
108108
extern int _PyObjectDict_SetItem(PyTypeObject *tp, PyObject **dictptr, PyObject *name, PyObject *value);
109+
extern int _PyDict_SetItem_LockHeld(PyDictObject *dict, PyObject *name, PyObject *value);
110+
extern int _PyDict_GetItemRef_Unicode_LockHeld(PyDictObject *op, PyObject *key, PyObject **result);
111+
extern int _PyDict_GetItemRef_KnownHash(PyDictObject *op, PyObject *key, Py_hash_t hash, PyObject **result);
109112

110113
extern int _PyDict_Pop_KnownHash(
111114
PyDictObject *dict,

‎Include/internal/pycore_object.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -658,6 +658,7 @@ extern PyObject *_PyType_NewManagedObject(PyTypeObject *type);
658658
extern PyTypeObject* _PyType_CalculateMetaclass(PyTypeObject *, PyObject *);
659659
extern PyObject* _PyType_GetDocFromInternalDoc(const char *, const char *);
660660
extern PyObject* _PyType_GetTextSignatureFromInternalDoc(const char *, const char *, int);
661+
extern int _PyObject_SetAttributeErrorContext(PyObject *v, PyObject* name);
661662

662663
void _PyObject_InitInlineValues(PyObject *obj, PyTypeObject *tp);
663664
extern int _PyObject_StoreInstanceAttribute(PyObject *obj,

‎Lib/test/test_class.py

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -882,6 +882,25 @@ class Foo:
882882
f.a = 3
883883
self.assertEqual(f.a, 3)
884884

885+
def test_store_attr_type_cache(self):
886+
"""Verifies that the type cache doesn't provide a value which is
887+
inconsistent from the dict."""
888+
class X:
889+
def __del__(inner_self):
890+
v = C.a
891+
self.assertEqual(v, C.__dict__['a'])
892+
893+
class C:
894+
a = X()
895+
896+
# prime the cache
897+
C.a
898+
C.a
899+
900+
# destructor shouldn't be able to see inconsisent state
901+
C.a = X()
902+
C.a = X()
903+
885904

886905
if __name__ == '__main__':
887906
unittest.main()

‎Lib/test/test_free_threading/test_type.py

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
import unittest
2+
3+
from threading import Thread
4+
from multiprocessing.dummy import Pool
5+
from unittest import TestCase
6+
7+
from test.support import is_wasi
8+
9+
10+
NTHREADS = 6
11+
BOTTOM = 0
12+
TOP = 1000
13+
ITERS = 100
14+
15+
class A:
16+
attr = 1
17+
18+
@unittest.skipIf(is_wasi, "WASI has no threads.")
19+
class TestType(TestCase):
20+
def test_attr_cache(self):
21+
def read(id0):
22+
for _ in range(ITERS):
23+
for _ in range(BOTTOM, TOP):
24+
A.attr
25+
26+
def write(id0):
27+
for _ in range(ITERS):
28+
for _ in range(BOTTOM, TOP):
29+
# Make _PyType_Lookup cache hot first
30+
A.attr
31+
A.attr
32+
x = A.attr
33+
x += 1
34+
A.attr = x
35+
36+
37+
with Pool(NTHREADS) as pool:
38+
pool.apply_async(read, (1,))
39+
pool.apply_async(write, (1,))
40+
pool.close()
41+
pool.join()
42+
43+
def test_attr_cache_consistency(self):
44+
class C:
45+
x = 0
46+
47+
DONE = False
48+
def writer_func():
49+
for i in range(3000):
50+
C.x
51+
C.x
52+
C.x += 1
53+
nonlocal DONE
54+
DONE = True
55+
56+
def reader_func():
57+
while True:
58+
# We should always see a greater value read from the type than the
59+
# dictionary
60+
a = C.__dict__['x']
61+
b = C.x
62+
self.assertGreaterEqual(b, a)
63+
64+
if DONE:
65+
break
66+
67+
self.run_one(writer_func, reader_func)
68+
69+
def test_attr_cache_consistency_subclass(self):
70+
class C:
71+
x = 0
72+
73+
class D(C):
74+
pass
75+
76+
DONE = False
77+
def writer_func():
78+
for i in range(3000):
79+
D.x
80+
D.x
81+
C.x += 1
82+
nonlocal DONE
83+
DONE = True
84+
85+
def reader_func():
86+
while True:
87+
# We should always see a greater value read from the type than the
88+
# dictionary
89+
a = C.__dict__['x']
90+
b = D.x
91+
self.assertGreaterEqual(b, a)
92+
93+
if DONE:
94+
break
95+
96+
self.run_one(writer_func, reader_func)
97+
98+
def run_one(self, writer_func, reader_func):
99+
writer = Thread(target=writer_func)
100+
readers = []
101+
for x in range(30):
102+
reader = Thread(target=reader_func)
103+
readers.append(reader)
104+
reader.start()
105+
106+
writer.start()
107+
writer.join()
108+
for reader in readers:
109+
reader.join()
110+
111+
if __name__ == "__main__":
112+
unittest.main()

‎Misc/NEWS.d/next/Core and Builtins/2024-05-01-14-20-28.gh-issue-118492.VUsSfn.rst

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Fix an issue where the type cache can expose a previously accessed attribute
2+
when a finalizer is run.

‎Modules/_collectionsmodule.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2511,9 +2511,9 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping,
25112511
/* Only take the fast path when get() and __setitem__()
25122512
* have not been overridden.
25132513
*/
2514-
mapping_get = _PyType_Lookup(Py_TYPE(mapping), &_Py_ID(get));
2514+
mapping_get = _PyType_LookupRef(Py_TYPE(mapping), &_Py_ID(get));
25152515
dict_get = _PyType_Lookup(&PyDict_Type, &_Py_ID(get));
2516-
mapping_setitem = _PyType_Lookup(Py_TYPE(mapping), &_Py_ID(__setitem__));
2516+
mapping_setitem = _PyType_LookupRef(Py_TYPE(mapping), &_Py_ID(__setitem__));
25172517
dict_setitem = _PyType_Lookup(&PyDict_Type, &_Py_ID(__setitem__));
25182518

25192519
if (mapping_get != NULL && mapping_get == dict_get &&
@@ -2587,6 +2587,8 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping,
25872587
}
25882588

25892589
done:
2590+
Py_XDECREF(mapping_get);
2591+
Py_XDECREF(mapping_setitem);
25902592
Py_DECREF(it);
25912593
Py_XDECREF(key);
25922594
Py_XDECREF(newval);

‎Modules/_ctypes/_ctypes.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1096,7 +1096,7 @@ static int
10961096
UnionType_setattro(PyObject *self, PyObject *key, PyObject *value)
10971097
{
10981098
/* XXX Should we disallow deleting _fields_? */
1099-
if (-1 == PyObject_GenericSetAttr(self, key, value))
1099+
if (-1 == PyType_Type.tp_setattro(self, key, value))
11001100
return -1;
11011101

11021102
if (PyUnicode_Check(key) &&

‎Modules/_lsprof.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -177,8 +177,7 @@ normalizeUserObj(PyObject *obj)
177177
PyObject *modname = fn->m_module;
178178

179179
if (name != NULL) {
180-
PyObject *mo = _PyType_Lookup(Py_TYPE(self), name);
181-
Py_XINCREF(mo);
180+
PyObject *mo = _PyType_LookupRef(Py_TYPE(self), name);
182181
Py_DECREF(name);
183182
if (mo != NULL) {
184183
PyObject *res = PyObject_Repr(mo);

‎Modules/_testcapi/gc.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,10 +99,11 @@ slot_tp_del(PyObject *self)
9999
return;
100100
}
101101
/* Execute __del__ method, if any. */
102-
del = _PyType_Lookup(Py_TYPE(self), tp_del);
102+
del = _PyType_LookupRef(Py_TYPE(self), tp_del);
103103
Py_DECREF(tp_del);
104104
if (del != NULL) {
105105
res = PyObject_CallOneArg(del, self);
106+
Py_DECREF(del);
106107
if (res == NULL)
107108
PyErr_WriteUnraisable(del);
108109
else

‎Objects/classobject.c

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -188,15 +188,18 @@ method_getattro(PyObject *obj, PyObject *name)
188188
if (PyType_Ready(tp) < 0)
189189
return NULL;
190190
}
191-
descr = _PyType_Lookup(tp, name);
191+
descr = _PyType_LookupRef(tp, name);
192192
}
193193

194194
if (descr != NULL) {
195195
descrgetfunc f = TP_DESCR_GET(Py_TYPE(descr));
196-
if (f != NULL)
197-
return f(descr, obj, (PyObject *)Py_TYPE(obj));
196+
if (f != NULL) {
197+
PyObject *res = f(descr, obj, (PyObject *)Py_TYPE(obj));
198+
Py_DECREF(descr);
199+
return res;
200+
}
198201
else {
199-
return Py_NewRef(descr);
202+
return descr;
200203
}
201204
}
202205

@@ -410,14 +413,17 @@ instancemethod_getattro(PyObject *self, PyObject *name)
410413
if (PyType_Ready(tp) < 0)
411414
return NULL;
412415
}
413-
descr = _PyType_Lookup(tp, name);
416+
descr = _PyType_LookupRef(tp, name);
414417

415418
if (descr != NULL) {
416419
descrgetfunc f = TP_DESCR_GET(Py_TYPE(descr));
417-
if (f != NULL)
418-
return f(descr, self, (PyObject *)Py_TYPE(self));
420+
if (f != NULL) {
421+
PyObject *res = f(descr, self, (PyObject *)Py_TYPE(self));
422+
Py_DECREF(descr);
423+
return res;
424+
}
419425
else {
420-
return Py_NewRef(descr);
426+
return descr;
421427
}
422428
}
423429

0 commit comments

Comments
 (0)