Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gh-118331: Handle errors in _PyObject_SetManagedDict #118334

Merged
merged 2 commits into from Apr 29, 2024
Merged

gh-118331: Handle errors in _PyObject_SetManagedDict #118334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8451c3
gh-118331: Handle errors in _PyObject_SetManagedDict
colesbury Apr 26, 2024
8cf397e
Add assertions
colesbury Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Include/cpython/object.h
View file
Open in desktop
Expand Up @@ -493,7 +493,7 @@ do { \
PyAPI_FUNC(void *) PyObject_GetItemData(PyObject *obj);

PyAPI_FUNC(int) PyObject_VisitManagedDict(PyObject *obj, visitproc visit, void *arg);
PyAPI_FUNC(void) _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict);
PyAPI_FUNC(int) _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict);
PyAPI_FUNC(void) PyObject_ClearManagedDict(PyObject *obj);

#define TYPE_MAX_WATCHERS 8
Expand Down
29 changes: 18 additions & 11 deletions Objects/dictobject.c
View file
Open in desktop
Expand Up @@ -7056,11 +7056,12 @@ set_dict_inline_values(PyObject *obj, PyDictObject *new_dict)
}
}

void
int
_PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict)
{
assert(Py_TYPE(obj)->tp_flags & Py_TPFLAGS_MANAGED_DICT);
assert(_PyObject_InlineValuesConsistencyCheck(obj));
int err = 0;
PyTypeObject *tp = Py_TYPE(obj);
if (tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) {
PyDictObject *dict = _PyObject_GetManagedDict(obj);
Expand All @@ -7076,11 +7077,11 @@ _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict)
Py_END_CRITICAL_SECTION();

if (dict == NULL) {
return;
return 0;
}
#else
set_dict_inline_values(obj, (PyDictObject *)new_dict);
return;
return 0;
#endif
}

Expand All @@ -7089,15 +7090,16 @@ _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict)
// We've locked dict, but the actual dict could have changed
// since we locked it.
dict = _PyObject_ManagedDictPointer(obj)->dict;

FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict,
(PyDictObject *)Py_XNewRef(new_dict));

_PyDict_DetachFromObject(dict, obj);

err = _PyDict_DetachFromObject(dict, obj);
if (err == 0) {
FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're no longer assigning this before calling _PyDict_DetachFromObject we can bring back some of the assertions in _PyDict_DetachFromObject, specifically:

    assert(_PyObject_ManagedDictPointer(obj)->dict == mp);
    assert(_PyObject_InlineValuesConsistencyCheck(obj));

I think assigning after should be fine after all, because the object and the dict are locked, so we'd only see reads which are proceeding against the values which are being copied. Once the values are invalidated we'd go to the dict which is being detached, and then we'd finally start seeing the new dict.

(PyDictObject *)Py_XNewRef(new_dict));
}
Py_END_CRITICAL_SECTION2();

Py_XDECREF(dict);
if (err == 0) {
Py_XDECREF(dict);
}
}
else {
PyDictObject *dict;
Expand All @@ -7114,18 +7116,23 @@ _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict)
Py_XDECREF(dict);
}
assert(_PyObject_InlineValuesConsistencyCheck(obj));
return err;
}

void
PyObject_ClearManagedDict(PyObject *obj)
{
_PyObject_SetManagedDict(obj, NULL);
if (_PyObject_SetManagedDict(obj, NULL) < 0) {
PyErr_WriteUnraisable(NULL);
}
}

int
_PyDict_DetachFromObject(PyDictObject *mp, PyObject *obj)
{
_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(obj);
assert(_PyObject_ManagedDictPointer(obj)->dict == mp);
assert(_PyObject_InlineValuesConsistencyCheck(obj));

if (FT_ATOMIC_LOAD_PTR_RELAXED(mp->ma_values) != _PyObject_InlineValues(obj)) {
return 0;
Expand Down
2 changes: 1 addition & 1 deletion Objects/typeobject.c
View file
Open in desktop
Expand Up @@ -3167,7 +3167,7 @@ subtype_setdict(PyObject *obj, PyObject *value, void *context)
}

if (Py_TYPE(obj)->tp_flags & Py_TPFLAGS_MANAGED_DICT) {
_PyObject_SetManagedDict(obj, value);
return _PyObject_SetManagedDict(obj, value);
}
else {
dictptr = _PyObject_ComputedDictPointer(obj);
Expand Down