gh-112075: Fix race in constructing dict for instance - #118499
Conversation
Sorry, something went wrong.
colesbury
left a comment
There was a problem hiding this comment.
We also need to fix:
- PyObject_GenericGetDict (non-managed dict case)
- PyObject_GenericSetDict
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
|
|
||
| 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.
Sorry, something went wrong.
| { | ||
| PyDictKeysObject *cached; | ||
|
|
||
| PyObject *dict = FT_ATOMIC_LOAD_PTR_RELAXED(*dictptr); |
There was a problem hiding this comment.
acquire (or seq-cst)
Sorry, something went wrong.
| else { | ||
| dict = PyDict_New(); | ||
| } | ||
| FT_ATOMIC_STORE_PTR_RELAXED(*dictptr, dict); |
There was a problem hiding this comment.
release (or seq-cst)
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| } | ||
| #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)
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
colesbury
left a comment
There was a problem hiding this comment.
LGTM. Two minor comments below
Sorry, something went wrong.
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_SetItem if we don't already have a dict to ensure only one thread creates the lock at a time.