Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Some operations on managed dict are not safe from memory_order POV #133980

Copy link
Copy link
Closed
@sergey-miryanov

Description

@sergey-miryanov
Issue body actions

Bug report

Bug description:

First of all, I'm not a threading expert and my understanding of the memory-ordering model may be wrong. So, if I'm wrong I will be happy to fix my knowledge lacoons.

I saw some inconsistency (from my sight) of loading and writing of managed dict pointer.
Non-atomic loads:

  1. PyObject_VisitManagedDict
    Py_VISIT(_PyObject_ManagedDictPointer(obj)->dict);
  2. PyObject_ClearManagedDict
    Py_CLEAR(_PyObject_ManagedDictPointer(obj)->dict);
  3. _PyObject_GetDictPtr
    return (PyObject **)&_PyObject_ManagedDictPointer(obj)->dict;

Non-atomic stores:

  1. _PyObject_InitInlineValues

    cpython/Objects/dictobject.c

    Lines 6787 to 6791 in 9ad0c7b

    for (size_t i = 0; i < size; i++) {
    values->values[i] = NULL;
    }
    _PyObject_ManagedDictPointer(obj)->dict = NULL;
    }

IIUC mixing of non-atomic loads/stores with atomic ones may lead to data races.

memory_order_acquire loads:

  1. _PyObject_GetManagedDict
    _PyObject_GetManagedDict(PyObject *obj)
    {
    PyManagedDictPointer *dorv = _PyObject_ManagedDictPointer(obj);
    return (PyDictObject *)FT_ATOMIC_LOAD_PTR_ACQUIRE(dorv->dict);
    }

memory_order_release stores:

  1. _PyObject_MaterializeManagedDict_LockHeld

    cpython/Objects/dictobject.c

    Lines 6827 to 6829 in 9ad0c7b

    FT_ATOMIC_STORE_PTR_RELEASE(_PyObject_ManagedDictPointer(obj)->dict,
    dict);
    return dict;
  2. store_instance_attr_lock_held

    cpython/Objects/dictobject.c

    Lines 6925 to 6927 in 9ad0c7b

    FT_ATOMIC_STORE_PTR_RELEASE(_PyObject_ManagedDictPointer(obj)->dict,
    (PyDictObject *)dict);
    return 0;
  3. ensure_managed_dict

    cpython/Objects/dictobject.c

    Lines 7494 to 7495 in 9ad0c7b

    FT_ATOMIC_STORE_PTR_RELEASE(_PyObject_ManagedDictPointer(obj)->dict,
    (PyDictObject *)dict);

memory_order_seq_cst stores:

  1. set_dict_inline_values
    FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict, new_dict);
  2. try_set_dict_inline_only_or_other_dict

    cpython/Objects/dictobject.c

    Lines 7252 to 7253 in 9ad0c7b

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

    cpython/Objects/dictobject.c

    Lines 7287 to 7289 in 9ad0c7b

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

IIUC mixing acquire/release with seq_cst may break total order of seq_cst operations.

Mixing with memory_order_seq_cst stores

  1. _PyObject_SetManagedDict

    cpython/Objects/dictobject.c

    Lines 7356 to 7365 in 9ad0c7b

    PyDictObject *dict = _PyObject_GetManagedDict(obj);
    if (dict == NULL) {
    set_dict_inline_values(obj, (PyDictObject *)new_dict);
    return 0;
    }
    if (_PyDict_DetachFromObject(dict, obj) == 0) {
    _PyObject_ManagedDictPointer(obj)->dict = (PyDictObject *)Py_XNewRef(new_dict);
    Py_DECREF(dict);
    return 0;
    }

_PyObject_SetManagedDict uses non-atomic load but stores with seq_cst mode so it is OK (IIUC), but following store without fence may lead to data race.

Are my findings valid or am I completely wrong?

cc @colesbury @kumaraditya303

CPython versions tested on:

CPython main branch

Operating systems tested on:

No response

Linked PRs

Metadata

Metadata

Assignees

No one assigned

    Labels

    3.14bugs and security fixesbugs and security fixes3.15new features, bugs and security fixesnew features, bugs and security fixesinterpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)topic-free-threadingtype-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Morty Proxy This is a proxified and sanitized view of the page, visit original site.