GeezerVison is showing github.com — Race in stgdict PyCStructUnionType_update_stginfo under free-threading · Issue #128570 · python/cpython
Race in stgdict PyCStructUnionType_update_stginfo under free-threading #128570DescriptionBug report
Bug description:
I built cpython (3.13 branch) with free-threading and TSAN. The following python code reports TSAN warnings:
import ctypes
import concurrent.futures
import threading
def make_nd_memref_descriptor(rank, dtype):
class MemRefDescriptor(ctypes.Structure):
_fields_ = [
("aligned", ctypes.POINTER(dtype)),
]
return MemRefDescriptor
class F32(ctypes.Structure):
_fields_ = [("f32", ctypes.c_float)]
def test_ctypes():
arg1_ndim = 1
ctp = F32
ctypes.pointer(
ctypes.pointer(make_nd_memref_descriptor(arg1_ndim, ctp)())
)
if __name__ == "__main__":
num_workers = 20
barrier = threading.Barrier(num_workers)
def closure():
barrier.wait()
test_ctypes()
with concurrent.futures.ThreadPoolExecutor(max_workers=num_workers) as executor:
futures = []
for i in range(num_workers):
futures.append(executor.submit(closure))
assert len(list(f.result() for f in futures)) == num_workersTSAN report extract:
WARNING: ThreadSanitizer: data race (pid=264065)
Read of size 4 at 0x7fffb80a0430 by thread T17:
#0 PyCStructUnionType_update_stginfo /project/cpython/./Modules/_ctypes/stgdict.c:438:19 (_ctypes.cpython-313t-x86_64-linux-gnu.so+0x20976) (BuildId: 6a7727f376cee83a3124898fe6eb84e45e3a54bc)
#1 PyCStructType_setattro /project/cpython/./Modules/_ctypes/_ctypes.c:1096:16 (_ctypes.cpython-313t-x86_64-linux-gnu.so+0xc809) (BuildId: 6a7727f376cee83a3124898fe6eb84e45e3a54bc)
#2 PyObject_SetAttr /project/cpython/Objects/object.c:1415:15 (python3.13t+0x294fba) (BuildId: 65e52348ca9319985f9f5e265fd43aab2c867d59)
#3 StructUnionType_init /project/cpython/./Modules/_ctypes/_ctypes.c:685:13 (_ctypes.cpython-313t-x86_64-linux-gnu.so+0xe222) (BuildId: 6a7727f376cee83a3124898fe6eb84e45e3a54bc)
#4 PyCStructType_init /project/cpython/./Modules/_ctypes/_ctypes.c:715:12 (_ctypes.cpython-313t-x86_64-linux-gnu.so+0xc84a) (BuildId: 6a7727f376cee83a3124898fe6eb84e45e3a54bc)
Full traceback
Related to #127945
CPython versions tested on:
3.13
Operating systems tested on:
Linux Activitytom-pytel commented on Jan 13, 2025 PyCStructUnionType_update_stginfo() definitely needs a lock:
import ctypes
import threading
if __name__ == "__main__":
num_workers = 100
def closure(barrier, cls, i):
fields = [(f'i{i}', ctypes.c_int * i)]
barrier.wait()
try:
cls._fields_ = fields
except AttributeError as exc:
pass
while True:
class cls(ctypes.Structure):
pass
barrier = threading.Barrier(num_workers)
threads = []
for i in range(num_workers):
thread = threading.Thread(target=closure, args=(barrier, cls, i))
threads.append(thread)
thread.start()
for thread in threads:
thread.join()
print(cls._fields_)Results:
$ ./python ../../free/check2.py
[('i24', <class '__main__.c_int_Array_24'>)]
[('i98', <class '__main__.c_int_Array_98'>)]
[('i97', <class '__main__.c_int_Array_97'>)]
[('i97', <class '__main__.c_int_Array_97'>)]
[('i98', <class '__main__.c_int_Array_98'>)]
mimalloc: error: double free detected of block 0x20000091050 with size 48
Segmentation fault (core dumped)
$ ./python ../../free/check2.py
[('i99', <class '__main__.c_int_Array_99'>)]
[('i78', <class '__main__.c_int_Array_78'>)]
[('i99', <class '__main__.c_int_Array_99'>)]
Debug memory block at address p=0x20000090b50: API '�'
16102724419182469211 bytes originally requested
The 7 pad bytes at p-7 are not all FORBIDDENBYTE (0xfd):
at p-7: 0xdd *** OUCH
at p-6: 0xdd *** OUCH
at p-5: 0xdd *** OUCH
at p-4: 0xdd *** OUCH
at p-3: 0xdd *** OUCH
at p-2: 0xdd *** OUCH
at p-1: 0xdd *** OUCH
Because memory is corrupted at the start, the count of bytes requested
may be bogus, and checking the trailing pad bytes may segfault.
The 8 pad bytes at tail=0xdf78608b196a2bab are Segmentation fault (core dumped)Also in some other functions like generic_pycdata_new() because that may wind up attempting allocation as the type is being finalized with an assignment to _fields_. This also applies to #128567. ZeroIntensity commented on Jan 13, 2025 I haven't fully decided what to do about stgdict's yet. I'm leaning towards just slapping big critical sections on them, but I need to look through what fields are written so we can go for locks or atomics as needed. tom-pytel commented on Jan 14, 2025
I'm leaning towards just slapping big critical sections on them, but I need to look through what fields are written so we can go for locks or atomics as needed.
A mutex fixed the problem for me. Global instead of per-type because of possibility of A._fields_ = [("b", B)] in one thread simultaneously with B._fields_ = [("a", A)] in another:
$ git diff main
diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c
index ede95bdf98..f6479df571 100644
--- a/Modules/_ctypes/_ctypes.c
+++ b/Modules/_ctypes/_ctypes.c
@@ -699,11 +699,14 @@ StructUnionType_init(PyObject *self, PyObject *args, PyObject *kwds, int isStruc
}
/* copy base info */
+ PyMutex_Lock(&st->mutex);
if (PyCStgInfo_clone(info, baseinfo) < 0) {
+ PyMutex_Unlock(&st->mutex);
return -1;
}
info->flags &= ~DICTFLAG_FINAL; /* clear the 'final' flag in the subclass info */
baseinfo->flags |= DICTFLAG_FINAL; /* set the 'final' flag in the baseclass info */
+ PyMutex_Unlock(&st->mutex);
}
return 0;
}
@@ -3138,7 +3141,9 @@ PyCData_FromBaseObj(ctypes_state *st,
return NULL;
}
+ PyMutex_Lock(&st->mutex);
info->flags |= DICTFLAG_FINAL;
+ PyMutex_Unlock(&st->mutex);
cmem = (CDataObject *)((PyTypeObject *)type)->tp_alloc((PyTypeObject *)type, 0);
if (cmem == NULL) {
return NULL;
@@ -3187,7 +3192,9 @@ PyCData_AtAddress(ctypes_state *st, PyObject *type, void *buf)
return NULL;
}
+ PyMutex_Lock(&st->mutex);
info->flags |= DICTFLAG_FINAL;
+ PyMutex_Unlock(&st->mutex);
pd = (CDataObject *)((PyTypeObject *)type)->tp_alloc((PyTypeObject *)type, 0);
if (!pd) {
@@ -3401,7 +3408,9 @@ generic_pycdata_new(ctypes_state *st,
return NULL;
}
+ PyMutex_Lock(&st->mutex);
info->flags |= DICTFLAG_FINAL;
+ PyMutex_Unlock(&st->mutex);
obj = (CDataObject *)type->tp_alloc(type, 0);
if (!obj)
diff --git a/Modules/_ctypes/ctypes.h b/Modules/_ctypes/ctypes.h
index 45e00a538f..82688f0084 100644
--- a/Modules/_ctypes/ctypes.h
+++ b/Modules/_ctypes/ctypes.h
@@ -75,6 +75,7 @@ typedef struct {
PyObject *error_object_name; // callproc.c
PyObject *PyExc_ArgError;
PyObject *swapped_suffix;
+ PyMutex mutex;
} ctypes_state;Not sure is in all needed places though or fits desired convention so I leave it there if u are messing with it. ZeroIntensity commented on Jan 14, 2025 That's not safe, we need to use critical sections (Py_BEGIN_CRITICAL_SECTION/Py_END_CRITICAL_SECTION) because PyCStgInfo_clone is prone to re-entrancy deadlocks via an object's finalizer. For info->flags, we probably want _Py_atomic_or_uint8 (I think it's a uint8?), and then add some critical section wrapper functions around the stginfo functions.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|