10000 Some operations on managed dict are not safe from memory_order POV · Issue #133980 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

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

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
Closed
sergey-miryanov opened this issue May 13, 2025 · 4 comments
Closed

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

sergey-miryanov opened this issue May 13, 2025 · 4 comments
Labels
3.14 bugs and security fixes 3.15 new features, bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-free-threading type-bug An unexpected behavior, bug, or error

Comments

@sergey-miryanov
Copy link
Contributor
sergey-miryanov commented May 13, 2025 •
edited by bedevere-app bot
Loading

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

    cpython/Objects/dictobject.c

    Line 7202 in 9ad0c7b

    Py_VISIT(_PyObject_ManagedDictPointer(obj)->dict);
  2. PyObject_ClearManagedDict

    cpython/Objects/dictobject.c

    Line 7462 in 9ad0c7b

    Py_CLEAR(_PyObject_ManagedDictPointer(obj)->dict);
  3. _PyObject_GetDictPtr

    cpython/Objects/object.c

    Line 1541 in 9ad0c7b

    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

    cpython/Include/internal/pycore_object.h

    Lines 932 to 936 in 9ad0c7b

    _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

    cpython/Objects/dictobject.c

    Line 7225 in 9ad0c7b

    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
@sergey-miryanov sergey-miryanov added the type-bug An unexpected behavior, bug, or error label May 13, 2025
@Eclips4 Eclips4 added interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-free-threading labels May 13, 2025
@colesbury
Copy link
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
Copy link
Contributor Author
sergey-miryanov commented May 14, 2025 •
edited
Loading

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();

@bedevere-app bedevere-app bot mentioned this issue May 14, 2025
Merged
@kumaraditya303
Copy link
Contributor
kumaraditya303 commented May 14, 2025 •
edited
Loading

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()

@kumaraditya303 kumaraditya303 added 3.14 bugs and security fixes 3.15 new features, bugs and security fixes labels May 14, 2025
@sergey-miryanov
Copy link
Contributor Author

@kumaraditya303 Thanks!

kumaraditya303 added a commit that referenced this issue May 20, 2025
@kumaraditya303
gh-133980: use atomic store in PyObject_GenericSetDict (#133988)
ec39fd2
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 20, 2025
@kumaraditya303 @miss-islington
pythongh-133980: use atomic store in PyObject_GenericSetDict (pytho…
4ad0caf 
…nGH-133988)

(cherry picked from commit ec39fd2)

Co-authored-by: Kumar Aditya <kumaraditya@python.org>
@bedevere-app bedevere-app bot mentioned this issue May 20, 2025
Merged
kumaraditya303 added a commit that referenced this issue May 21, 2025
@miss-islington @kumaraditya303
[3.14] gh-133980: use atomic store in PyObject_GenericSetDict (GH-1…
87d7a19 
…33988) (#134354)

gh-133980: use atomic store in `PyObject_GenericSetDict` (GH-133988)
(cherry picked from commit ec39fd2)

Co-authored-by: Kumar Aditya <kumaraditya@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
3.14 bugs and security fixes 3.15 new features, bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-free-threading type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@sergey-miryanov @colesbury @kumaraditya303 @Eclips4
0