Skip to content

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

Closed
@sergey-miryanov

Description

@sergey-miryanov

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

Activity

colesbury

colesbury commented on May 14, 2025

@colesbury
Contributor

I think these are fine:

1: GC traverse is called during stop the world (no other threads are running) so it doesn't need atomics
2: GC clear is called when the object is not accessible so it doesn't need atomics (same logic applies to tp_dealloc)
3: This constructs a pointer to the dictionary; it doesn't load the dictionary

_PyObject_InitInlineValues: object initialization doesn't need atomics (no other threads have access to those fields)

The memory_order_seq_cst stores can be memory_order_release, but it doesn't matter (except possibly for perf). memory_order_seq_cst is strictly stronger than memory_order_seq_cst. We don't care about a total order for memory_order_seq_cst across stores to other memory locations here.

_PyObject_SetManagedDict: this is code in an #ifdef that only runs in the default GIL-enabled build (not the free threaded build)

sergey-miryanov

sergey-miryanov commented on May 14, 2025

@sergey-miryanov
ContributorAuthor

Thanks for explanation!

About _PyObject_GetDictPtr it is called from PyObject_GenericSetDict where store to ptr is made. It is guarded by CS, but I believe it should use atomic store too. WDYT?

cpython/Objects/object.c

Lines 1933 to 1935 in 9ad0c7b

Py_BEGIN_CRITICAL_SECTION(obj);
Py_XSETREF(*dictptr, Py_NewRef(value));
Py_END_CRITICAL_SECTION();

kumaraditya303

kumaraditya303 commented on May 14, 2025

@kumaraditya303
Contributor

About _PyObject_GetDictPtr it is called from PyObject_GenericSetDict where store to ptr is made. It is guarded by CS, but I believe it should use atomic store too. WDYT?

Yes, it should use atomic store with release memory order, created #133988 to fix it.

Reproducer:

from threading import Thread

e = Exception()

def writer():
    for i in range(10000):
        e.__dict__ = {1:2}

def reader():
    for i in range(10000):
        e.__dict__

t1 = Thread(target=writer)
t2 = Thread(target=reader)
t1.start()
t2.start()
t1.join()
t2.join()
added
3.14bugs and security fixes
3.15new features, bugs and security fixes
on May 14, 2025
sergey-miryanov

sergey-miryanov commented on May 14, 2025

@sergey-miryanov
ContributorAuthor
added a commit that references this issue on May 20, 2025

gh-133980: use atomic store in `PyObject_GenericSetDict` (#133988)

ec39fd2
added a commit that references this issue on May 20, 2025

pythongh-133980: use atomic store in `PyObject_GenericSetDict` (pytho…

added a commit that references this issue on May 21, 2025

[3.14] gh-133980: use atomic store in `PyObject_GenericSetDict` (GH-1…

87d7a19

1 remaining item

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

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

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Some operations on managed dict are not safe from memory_order POV · Issue #133980 · python/cpython

      Follow Lee on X/Twitter - Father, Husband, Serial builder creating AI, crypto, games & web tools. We are friends :) AI Will Come To Life!

      Check out: eBank.nz (Art Generator) | Netwrck.com (AI Tools) | Text-Generator.io (AI API) | BitBank.nz (Crypto AI) | ReadingTime (Kids Reading) | RewordGame | BigMultiplayerChess | WebFiddle | How.nz | Helix AI Assistant