8000 gh-118362: Fix thread safety around lookups from the type cache in th… · python/cpython@5a1618a · GitHub
[go: up one dir, main page]
Skip to content

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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)
0