Data races found by TSAN on free-threaded build

[ngoldbaum]

I'm running on a mac with clang-19 installed via homebrew. I also have a version of the CPython 3.13 branch compiled in free-threaded mode with TSAN instrumentation. If you use the following cargo build invocation

LDFLAGS="-L/opt/homebrew/opt/llvm/lib" CPPFLAGS="-I/opt/homebrew/opt/llvm/include" RUSTFLAGS="-Zsanitizer=thread" cargo build --all-targets -Zbuild-std --target aarch64-apple-darwin

and then run nox, you'll see some errors reported by TSAN over the course of the tests.

Here is a gist that includes the full nox -s test output: https://gist.github.com/ngoldbaum/b01ea1cd3c6f39fbf2e68938b6489243

Looking through, It looks like almost all are due to uses of Py_CompileString. Apparently Py_CompileString isn't thread-safe. I don't see any upstream issues about that:

test conversions::std::array::tests::array_try_from_fn ... ok
==================
WARNING: ThreadSanitizer: data race (pid=53033)
  Read of size 8 at 0x000103d43f88 by thread T10 (mutexes: write M0):
    #0 bytes_hash bytesobject.c:1588 (libpython3.13t.dylib:arm64+0x74548)
    #1 PyObject_Hash object.c (libpython3.13t.dylib:arm64+0x11e470)
    #2 tuplehash tupleobject.c:328 (libpython3.13t.dylib:arm64+0x163dbc)
    #3 PyObject_Hash object.c (libpython3.13t.dylib:arm64+0x11e470)
    #4 dict_setdefault_ref_lock_held dictobject.c:4228 (libpython3.13t.dylib:arm64+0xf6574)
    #5 PyDict_SetDefaultRef dictobject.c:4332 (libpython3.13t.dylib:arm64+0xf6420)
    #6 _PyCompile_ConstCacheMergeOne compile.c:7606 (libpython3.13t.dylib:arm64+0x274ffc)
    #7 _PyAssemble_MakeCodeObject assemble.c:752 (libpython3.13t.dylib:arm64+0x23d9bc)
    #8 optimize_and_assemble compile.c:7704 (libpython3.13t.dylib:arm64+0x276b38)
    #9 _PyAST_Compile compile.c:451 (libpython3.13t.dylib:arm64+0x27483c)
    #10 Py_CompileStringObject pythonrun.c:1496 (libpython3.13t.dylib:arm64+0x303bbc)
    #11 Py_CompileStringExFlags pythonrun.c:1509 (libpython3.13t.dylib:arm64+0x303c88)
    #12 pyo3_ffi::cpython::pythonrun::Py_CompileString::h15f81e42b5109582 pythonrun.rs:140 (pyo3-ac906cf3b9e6a601:arm64+0x1002aea44)

  Previous write of size 8 at 0x000103d43f88 by thread T4 (mutexes: write M1):
    #0 bytes_hash bytesobject.c:1590 (libpython3.13t.dylib:arm64+0x74574)
    #1 PyObject_Hash object.c (libpython3.13t.dylib:arm64+0x11e470)
    #2 tuplehash tupleobject.c:328 (libpython3.13t.dylib:arm64+0x163dbc)
    #3 PyObject_Hash object.c (libpython3.13t.dylib:arm64+0x11e470)
    #4 dict_setdefault_ref_lock_held dictobject.c:4228 (libpython3.13t.dylib:arm64+0xf6574)
    #5 PyDict_SetDefaultRef dictobject.c:4332 (libpython3.13t.dylib:arm64+0xf6420)
    #6 _PyCompile_ConstCacheMergeOne compile.c:7606 (libpython3.13t.dylib:arm64+0x274ffc)
    #7 _PyAssemble_MakeCodeObject assemble.c:752 (libpython3.13t.dylib:arm64+0x23d9bc)
    #8 optimize_and_assemble compile.c:7704 (libpython3.13t.dylib:arm64+0x276b38)
    #9 _PyAST_Compile compile.c:451 (libpython3.13t.dylib:arm64+0x27483c)
    #10 Py_CompileStringObject pythonrun.c:1496 (libpython3.13t.dylib:arm64+0x303bbc)
    #11 Py_CompileStringExFlags pythonrun.c:1509 (libpython3.13t.dylib:arm64+0x303c88)
    #12 pyo3_ffi::cpython::pythonrun::Py_CompileString::h15f81e42b5109582 pythonrun.rs:140 (pyo3-ac906cf3b9e6a601:arm64+0x1002aea44)

  Location is global '_PyRuntime' at 0x000103d3da80 (libpython3.13t.dylib+0x5e7f88)

These look like races in pyo3 internals:

test pycell::impl_::tests::test_inherited_mutability ... ok
==================
WARNING: ThreadSanitizer: data race (pid=53033)
  Read of size 8 at 0x000300040838 by thread T343 (mutexes: write M0):
    #0 _PyType_AllocNoTrack typeobject.c:2036 (libpython3.13t.dylib:arm64+0x16961c)
    #1 PyType_GenericAlloc typeobject.c:2072 (libpython3.13t.dylib:arm64+0x169588)
    #2 _$LT$pyo3..impl_..pyclass_init..PyNativeTypeInitializer$LT$T$GT$$u20$as$u20$pyo3..impl_..pyclass_init..PyObjectInit$LT$T$GT$$GT$::into_new_object::inner::hdfdf96f099d45698 pyclass_init.rs:52 (pyo3-ac906cf3b9e6a601:arm64+0x1001a2efc)

  Previous write of size 8 at 0x000300040838 by thread T341 (mutexes: write M1):
    #0 type_ready typeobject.c:8219 (libpython3.13t.dylib:arm64+0x1742d4)
    #1 PyType_Ready typeobject.c:8272 (libpython3.13t.dylib:arm64+0x16cef0)
    #2 _PyType_FromMetaclass_impl typeobject.c:4791 (libpython3.13t.dylib:arm64+0x16b3e8)
    #3 PyType_FromSpec typeobject.c:4885 (libpython3.13t.dylib:arm64+0x16b628)
    #4 pyo3::pyclass::create_type_object::PyTypeBuilder::build::h481a95053e080b2c create_type_object.rs:493 (pyo3-ac906cf3b9e6a601:arm64+0x1000bee6c)

