Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions Lib/test/test_free_threading/test_dict.py
Original file line number Diff line number Diff line change
Expand Up @@ -336,5 +336,59 @@ def reader():
with threading_helper.start_threads([t1, t2]):
pass

def test_racing_dict_update_and_method_lookup_with_inline_values(self):
# gh-149816: sub-case 108
# The race below checks that setattr() does not write into a detached
# dict after __dict__ is replaced by another thread.
class Target:
pass

def appender(obj: Target, start: Barrier, iter_times: int) -> None:
start.wait()
index = 0
for _ in range(iter_times):
setattr(obj, f"probe_{index}", index)
index += 1
time.sleep(0)

def replacer(obj: Target, start: Barrier, iter_times: int,
detached_dict_queue: list[tuple[dict, frozenset]]) -> None:
start.wait()
for _ in range(iter_times):
dict_snapshot = obj.__dict__
obj.__dict__ = {}
current_keys = frozenset(dict_snapshot)
detached_dict_queue.append((dict_snapshot, current_keys))
time.sleep(0)

def race(iter_times: int, appender_threads: int,
replacer_threads: int) -> None:
obj = Target()
setattr(obj, "origin", 0)
_ = obj.__dict__ # Access __dict__ to ensure it's initialized

start = Barrier(appender_threads + replacer_threads)
detached_dict_queue: list[tuple[dict, frozenset]] = []
threads = []
for _ in range(appender_threads):
threads.append(Thread(target=appender, args=(obj, start, iter_times),
name="appender"))
for _ in range(replacer_threads):
threads.append(Thread(target=replacer, args=(obj, start, iter_times, detached_dict_queue),
name="replacer"))

with threading_helper.start_threads(threads):
pass

for dict_snapshot, current_keys in detached_dict_queue:
self.assertEqual(set(dict_snapshot), current_keys,
f"Detached dict keys {set(dict_snapshot)} " +
f"do not match current keys {current_keys}")

ITER_TIMES = 50_000
APPENDER_THREADS = 8
REPLACER_THREADS = 8
race(ITER_TIMES, APPENDER_THREADS, REPLACER_THREADS)

if __name__ == "__main__":
unittest.main()
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix a race in free-threaded builds where storing an instance attribute could
write to a detached ``__dict__`` if another thread replaced ``obj.__dict__``
concurrently.
99 changes: 99 additions & 0 deletions Objects/dictobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -7548,10 +7548,108 @@ store_instance_attr_dict(PyObject *obj, PyDictObject *dict, PyObject *name, PyOb
return res;
}

#ifdef Py_GIL_DISABLED
static inline int
store_instance_attr_with_dict_lock_held(PyObject *obj, PyDictObject *dict,
PyObject *name, PyObject *value)
{
ASSERT_WORLD_STOPPED_OR_OBJ_LOCKED(obj);
ASSERT_WORLD_STOPPED_OR_DICT_LOCKED(dict);
int res;
PyDictValues *values = _PyObject_InlineValues(obj);
if (dict->ma_values == values) {
res = store_instance_attr_lock_held(obj, values, name, value);
}
else {
res = _PyDict_SetItem_LockHeld(dict, name, value);
}
return res;
}

typedef enum {
TRY_STORE_ATTR_FAILURE = -1,
TRY_STORE_ATTR_DONE = 0,
TRY_STORE_ATTR_RETRY = 1,
TRY_STORE_ATTR_ALREADY_VALID = 2
} try_store_instance_attr_status;

static try_store_instance_attr_status
try_store_instance_attr_invalid_inline(PyObject *obj, PyObject *name,
PyObject *value, int *res)
{
bool valid;
PyDictObject *dict;
PyDictValues *values = _PyObject_InlineValues(obj);
*res = 0;
Py_BEGIN_CRITICAL_SECTION(obj);
if (!(valid = FT_ATOMIC_LOAD_UINT8(values->valid))) {
dict = _PyObject_GetManagedDict(obj);
if (dict != NULL) {
dict = (PyDictObject *)Py_NewRef(dict);
}
}
Py_END_CRITICAL_SECTION();

if (valid) {
return TRY_STORE_ATTR_ALREADY_VALID;
}

if (dict == NULL) {
// PyObject_GenericGetDict may lock the object,
// so we need to do it outside of the critical section.
dict = (PyDictObject *)PyObject_GenericGetDict(obj, NULL);
if (dict == NULL) {
*res = -1;
return TRY_STORE_ATTR_FAILURE;
}
}

bool success = false;
Py_BEGIN_CRITICAL_SECTION2(obj, dict);
PyDictObject *current_dict = _PyObject_GetManagedDict(obj);
if (current_dict == dict && !(valid = FT_ATOMIC_LOAD_UINT8(values->valid))) {
success = true;
*res = store_instance_attr_with_dict_lock_held(obj, dict, name, value);
}
Py_END_CRITICAL_SECTION2();
Py_DECREF(dict);
if (success) {
return TRY_STORE_ATTR_DONE;
}
return valid ? TRY_STORE_ATTR_ALREADY_VALID : TRY_STORE_ATTR_RETRY;
}
#endif

int
_PyObject_StoreInstanceAttribute(PyObject *obj, PyObject *name, PyObject *value)
{
PyDictValues *values = _PyObject_InlineValues(obj);
#ifdef Py_GIL_DISABLED
uint8_t valid;
int res;
try_store_instance_attr_status try_status = TRY_STORE_ATTR_ALREADY_VALID;
while (!(valid = FT_ATOMIC_LOAD_UINT8(values->valid))) {
// Retry if the managed dict changes before we can lock and validate it.
try_status = try_store_instance_attr_invalid_inline(obj, name, value, &res);
if (try_status != TRY_STORE_ATTR_RETRY) {
break;
}
}
switch (try_status) {
case TRY_STORE_ATTR_FAILURE:
case TRY_STORE_ATTR_DONE:
return res;
case TRY_STORE_ATTR_ALREADY_VALID:
break;
case TRY_STORE_ATTR_RETRY:
// It will happen if the inline values become valid after we read it
// but before we can lock the object.
assert(valid);
break;
default:
Py_UNREACHABLE();
}
#else
if (!FT_ATOMIC_LOAD_UINT8(values->valid)) {
PyDictObject *dict = _PyObject_GetManagedDict(obj);
if (dict == NULL) {
Expand All @@ -7565,6 +7663,7 @@ _PyObject_StoreInstanceAttribute(PyObject *obj, PyObject *name, PyObject *value)
}
return store_instance_attr_dict(obj, dict, name, value);
}
#endif

#ifdef Py_GIL_DISABLED
// We have a valid inline values, at least for now... There are two potential
Expand Down
Loading