Some operations on managed dict are not safe from memory_order POV

[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
   

   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

• gh-133980: use atomic store in PyObject_GenericSetDict #133988

[colesbury]

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]

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]

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

[sergey-miryanov]

@kumaraditya303 Thanks!