gh-112075: Fix race in constructing dict for instance #118499
Conversation
Objects/dictobject.c
Outdated
| @@ -7222,37 +7222,39 @@ _PyObjectDict_SetItem(PyTypeObject *tp, PyObject **dictptr, | |||
| PyInterpreterState *interp = _PyInterpreterState_GET(); | |||
|
|
|||
| assert(dictptr != NULL); | |||
| if ((tp->tp_flags & Py_TPFLAGS_HEAPTYPE) && (cached = CACHED_KEYS(tp))) { | |||
| assert(dictptr != NULL); | |||
| dict = *dictptr; | |||
There was a problem hiding this comment.
We need to use atomics to load from the dictptr or there will be a data race between the read here and the write within the critical section.
a9ad363 to
56a79e6
Compare
Objects/dictobject.c
Outdated
|
|
||
| Py_BEGIN_CRITICAL_SECTION(dict); | ||
| res = set_or_del_lock_held((PyDictObject *)dict, key, value); | ||
| Py_END_CRITICAL_SECTION(); |
There was a problem hiding this comment.
I think the ASSERT_CONSISTENT needs to be inside the critical section.
Objects/dictobject.c
Outdated
| { | ||
| PyDictKeysObject *cached; | ||
|
|
||
| PyObject *dict = FT_ATOMIC_LOAD_PTR_RELAXED(*dictptr); |
Objects/dictobject.c
Outdated
| else { | ||
| dict = PyDict_New(); | ||
| } | ||
| FT_ATOMIC_STORE_PTR_RELAXED(*dictptr, dict); |
Objects/dictobject.c
Outdated
| if (dict == NULL) { | ||
| dictkeys_decref(interp, cached, false); | ||
| } |
There was a problem hiding this comment.
I think new_dict_with_shared_keys already handles the dictkeys_decref in the error case.
There was a problem hiding this comment.
Oh, that's super subtle, I don't think there's any reason to spread this out, I'll move the incref into new_dict_with_shared_keys
Objects/object.c
Outdated
| @@ -1797,7 +1797,11 @@ PyObject_GenericSetDict(PyObject *obj, PyObject *value, void *context) | |||
| "not a '%.200s'", Py_TYPE(value)->tp_name); | |||
| return -1; | |||
| } | |||
| #ifdef Py_GIL_DISABLED | |||
| Py_XDECREF(_Py_atomic_exchange_ptr(dictptr, Py_NewRef(value))); | |||
There was a problem hiding this comment.
I think we need to be consistent about either using atomic exchange/cas or using the object locks to protect the dictptr. I don't think we can safely mix them.
Objects/dictobject.c
Outdated
| } | ||
| #endif | ||
| PyTypeObject *tp = Py_TYPE(obj); | ||
| if ((tp->tp_flags & Py_TPFLAGS_HEAPTYPE) && (cached = CACHED_KEYS(tp))) { |
There was a problem hiding this comment.
maybe: _PyType_HasFeature(tp, Py_TPFLAGS_HEAPTYPE)
Objects/dictobject.c
Outdated
| } | ||
| #endif | ||
| OBJECT_STAT_INC(dict_materialized_on_request); |
There was a problem hiding this comment.
Are we consistent about when we increment this stat?
There was a problem hiding this comment.
We seem to actually have an extra call in materialize_managed_dict_lock_held which is the inline values case. This one seems like maybe it shouldn't exist at all, as we no longer have a simple values array which is outside of the dict, so even though we have shared keys here we will always have a dict. I'll remove it from here.
Lock obj instead of using atomic, and fix reads/writes
Fix a small thread safety issue w/ dicts. When we are constructing the dict for an object with either a non-inline managed dict or with a non-managed dict we're not properly protecting the creation with a lock. This locks the object in
_PyObjectDict_SetItemif we don't already have a dict to ensure only one thread creates the lock at a time.dictobjects thread-safe in--disable-gilbuilds #112075