gh-112075: Fix race in constructing dict for instance#118499
gh-112075: Fix race in constructing dict for instance#118499ambv merged 4 commits intopython:mainfrom
Conversation
colesbury
left a comment
colesbury
left a comment
There was a problem hiding this comment.
We also need to fix:
- PyObject_GenericGetDict (non-managed dict case)
- PyObject_GenericSetDict
Objects/dictobject.c
Outdated
| 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.
DinoV
force-pushed
the
nogil_dict_creation
branch
2 times, most recently
from
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
| 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
| goto done; | ||
| } | ||
| #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.
colesbury
left a comment
colesbury
left a comment
There was a problem hiding this comment.
LGTM. Two minor comments below
Lock obj instead of using atomic, and fix reads/writes
3 participants
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