Skip to content

Commit fbd9677

Browse files
committed
Fix dict thread safety issues
1 parent 423bbcb commit fbd9677

File tree

1 file changed

+39
-24
lines changed

1 file changed

+39
-24
lines changed

Objects/dictobject.c

Lines changed: 39 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,11 @@ ASSERT_DICT_LOCKED(PyObject *op)
154154
_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(op);
155155
}
156156
#define ASSERT_DICT_LOCKED(op) ASSERT_DICT_LOCKED(_Py_CAST(PyObject*, op))
157+
#define ASSERT_WORLD_STOPPED_OR_DICT_LOCKED(op) \
158+
if (!_PyInterpreterState_GET()->stoptheworld.world_stopped) { \
159+
ASSERT_DICT_LOCKED(op); \
160+
}
161+
157162
#define IS_DICT_SHARED(mp) _PyObject_GC_IS_SHARED(mp)
158163
#define SET_DICT_SHARED(mp) _PyObject_GC_SET_SHARED(mp)
159164
#define LOAD_INDEX(keys, size, idx) _Py_atomic_load_int##size##_relaxed(&((const int##size##_t*)keys->dk_indices)[idx]);
@@ -670,6 +675,8 @@ dump_entries(PyDictKeysObject *dk)
670675
int
671676
_PyDict_CheckConsistency(PyObject *op, int check_content)
672677
{
678+
ASSERT_WORLD_STOPPED_OR_DICT_LOCKED(op);
679+
673680
#define CHECK(expr) \
674681
do { if (!(expr)) { _PyObject_ASSERT_FAILED_MSG(op, Py_STRINGIFY(expr)); } } while (0)
675682

@@ -1580,6 +1587,8 @@ _PyDict_MaybeUntrack(PyObject *op)
15801587
PyObject *value;
15811588
Py_ssize_t i, numentries;
15821589

1590+
ASSERT_WORLD_STOPPED_OR_DICT_LOCKED(op);
1591+
15831592
if (!PyDict_CheckExact(op) || !_PyObject_GC_IS_TRACKED(op))
15841593
return;
15851594

@@ -1722,13 +1731,14 @@ static void
17221731
insert_split_value(PyInterpreterState *interp, PyDictObject *mp, PyObject *key, PyObject *value, Py_ssize_t ix)
17231732
{
17241733
assert(PyUnicode_CheckExact(key));
1734+
ASSERT_DICT_LOCKED(mp);
17251735
MAINTAIN_TRACKING(mp, key, value);
17261736
PyObject *old_value = mp->ma_values->values[ix];
17271737
if (old_value == NULL) {
17281738
uint64_t new_version = _PyDict_NotifyEvent(interp, PyDict_EVENT_ADDED, mp, key, value);
17291739
STORE_SPLIT_VALUE(mp, ix, Py_NewRef(value));
17301740
_PyDictValues_AddToInsertionOrder(mp->ma_values, ix);
1731-
mp->ma_used++;
1741+
STORE_USED(mp, mp->ma_used + 1);
17321742
mp->ma_version_tag = new_version;
17331743
}
17341744
else {
@@ -1792,7 +1802,7 @@ insertdict(PyInterpreterState *interp, PyDictObject *mp,
17921802
goto Fail;
17931803
}
17941804
mp->ma_version_tag = new_version;
1795-
mp->ma_used++;
1805+
STORE_USED(mp, mp->ma_used + 1);
17961806
ASSERT_CONSISTENT(mp);
17971807
return 0;
17981808
}
@@ -1861,7 +1871,7 @@ insert_to_emptydict(PyInterpreterState *interp, PyDictObject *mp,
18611871
ep->me_hash = hash;
18621872
STORE_VALUE(ep, value);
18631873
}
1864-
FT_ATOMIC_STORE_SSIZE_RELAXED(mp->ma_used, FT_ATOMIC_LOAD_SSIZE_RELAXED(mp->ma_used) + 1);
1874+
STORE_USED(mp, mp->ma_used + 1);
18651875
mp->ma_version_tag = new_version;
18661876
newkeys->dk_usable--;
18671877
newkeys->dk_nentries++;
@@ -1870,11 +1880,7 @@ insert_to_emptydict(PyInterpreterState *interp, PyDictObject *mp,
18701880
// the case where we're inserting from the non-owner thread. We don't use
18711881
// set_keys here because the transition from empty to non-empty is safe
18721882
// as the empty keys will never be freed.
1873-
#ifdef Py_GIL_DISABLED
1874-
_Py_atomic_store_ptr_release(&mp->ma_keys, newkeys);
1875-
#else
1876-
mp->ma_keys = newkeys;
1877-
#endif
1883+
FT_ATOMIC_STORE_PTR_RELEASE(mp->ma_keys, newkeys);
18781884
return 0;
18791885
}
18801886

@@ -2580,7 +2586,7 @@ delitem_common(PyDictObject *mp, Py_hash_t hash, Py_ssize_t ix,
25802586
Py_ssize_t hashpos = lookdict_index(mp->ma_keys, hash, ix);
25812587
assert(hashpos >= 0);
25822588

2583-
FT_ATOMIC_STORE_SSIZE_RELAXED(mp->ma_used, FT_ATOMIC_LOAD_SSIZE(mp->ma_used) - 1);
2589+
STORE_USED(mp, mp->ma_used - 1);
25842590
mp->ma_version_tag = new_version;
25852591
if (_PyDict_HasSplitTable(mp)) {
25862592
assert(old_value == mp->ma_values->values[ix]);
@@ -2752,7 +2758,7 @@ clear_lock_held(PyObject *op)
27522758
// We don't inc ref empty keys because they're immortal
27532759
ensure_shared_on_resize(mp);
27542760
mp->ma_version_tag = new_version;
2755-
mp->ma_used = 0;
2761+
STORE_USED(mp, 0);
27562762
if (oldvalues == NULL) {
27572763
set_keys(mp, Py_EMPTY_KEYS);
27582764
assert(oldkeys->dk_refcnt == 1);
@@ -3191,6 +3197,8 @@ dict_repr_lock_held(PyObject *self)
31913197
_PyUnicodeWriter writer;
31923198
int first;
31933199

3200+
ASSERT_DICT_LOCKED(mp);
3201+
31943202
i = Py_ReprEnter((PyObject *)mp);
31953203
if (i != 0) {
31963204
return i > 0 ? PyUnicode_FromString("{...}") : NULL;
@@ -3279,8 +3287,7 @@ dict_repr(PyObject *self)
32793287
static Py_ssize_t
32803288
dict_length(PyObject *self)
32813289
{
3282-
PyDictObject *mp = (PyDictObject *)self;
3283-
return _Py_atomic_load_ssize_relaxed(&mp->ma_used);
3290+
return FT_ATOMIC_LOAD_SSIZE_RELAXED(((PyDictObject *)self)->ma_used);
32843291
}
32853292

32863293
static PyObject *
@@ -3672,6 +3679,9 @@ PyDict_MergeFromSeq2(PyObject *d, PyObject *seq2, int override)
36723679
static int
36733680
dict_dict_merge(PyInterpreterState *interp, PyDictObject *mp, PyDictObject *other, int override)
36743681
{
3682+
ASSERT_DICT_LOCKED(mp);
3683+
ASSERT_DICT_LOCKED(other);
3684+
36753685
if (other == mp || other->ma_used == 0)
36763686
/* a.update(a) or a.update({}); nothing to do */
36773687
return 0;
@@ -3699,7 +3709,7 @@ dict_dict_merge(PyInterpreterState *interp, PyDictObject *mp, PyDictObject *othe
36993709
ensure_shared_on_resize(mp);
37003710
dictkeys_decref(interp, mp->ma_keys, IS_DICT_SHARED(mp));
37013711
mp->ma_keys = keys;
3702-
mp->ma_used = other->ma_used;
3712+
STORE_USED(mp, other->ma_used);
37033713
mp->ma_version_tag = new_version;
37043714
ASSERT_CONSISTENT(mp);
37053715

@@ -4034,7 +4044,7 @@ PyDict_Size(PyObject *mp)
40344044
PyErr_BadInternalCall();
40354045
return -1;
40364046
}
4037-
return ((PyDictObject *)mp)->ma_used;
4047+
return FT_ATOMIC_LOAD_SSIZE_RELAXED(((PyDictObject *)mp)->ma_used);
40384048
}
40394049

40404050
/* Return 1 if dicts equal, 0 if not, -1 if error.
@@ -4291,7 +4301,7 @@ dict_setdefault_ref_lock_held(PyObject *d, PyObject *key, PyObject *default_valu
42914301
}
42924302

42934303
MAINTAIN_TRACKING(mp, key, value);
4294-
mp->ma_used++;
4304+
STORE_USED(mp, mp->ma_used + 1);
42954305
mp->ma_version_tag = new_version;
42964306
assert(mp->ma_keys->dk_usable >= 0);
42974307
ASSERT_CONSISTENT(mp);
@@ -4413,6 +4423,8 @@ dict_popitem_impl(PyDictObject *self)
44134423
uint64_t new_version;
44144424
PyInterpreterState *interp = _PyInterpreterState_GET();
44154425

4426+
ASSERT_DICT_LOCKED(self);
4427+
44164428
/* Allocate the result tuple before checking the size. Believe it
44174429
* or not, this allocation could trigger a garbage collection which
44184430
* could empty the dict, so if we checked the size first and that
@@ -4952,19 +4964,21 @@ typedef struct {
49524964
static PyObject *
49534965
dictiter_new(PyDictObject *dict, PyTypeObject *itertype)
49544966
{
4967+
Py_ssize_t used;
49554968
dictiterobject *di;
49564969
di = PyObject_GC_New(dictiterobject, itertype);
49574970
if (di == NULL) {
49584971
return NULL;
49594972
}
49604973
di->di_dict = (PyDictObject*)Py_NewRef(dict);
4961-
di->di_used = dict->ma_used;
4962-
di->len = dict->ma_used;
4974+
used = FT_ATOMIC_LOAD_SSIZE(dict->ma_used);
4975+
di->di_used = used;
4976+
di->len = used;
49634977
if (itertype == &PyDictRevIterKey_Type ||
49644978
itertype == &PyDictRevIterItem_Type ||
49654979
itertype == &PyDictRevIterValue_Type) {
49664980
if (_PyDict_HasSplitTable(dict)) {
4967-
di->di_pos = dict->ma_used - 1;
4981+
di->di_pos = used - 1;
49684982
}
49694983
else {
49704984
di->di_pos = load_keys_nentries(dict) - 1;
@@ -5013,8 +5027,8 @@ dictiter_len(PyObject *self, PyObject *Py_UNUSED(ignored))
50135027
{
50145028
dictiterobject *di = (dictiterobject *)self;
50155029
Py_ssize_t len = 0;
5016-
if (di->di_dict != NULL && di->di_used == di->di_dict->ma_used)
5017-
len = di->len;
5030+
if (di->di_dict != NULL && di->di_used == FT_ATOMIC_LOAD_SSIZE_RELAXED(di->di_dict->ma_used))
5031+
len = FT_ATOMIC_LOAD_SSIZE_RELAXED(di->len);
50185032
return PyLong_FromSize_t(len);
50195033
}
50205034

@@ -5297,6 +5311,7 @@ dictiter_iternextitem_lock_held(PyDictObject *d, PyObject *self,
52975311
Py_ssize_t i;
52985312

52995313
assert (PyDict_Check(d));
5314+
ASSERT_DICT_LOCKED(d);
53005315

53015316
if (di->di_used != d->ma_used) {
53025317
PyErr_SetString(PyExc_RuntimeError,
@@ -5811,7 +5826,7 @@ dictview_len(PyObject *self)
58115826
_PyDictViewObject *dv = (_PyDictViewObject *)self;
58125827
Py_ssize_t len = 0;
58135828
if (dv->dv_dict != NULL)
5814-
len = dv->dv_dict->ma_used;
5829+
len = FT_ATOMIC_LOAD_SSIZE_RELAXED(dv->dv_dict->ma_used);
58155830
return len;
58165831
}
58175832

@@ -6820,15 +6835,15 @@ store_instance_attr_lock_held(PyObject *obj, PyDictValues *values,
68206835
_PyDictValues_AddToInsertionOrder(values, ix);
68216836
if (dict) {
68226837
assert(dict->ma_values == values);
6823-
dict->ma_used++;
6838+
STORE_USED(dict, dict->ma_used + 1);
68246839
}
68256840
}
68266841
else {
68276842
if (value == NULL) {
68286843
delete_index_from_values(values, ix);
68296844
if (dict) {
68306845
assert(dict->ma_values == values);
6831-
dict->ma_used--;
6846+
STORE_USED(dict, dict->ma_used - 1);
68326847
}
68336848
}
68346849
Py_DECREF(old_value);
@@ -7039,7 +7054,7 @@ _PyObject_IsInstanceDictEmpty(PyObject *obj)
70397054
if (dict == NULL) {
70407055
return 1;
70417056
}
7042-
return ((PyDictObject *)dict)->ma_used == 0;
7057+
return FT_ATOMIC_LOAD_SSIZE_RELAXED(((PyDictObject *)dict)->ma_used) == 0;
70437058
}
70447059

70457060
int

0 commit comments

Comments
 (0)