SUMMARY: ThreadSanitizer: data race typeobject.c:2036 in _PyType_AllocNoTrack

WARNING: ThreadSanitizer: data race (pid=53249)
  Atomic read of size 8 at 0x00011a040400 by thread T15 (mutexes: write M0):
    #0 _Py_IncRef object.c:341 (libpython3.13t.dylib:arm64+0x11a818)
    #1 pyo3::instance::Py$LT$T$GT$::clone_ref::h2bbb76304febf2d9 <null> (test_proto_methods-a5eae531fea38861:arm64+0x1000b4f28)

  Previous write of size 8 at 0x00011a040400 by thread T14 (mutexes: write M1):
    #0 _Py_NewReference object.c:2483 (libpython3.13t.dylib:arm64+0x123c94)
    #1 _PyType_AllocNoTrack typeobject.c:2061 (libpython3.13t.dylib:arm64+0x169824)
    #2 PyType_GenericAlloc typeobject.c:2072 (libpython3.13t.dylib:arm64+0x169588)
    #3 _PyType_FromMetaclass_impl typeobject.c:4685 (libpython3.13t.dylib:arm64+0x16ae20)
    #4 PyType_FromSpec typeobject.c:4885 (libpython3.13t.dylib:arm64+0x16b628)
    #5 pyo3::pyclass::create_type_object::PyTypeBuilder::build::h066e6d4dea4b3239 <null> (test_proto_methods-a5eae531fea38861:arm64+0x10009a118)

SUMMARY: ThreadSanitizer: data race object.c:341 in _Py_IncRef

[ngoldbaum]

@nascheme let me know privately that I probably want to use Python 3.14 to do this sort of testing - the issue with Py_CompileString (actually it's an issue with the bytes hash) was fixed back in January: python/cpython#128412.

[ngoldbaum]

I now have a more robust way to do this using a docker image with clang-20. Rust uses clang-20 and I think in order to properly detect races maybe we need to make sure all both the Python C code and Rust are compiled with the same TSAN runtime. That's just my guess why I didn't see these with clang-19.

With this combination of tools I see a race inside test_thread_safety_2. Here the race reports from TSAN: https://gist.github.com/ngoldbaum/37d8f3a6573194536130fd408cb82d2a.

Ping @davidhewitt. I don't think this is a soundness issue in pyo3 per se- just how this test is written. Not sure whether the test is still testing what it's supposed to test if we replaced the UnsafeCell with e.g. an AtomicI32.

EDIT: On second thought, this is probably a soundness issue in BorrowFlag.

If anyone else wants to play around, you can run the docker image with

docker run -it ghcr.io/nascheme/cpython-tsan:latest

You'll need to add a symbolic link from cc to clang-20 (we'll fix that in the image soon...)

ln -s /usr/bin/clang-20 /usr/bin/cc

You may also need to do sudo sysctl vm.mmap_rnd_bits=28 on the docker host system, otherwise ASLR may interfere with TSAN leading to segfaults when Python starts. Remember to reset it back to 32 after you're done because this makes your system less secure.

After setting up a nightly rust toolchain on the image and cloning PyO3, make sure you have the rust-src component:

rustup component add rust-src --toolchain nightly-x86_64-unknown-linux-gnu

you can run the PyO3 tests under TSAN with:

LD_LIBRRAY_PATH="$(pyenv prefix)/lib" LDFLAGS="-L$(pyenv prefix)/lib" RUSTFLAGS="-Zsanitizer=thread" cargo test --all-targets -Zbuild-std --target x86_64-unknown-linux-gnu

[ngoldbaum]

Another clue: if I set force_seq_cst_atomics=1 in TSAN_OPTIONS I don't seem to be able to trigger it

[ngoldbaum]

TSAN no longer reports races inside pyo3 with the following diff applied:

diff --git a/src/pycell/impl_.rs b/src/pycell/impl_.rs
index 1b0724d8..9410c0cc 100644
--- a/src/pycell/impl_.rs
+++ b/src/pycell/impl_.rs
@@ -70,8 +70,8 @@ impl BorrowFlag {
                 // last atomic load
                 value,
                 value + 1,
-                Ordering::Relaxed,
-                Ordering::Relaxed,
+                Ordering::AcqRel,
+                Ordering::Acquire,
             ) {
                 Ok(..) => {
                     // value has been successfully incremented, we need an acquire fence
@@ -90,7 +90,7 @@ impl BorrowFlag {
         // impossible to get into a bad state from here so relaxed
         // ordering is fine, the decrement only needs to eventually
         // be visible
-        self.0.fetch_sub(1, Ordering::Relaxed);
+        self.0.fetch_sub(1, Ordering::AcqRel);
     }
 }

I'm not sure if different orderings would also work..