diff --git a/CHANGELOG.md b/CHANGELOG.md index f2f76e0..b0ff890 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,14 @@ exits `3` ("pytest stopped after X of N collected tests"). A JUnit file left by an earlier run is removed first, so a session that dies before writing one is never read with the previous run's counts. +- **Reports inside dependencies' fence-based synchronisation no longer fail runs.** + TSan does not model standalone fences, and cannot see inside glibc. The default + suppressions now cover `oneshot`'s message handover, crossbeam-epoch's reclamation + (0.9.18 and older) and glibc freeing a finished thread's TLS block + (`_dl_deallocate_tls`); each was filed as a race "in your extension". Each entry names + only the dependency's own function, so races in your code that the dependency runs (a + message's `Drop`, a deferred closure, a thread-local destructor) are still reported. + Pinned by `clean/clean-dependency-fences` and `racy/race-in-dependency-callbacks`. ### Added diff --git a/docs/ci.md b/docs/ci.md index f457c78..749702b 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -163,12 +163,23 @@ killed after a short grace period and the crash is still reported. Suppressions, applied in this order: -1. **ftcheck's own** (`ftcheck/ci/suppressions/rust.supp`) — known deliberate races in - common Rust dependencies, each justified by that dependency's own source and written - as `race_top:`, so a real race in your code that merely runs inside the dependency is - still reported. Today: crossbeam-deque's buffer, as used by rayon's job queue. The - `clean/clean-rayon` fixture proves both that it is needed (without it: exit 1) and that - it holds. `FTCHECK_NO_DEFAULT_SUPPRESSIONS=1` turns them off, to audit what they hide. +1. **ftcheck's own** (`ftcheck/ci/suppressions/rust.supp`) — reports inside common + Rust dependencies that are not races, each justified by that dependency's own source: + - crossbeam-deque's buffer, as used by rayon's job queue — a documented deliberate race; + - `oneshot`'s message handover, ordered by a standalone `fence(Acquire)`; + - crossbeam-epoch's reclamation (0.9.18 and older), ordered by `SeqCst` fences; + - glibc freeing a finished thread's TLS block (`_dl_deallocate_tls`), ordered inside + glibc, which is not instrumented. + + Entries are `race_top:` where TSan allows, matching only the frame where the access + happened. Where that frame is an interceptor (`free`, `memcpy`) they are `race:` on a + function of the dependency that runs none of your code. Either way, a real race in + your code that runs inside the dependency — a message's `Drop` run by `oneshot`, a + closure deferred to crossbeam-epoch, a thread-local destructor — is still reported. + `clean/clean-rayon` and `clean/clean-dependency-fences` prove the entries are needed + (without them: exit 1) and that they hold; `racy/race-in-dependency-callbacks` proves + they stop at the dependency's code. `FTCHECK_NO_DEFAULT_SUPPRESSIONS=1` turns them + off, to audit what they hide. 2. **The image's CPython list** (`/work/tsan_suppressions/cpython.txt`), through `$FTCHECK_TSAN_SUPPRESSIONS`. 3. **Yours**, with `--suppressions FILE`. diff --git a/docs/limitations.md b/docs/limitations.md index d7d8bb6..e1a1123 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -60,10 +60,21 @@ Known gaps, each still open: single-threaded baseline never sees the mutator's transient state. The finding says when mutators were running, but does not check whether one caused it. A mutator must keep the shared inputs valid at every instant. -- **Some dependency-internal TSan reports still fail runs.** Beyond the suppressed - crossbeam-deque race, reports inside dependencies' fence-based synchronisation (which - TSan does not model) and glibc's thread-local teardown can be filed as "in your - extension" with no frame of yours on either access. +- **Other dependencies' fence-based synchronisation can still fail runs.** TSan does + not model standalone fences. ftcheck suppresses the reports it knows of in common + dependencies — crossbeam-deque, `oneshot`, crossbeam-epoch 0.9.18 and older, and + glibc's thread-local teardown (see [ci.md](ci.md#attribution-yours-reached-from-yours-or-not-yours)) — but a dependency + that synchronises the same way and is not on that list is still filed as "in your + extension". Add a `--suppressions` file for it, scoped to the dependency's frames. +- **The glibc teardown suppression can hide a use-after-free.** The report's own frame + is the `free` interceptor, so the entry matches `_dl_deallocate_tls` anywhere on the + stack. A genuine race on a finished thread's thread-local, reached by another thread + through a pointer that escaped it, is hidden with the false report. Safe Rust cannot + express that; `unsafe` code can. +- **Your own accesses ordered only by such a fence are reported.** Data you write before + a `oneshot` send and read after the receive is ordered by the channel's fence, which + TSan cannot see, so the access looks unordered in your code. Suppressing it would hide + real races, so ftcheck does not. ## The lint on public projects diff --git a/fixtures/clean/clean-dependency-fences/Cargo.lock b/fixtures/clean/clean-dependency-fences/Cargo.lock new file mode 100644 index 0000000..01d01f5 --- /dev/null +++ b/fixtures/clean/clean-dependency-fences/Cargo.lock @@ -0,0 +1,155 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +version = 4 + +[[package]] +name = "clean-dependency-fences" +version = "0.0.0" +dependencies = [ + "crossbeam-epoch", + "oneshot", + "pyo3", +] + +[[package]] +name = "crossbeam-epoch" +version = "0.9.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5b82ac4a3c2ca9c3460964f020e1402edd5753411d7737aa39c3714ad1b5420e" +dependencies = [ + "crossbeam-utils", +] + +[[package]] +name = "crossbeam-utils" +version = "0.8.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a31eee39dddec8330830986fcd7625edb5a24ec90ea038215273bbc3adb08ac6" + +[[package]] +name = "heck" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" + +[[package]] +name = "libc" +version = "0.2.189" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3eaf3ede3fee6db1a4c2ee091bf8a8b4dccdc6d17f656fb07896ee72867612f2" + +[[package]] +name = "once_cell" +version = "1.21.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9f7c3e4beb33f85d45ae3e3a1792185706c8e16d043238c593331cc7cd313b50" + +[[package]] +name = "oneshot" +version = "0.1.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "269bca4c2591a28585d6bf10d9ed0332b7d76900a1b02bec41bdc3a2cdcda107" + +[[package]] +name = "portable-atomic" +version = "1.15.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "05c8b63e8d9609db387f0324918f81d68fe27748f084ef092fb35954d0539a85" + +[[package]] +name = "proc-macro2" +version = "1.0.107" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "985e7ec9bb745e6ce6535b544d84d6cd6f7ad8bd711c398938ae983b91a766d9" +dependencies = [ + "unicode-ident", +] + +[[package]] +name = "pyo3" +version = "0.29.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4688ddedf473e32662b9b067670129a8afb8c18e351482c70d62ba4a88171e8b" +dependencies = [ + "libc", + "once_cell", + "portable-atomic", + "pyo3-build-config", + "pyo3-ffi", + "pyo3-macros", +] + +[[package]] +name = "pyo3-build-config" +version = "0.29.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f41027e41b4bd03f6e60f9f417fe24a6341a6bb744edd62b6f709f2a52ea30e9" +dependencies = [ + "target-lexicon", +] + +[[package]] +name = "pyo3-ffi" +version = "0.29.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e591a95526fead067432c3b3a33fc74770b87b1e04e73671090d9c2055a2b327" +dependencies = [ + "libc", + "pyo3-build-config", +] + +[[package]] +name = "pyo3-macros" +version = "0.29.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "73225868fc1cd84eef2c3c230ddb91273bf1de46aeb8a4248da76d32a0924a1c" +dependencies = [ + "proc-macro2", + "pyo3-macros-backend", + "quote", + "syn", +] + +[[package]] +name = "pyo3-macros-backend" +version = "0.29.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "571575aa3749fa6216757dd47d2a3e7ef360f329a40f0666a9fbd14889024952" +dependencies = [ + "heck", + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "quote" +version = "1.0.47" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1fbf4db142a473a8d80c26bbf18454ed458bf8d26c8219c331daecfdbd079001" +dependencies = [ + "proc-macro2", +] + +[[package]] +name = "syn" +version = "2.0.119" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "872831b642d1a07999a962a351ed35b955ea2cfc8f3862091e2a240a84f17297" +dependencies = [ + "proc-macro2", + "quote", + "unicode-ident", +] + +[[package]] +name = "target-lexicon" +version = "0.13.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "adb6935a6f5c20170eeceb1a3835a49e12e19d792f6dd344ccc76a985ca5a6ca" + +[[package]] +name = "unicode-ident" +version = "1.0.26" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d245f478577f809a851594d02313b640fb437e0bb33866753cff937863096954" diff --git a/fixtures/clean/clean-dependency-fences/Cargo.toml b/fixtures/clean/clean-dependency-fences/Cargo.toml new file mode 100644 index 0000000..8eadcae --- /dev/null +++ b/fixtures/clean/clean-dependency-fences/Cargo.toml @@ -0,0 +1,17 @@ +[package] +name = "clean-dependency-fences" +version = "0.0.0" +edition = "2021" +publish = false + +[lib] +name = "clean_dependency_fences" +crate-type = ["cdylib", "rlib"] + +[dependencies] +pyo3 = { version = "0.29", features = ["extension-module"] } +oneshot = "0.1" +# Pinned to the last release before crossbeam-epoch modelled its fences for +# ThreadSanitizer (0.9.19 and 0.9.21 did, under `crossbeam_sanitize_thread`). +# Projects that lock an older release still get these reports. +crossbeam-epoch = "=0.9.18" diff --git a/fixtures/clean/clean-dependency-fences/expected.toml b/fixtures/clean/clean-dependency-fences/expected.toml new file mode 100644 index 0000000..1a1f94c --- /dev/null +++ b/fixtures/clean/clean-dependency-fences/expected.toml @@ -0,0 +1,3 @@ +race = false +rules = [] +justification = "Values handed over by oneshot, memory reclaimed by crossbeam-epoch 0.9.18, and short-lived threads detached after they finish using a thread-local with a destructor, all called from many Python threads, each call on its own data. Every access is ordered, by fences TSan does not model (oneshot, crossbeam-epoch) or inside uninstrumented glibc (the finished thread's TLS block freed in _dl_deallocate_tls). Without ftcheck's default suppressions TSan reports all three; this fixture pins that the suppressions hold." diff --git a/fixtures/clean/clean-dependency-fences/ftcheck.toml b/fixtures/clean/clean-dependency-fences/ftcheck.toml new file mode 100644 index 0000000..8ec0fc3 --- /dev/null +++ b/fixtures/clean/clean-dependency-fences/ftcheck.toml @@ -0,0 +1,5 @@ +# Small counts: every call spawns threads, and the stress budget is shared. +[stress.args] +"clean_dependency_fences.oneshot_roundtrip" = ["4", "16"] +"clean_dependency_fences.epoch_churn" = ["2", "4"] +"clean_dependency_fences.short_lived_threads" = ["2", "8"] diff --git a/fixtures/clean/clean-dependency-fences/src/lib.rs b/fixtures/clean/clean-dependency-fences/src/lib.rs new file mode 100644 index 0000000..262c6b0 --- /dev/null +++ b/fixtures/clean/clean-dependency-fences/src/lib.rs @@ -0,0 +1,127 @@ +//! Correct: three patterns whose synchronisation ThreadSanitizer cannot see. +//! +//! Every access below is ordered, but not in a way TSan models, so without +//! ftcheck's default suppressions each one is reported as a data race: +//! +//! - `oneshot` hands the message over with a relaxed load of the channel state +//! followed by a standalone `fence(Acquire)`. TSan does not model standalone +//! fences, so the sender's write of the message and the receiver's read (and +//! its free of the channel) look unordered. +//! - `crossbeam-epoch` reclaims memory once every pinned thread has moved past +//! an epoch, which it proves with `SeqCst` fences. TSan sees the collector +//! free a garbage bag, or a finished thread's `Local`, that another thread +//! read without a happens-before edge. +//! - glibc frees a finished, detached thread's TLS block in `_dl_deallocate_tls`, +//! on the thread that drops the handle. glibc is not instrumented, so nothing +//! TSan sees orders the finished thread's own thread-local accesses before +//! that free. +//! +//! None of it is the extension's code. Each call works on its own data. + +use crossbeam_epoch::{self as epoch, Atomic, Owned}; +use pyo3::prelude::*; +use std::cell::RefCell; +use std::sync::atomic::Ordering::AcqRel; +use std::sync::{mpsc, Arc}; +use std::thread; +use std::time::Duration; + +/// Large enough that moving it through the channel is a `memcpy`. +type Payload = [u64; 32]; + +/// A worker thread sends one value back; the caller polls, then blocks. +#[pyfunction] +fn oneshot_roundtrip(py: Python<'_>, n: usize) -> u64 { + py.detach(|| { + let mut total = 0; + for i in 0..n { + let (tx, rx) = oneshot::channel::(); + let worker = thread::spawn(move || tx.send([i as u64; 32]).unwrap()); + let value = if i % 2 == 0 { + // try_recv: relaxed load, then fence(Acquire) once it sees MESSAGE. + loop { + match rx.try_recv() { + Ok(v) => break v, + Err(oneshot::TryRecvError::Empty) => thread::yield_now(), + Err(e) => panic!("{e}"), + } + } + } else { + rx.recv().unwrap() + }; + worker.join().unwrap(); + total += value[31]; + } + total + }) +} + +/// Threads swap a shared pointer and retire the old value through the epoch +/// collector, then exit, so their `Local`s are reclaimed by whoever collects. +#[pyfunction] +fn epoch_churn(py: Python<'_>, n: usize) -> usize { + py.detach(|| { + let shared = Arc::new(Atomic::new([0u64; 8])); + let workers: Vec<_> = (0..n) + .map(|t| { + let shared = Arc::clone(&shared); + thread::spawn(move || { + for i in 0..64u64 { + let guard = epoch::pin(); + let old = shared.swap(Owned::new([t as u64 ^ i; 8]), AcqRel, &guard); + // SAFETY: `old` is unlinked, and only reachable by + // threads pinned before the swap. + unsafe { guard.defer_destroy(old) }; + } + epoch::pin().flush(); + }) + }) + .collect(); + let count = workers.len(); + for w in workers { + w.join().unwrap(); + } + let guard = epoch::pin(); + // SAFETY: every worker has been joined; nothing else holds the pointer. + unsafe { drop(shared.swap(epoch::Shared::null(), AcqRel, &guard).into_owned()) }; + count + }) +} + +thread_local! { + static SCRATCH: RefCell> = const { RefCell::new(Vec::new()) }; +} + +/// Short-lived threads that use a thread-local with a destructor. Each handle is +/// dropped once its thread has finished, which detaches it: glibc then frees the +/// finished thread's TLS block here, in `_dl_deallocate_tls`. +#[pyfunction] +fn short_lived_threads(py: Python<'_>, n: usize) -> u64 { + py.detach(|| { + let (done, finished) = mpsc::channel(); + let workers: Vec<_> = (0..n) + .map(|i| { + let done = done.clone(); + thread::spawn(move || { + let sum = SCRATCH.with(|s| { + s.borrow_mut().push(i as u64); + s.borrow().iter().sum::() + }); + done.send(sum).unwrap(); + }) + }) + .collect(); + let total = (0..n).map(|_| finished.recv().unwrap()).sum(); + // Give each thread time to run its TLS destructors and exit. + thread::sleep(Duration::from_millis(20)); + drop(workers); + total + }) +} + +#[pymodule] +fn clean_dependency_fences(m: &Bound<'_, PyModule>) -> PyResult<()> { + m.add_function(wrap_pyfunction!(oneshot_roundtrip, m)?)?; + m.add_function(wrap_pyfunction!(epoch_churn, m)?)?; + m.add_function(wrap_pyfunction!(short_lived_threads, m)?) +} diff --git a/fixtures/clean/clean-dependency-fences/tests/test_threads.py b/fixtures/clean/clean-dependency-fences/tests/test_threads.py new file mode 100644 index 0000000..2ffef3d --- /dev/null +++ b/fixtures/clean/clean-dependency-fences/tests/test_threads.py @@ -0,0 +1,9 @@ +"""Run by `ftcheck ci` under pytest-run-parallel: this body runs in N threads at once.""" +import clean_dependency_fences as m + + +def test_dependency_synchronisation_from_many_threads(): + for _ in range(5): + assert m.oneshot_roundtrip(8) == sum(range(8)) + assert m.epoch_churn(4) == 4 + assert m.short_lived_threads(4) == sum(range(4)) diff --git a/fixtures/racy/race-in-dependency-callbacks/Cargo.lock b/fixtures/racy/race-in-dependency-callbacks/Cargo.lock new file mode 100644 index 0000000..ba0233b --- /dev/null +++ b/fixtures/racy/race-in-dependency-callbacks/Cargo.lock @@ -0,0 +1,155 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +version = 4 + +[[package]] +name = "crossbeam-epoch" +version = "0.9.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5b82ac4a3c2ca9c3460964f020e1402edd5753411d7737aa39c3714ad1b5420e" +dependencies = [ + "crossbeam-utils", +] + +[[package]] +name = "crossbeam-utils" +version = "0.8.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a31eee39dddec8330830986fcd7625edb5a24ec90ea038215273bbc3adb08ac6" + +[[package]] +name = "heck" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" + +[[package]] +name = "libc" +version = "0.2.189" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3eaf3ede3fee6db1a4c2ee091bf8a8b4dccdc6d17f656fb07896ee72867612f2" + +[[package]] +name = "once_cell" +version = "1.21.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9f7c3e4beb33f85d45ae3e3a1792185706c8e16d043238c593331cc7cd313b50" + +[[package]] +name = "oneshot" +version = "0.1.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "269bca4c2591a28585d6bf10d9ed0332b7d76900a1b02bec41bdc3a2cdcda107" + +[[package]] +name = "portable-atomic" +version = "1.15.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "05c8b63e8d9609db387f0324918f81d68fe27748f084ef092fb35954d0539a85" + +[[package]] +name = "proc-macro2" +version = "1.0.107" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "985e7ec9bb745e6ce6535b544d84d6cd6f7ad8bd711c398938ae983b91a766d9" +dependencies = [ + "unicode-ident", +] + +[[package]] +name = "pyo3" +version = "0.29.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4688ddedf473e32662b9b067670129a8afb8c18e351482c70d62ba4a88171e8b" +dependencies = [ + "libc", + "once_cell", + "portable-atomic", + "pyo3-build-config", + "pyo3-ffi", + "pyo3-macros", +] + +[[package]] +name = "pyo3-build-config" +version = "0.29.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f41027e41b4bd03f6e60f9f417fe24a6341a6bb744edd62b6f709f2a52ea30e9" +dependencies = [ + "target-lexicon", +] + +[[package]] +name = "pyo3-ffi" +version = "0.29.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e591a95526fead067432c3b3a33fc74770b87b1e04e73671090d9c2055a2b327" +dependencies = [ + "libc", + "pyo3-build-config", +] + +[[package]] +name = "pyo3-macros" +version = "0.29.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "73225868fc1cd84eef2c3c230ddb91273bf1de46aeb8a4248da76d32a0924a1c" +dependencies = [ + "proc-macro2", + "pyo3-macros-backend", + "quote", + "syn", +] + +[[package]] +name = "pyo3-macros-backend" +version = "0.29.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "571575aa3749fa6216757dd47d2a3e7ef360f329a40f0666a9fbd14889024952" +dependencies = [ + "heck", + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "quote" +version = "1.0.47" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1fbf4db142a473a8d80c26bbf18454ed458bf8d26c8219c331daecfdbd079001" +dependencies = [ + "proc-macro2", +] + +[[package]] +name = "race-in-dependency-callbacks" +version = "0.0.0" +dependencies = [ + "crossbeam-epoch", + "oneshot", + "pyo3", +] + +[[package]] +name = "syn" +version = "2.0.119" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "872831b642d1a07999a962a351ed35b955ea2cfc8f3862091e2a240a84f17297" +dependencies = [ + "proc-macro2", + "quote", + "unicode-ident", +] + +[[package]] +name = "target-lexicon" +version = "0.13.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "adb6935a6f5c20170eeceb1a3835a49e12e19d792f6dd344ccc76a985ca5a6ca" + +[[package]] +name = "unicode-ident" +version = "1.0.26" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d245f478577f809a851594d02313b640fb437e0bb33866753cff937863096954" diff --git a/fixtures/racy/race-in-dependency-callbacks/Cargo.toml b/fixtures/racy/race-in-dependency-callbacks/Cargo.toml new file mode 100644 index 0000000..ce25be6 --- /dev/null +++ b/fixtures/racy/race-in-dependency-callbacks/Cargo.toml @@ -0,0 +1,14 @@ +[package] +name = "race-in-dependency-callbacks" +version = "0.0.0" +edition = "2021" +publish = false + +[lib] +name = "race_in_dependency_callbacks" +crate-type = ["cdylib", "rlib"] + +[dependencies] +pyo3 = { version = "0.29", features = ["extension-module"] } +oneshot = "0.1" +crossbeam-epoch = "=0.9.18" diff --git a/fixtures/racy/race-in-dependency-callbacks/expected.toml b/fixtures/racy/race-in-dependency-callbacks/expected.toml new file mode 100644 index 0000000..0bd4254 --- /dev/null +++ b/fixtures/racy/race-in-dependency-callbacks/expected.toml @@ -0,0 +1,4 @@ +race = true +rules = ["FT001"] +symbols = ["count_dropped", "deferred", "count_retired"] +justification = "Unsynchronised read-modify-writes of shared counters in code the extension hands to a dependency: a message's Drop run by oneshot, a closure deferred to crossbeam-epoch's collector, and a thread-local destructor run at thread exit. The stacks pass through the same dependencies whose own reports ftcheck suppresses, so this pins that the suppressions stop at the dependency's code. FT001 sees only the closure, which is written inside the #[pyfunction]." diff --git a/fixtures/racy/race-in-dependency-callbacks/ftcheck.toml b/fixtures/racy/race-in-dependency-callbacks/ftcheck.toml new file mode 100644 index 0000000..19eff2c --- /dev/null +++ b/fixtures/racy/race-in-dependency-callbacks/ftcheck.toml @@ -0,0 +1,4 @@ +[stress.args] +"race_in_dependency_callbacks.undelivered" = ["50"] +"race_in_dependency_callbacks.deferred" = ["50"] +"race_in_dependency_callbacks.thread_exit" = ["2"] diff --git a/fixtures/racy/race-in-dependency-callbacks/src/lib.rs b/fixtures/racy/race-in-dependency-callbacks/src/lib.rs new file mode 100644 index 0000000..0fcbb6b --- /dev/null +++ b/fixtures/racy/race-in-dependency-callbacks/src/lib.rs @@ -0,0 +1,91 @@ +//! Racy: the extension's own code, run by a dependency, races. +//! +//! ftcheck suppresses the reports that `oneshot`, `crossbeam-epoch` and glibc's +//! thread teardown produce under ThreadSanitizer (see `clean-dependency-fences`). +//! Each of those dependencies also calls back into the extension: `oneshot` +//! drops an undelivered message, the epoch collector runs deferred closures, +//! and thread exit runs thread-local destructors. The code in those callbacks +//! is the extension's, so a race in it must still be reported, even though +//! every stack it appears on passes through the dependency. +//! +//! Each callback below does an unsynchronised read-modify-write of a counter +//! shared by every thread. + +use crossbeam_epoch as epoch; +use pyo3::prelude::*; +use std::thread; + +static mut DROPPED: u64 = 0; +static mut RECLAIMED: u64 = 0; +static mut RETIRED: u64 = 0; + +/// Dropped by `oneshot` itself when the receiver goes away first. +struct Receipt([u64; 32]); + +impl Drop for Receipt { + fn drop(&mut self) { + count_dropped(self.0[0]); + } +} + +// The racy accesses are in functions of their own, so each race is reported +// under its own name rather than as another `drop`. +#[inline(never)] +fn count_dropped(n: u64) { + unsafe { DROPPED += n }; +} + +#[pyfunction] +fn undelivered(n: usize) -> usize { + for _ in 0..n { + let (tx, rx) = oneshot::channel::(); + tx.send(Receipt([1; 32])).unwrap(); + drop(rx); + } + n +} + +#[pyfunction] +fn deferred(n: usize) -> usize { + for _ in 0..n { + let guard = epoch::pin(); + guard.defer(|| unsafe { RECLAIMED += 1 }); + guard.flush(); + } + n +} + +struct Retire; + +impl Drop for Retire { + fn drop(&mut self) { + count_retired(); + } +} + +#[inline(never)] +fn count_retired() { + unsafe { RETIRED += 1 }; +} + +thread_local! { + static ON_EXIT: Retire = const { Retire }; +} + +#[pyfunction] +fn thread_exit(py: Python<'_>, n: usize) -> usize { + py.detach(|| { + let workers: Vec<_> = (0..n).map(|_| thread::spawn(|| ON_EXIT.with(|_| ()))).collect(); + for w in workers { + w.join().unwrap(); + } + }); + n +} + +#[pymodule] +fn race_in_dependency_callbacks(m: &Bound<'_, PyModule>) -> PyResult<()> { + m.add_function(wrap_pyfunction!(undelivered, m)?)?; + m.add_function(wrap_pyfunction!(deferred, m)?)?; + m.add_function(wrap_pyfunction!(thread_exit, m)?) +} diff --git a/fixtures/racy/race-in-dependency-callbacks/tests/test_threads.py b/fixtures/racy/race-in-dependency-callbacks/tests/test_threads.py new file mode 100644 index 0000000..9c9f222 --- /dev/null +++ b/fixtures/racy/race-in-dependency-callbacks/tests/test_threads.py @@ -0,0 +1,9 @@ +"""Run by `ftcheck ci` under pytest-run-parallel: this body runs in N threads at once.""" +import race_in_dependency_callbacks as m + + +def test_callbacks_from_many_threads(): + for _ in range(20): + assert m.undelivered(50) == 50 + assert m.deferred(50) == 50 + assert m.thread_exit(2) == 2 diff --git a/python/ftcheck/ci/suppressions/rust.supp b/python/ftcheck/ci/suppressions/rust.supp index f456c03..236ed31 100644 --- a/python/ftcheck/ci/suppressions/rust.supp +++ b/python/ftcheck/ci/suppressions/rust.supp @@ -1,9 +1,16 @@ # ftcheck's default ThreadSanitizer suppressions for Rust dependencies. # -# Every entry is justified by the dependency's own documentation, and is as -# narrow as TSan allows: `race_top:` matches only the frame where the access -# happened, so a real race in your code that merely runs inside the -# dependency (a closure executed by a rayon job, say) is still reported. +# Every entry is justified by the dependency's own source, and is as narrow as +# TSan allows. `race_top:` matches only the frame where the access happened, so +# a real race in your code that merely runs inside the dependency (a closure +# executed by a rayon job, say) is still reported. It is the default. +# +# `race:` matches any frame of either access, and is used only where the access +# TSan reports is an interceptor (`free`, `__tsan_memcpy`) that `race_top:` +# cannot name narrowly. Each such entry names a function that runs none of the +# caller's code, so no frame of yours can sit above it: a message's `Drop`, a +# deferred closure and a thread-local destructor all run in other frames, and +# races in them are still reported (see fixtures/racy/race-in-dependency-callbacks). # crossbeam-deque's Chase-Lev buffer, as used by rayon's job queue. # crossbeam-deque 0.8 src/deque.rs, Buffer::write / Buffer::read: @@ -14,3 +21,42 @@ # dependency's own documented race, not the calling project's code. race_top:write_volatile> race_top:read_volatile> + +# oneshot 0.1: the message handover. Sender::send writes the message, then sets +# the state with Release; the receiver loads the state Relaxed and then issues a +# standalone fence (src/lib.rs, Receiver::try_recv / recv): +# "We need to establish a happens-before relationship with the sender's write +# of the MESSAGE state [...] This is a separate fence instead of part of the +# load above" +# TSan does not model standalone fences, so the sender's write of the message +# looks unordered with the receiver's read of it and its free of the channel. +# Channel::write_message is `ptr::write` of the message into the slot; the Box +# drop of a Channel frees the allocation (its fields are MaybeUninit and atomics, +# so it drops no message and runs no user code). +race:oneshot::*::write_message:: +race:drop +race:drop + +# glibc thread teardown: when a finished thread is detached (its JoinHandle +# dropped), glibc frees the thread's TLS block in `_dl_deallocate_tls`, on the +# detaching thread. The thread's exit is handed over inside glibc, which is not +# instrumented, and detaching is not a synchronisation TSan knows of, so the free +# looks unordered with every access the finished thread made to its own +# thread-locals: in its body, in TLS destructors, in std's destructor list. +# The frame is glibc's deallocation of a finished thread's TLS; it runs no user +# code. It would also hide another thread using a leaked pointer to that TLS +# after the thread ended, which safe Rust cannot express. +race:_dl_deallocate_tls diff --git a/tests/test_default_suppressions.py b/tests/test_default_suppressions.py new file mode 100644 index 0000000..8e2a6ed --- /dev/null +++ b/tests/test_default_suppressions.py @@ -0,0 +1,239 @@ +# SPDX-License-Identifier: MIT OR Apache-2.0 +"""ftcheck's default TSan suppressions: what they hide, and what they must not. + +TSan's matching is reproduced here closely enough to check each entry against +synthetic reports shaped like the ones TSan prints. A `race_top:` entry is +checked against the top frame of each access; a `race:` entry against every +frame of every access and of every thread-creation stack. Either way, one +matching stack suppresses the whole report. + +The sanitizer ground truth (`clean/clean-dependency-fences`, +`racy/race-in-dependency-callbacks`) proves the same thing under a real TSan. +""" +import re + +from ftcheck.ci.pipeline import DEFAULT_SUPPRESSIONS + +# `race:` matches any frame, so an entry that names a function which calls back +# into other code would hide races in that code too. Each one here was checked +# against the dependency's source: the function it names runs none of the +# caller's code. Adding a `race:` entry means adding it here, deliberately. +REVIEWED_WHOLE_STACK_ENTRIES = { + "race:oneshot::*::write_message::", + "race:drop", + "race:_dl_deallocate_tls", +} + + +def _entries(): + lines = DEFAULT_SUPPRESSIONS.read_text().splitlines() + return [(i, ln) for i, ln in enumerate(lines) if ln.strip() and not ln.startswith("#")], lines + + +def _matches(template: str, text: str) -> bool: + """sanitizer_common's TemplateMatch: substring, `*` wildcard, `^`/`$` anchors.""" + if not text: + return False + start = template.startswith("^") + end = template.endswith("$") + body = template[start: len(template) - end if end else None] + pattern = ".*".join(re.escape(part) for part in body.split("*")) + return re.search(("^" if start else "") + pattern + ("$" if end else ""), text) is not None + + +def _suppressed(accesses, threads=()): + """accesses: stacks of the two accesses, top frame first; frames are function names.""" + entries = [ln for _, ln in _entries()[0]] + for entry in entries: + kind, _, template = entry.partition(":") + if kind == "race_top": + frames = [stack[0] for stack in accesses if stack] + else: + frames = [f for stack in (*accesses, *threads) for f in stack] + if any(_matches(template, f) for f in frames): + return entry + return None + + +def test_every_default_suppression_is_narrow_and_justified(): + """`race_top:` by default; `race:` only when reviewed. Each preceded by a comment.""" + entries, lines = _entries() + assert entries, "the default file must not be empty" + for i, line in entries: + assert line.startswith(("race_top:", "race:")), line + if line.startswith("race:"): + assert line in REVIEWED_WHOLE_STACK_ENTRIES, ( + f"{line}: a `race:` entry matches every frame; review it and list it here" + ) + assert any(lines[j].startswith("#") for j in range(max(0, i - 3), i)), line + + +def test_the_matcher_follows_tsan(): + assert _matches("drop, alloc::alloc::Global>") + assert _matches("oneshot::*::write_message::", "f>") + assert not _matches("^write_message", "oneshot::write_message") + assert _matches("free$", "free") and not _matches("free$", "freed") + + +# --- what the entries hide: the dependencies' own reports ---------------------- + +_USER = ["fixture::caller", "::__pymethod_call__"] + +_ONESHOT_WRITE = [ + "__tsan_memcpy", + "write<[u64; 32]>", + "{closure#0}<[u64; 32]>", + "with_message_mut<[u64; 32], oneshot::{impl#9}::write_message::{closure_env#0}<[u64; 32]>>", + "write_message<[u64; 32]>", + ">::send", + "{closure#0}", +] +_ONESHOT_TAKE = [ + "__tsan_memcpy", + "read>", + "take_message<[u64; 32]>", + ">::try_recv", + *_USER, +] +_ONESHOT_FREE = [ + "free", + "dealloc", + "__rustc::__rdl_dealloc", + "dealloc", + "drop, alloc::alloc::Global>", + "drop_in_place, alloc::alloc::Global>>", + "dealloc<[u64; 32]>", + ">::recv", + *_USER, +] +_BAG_READ = [ + "read", + "assume_init_read", + ">::try_pop_if", + "::collect", +] +_NODE_FREE = [ + "free", + "dealloc", + "drop, alloc::alloc::Global>", + "::collect", +] +_LOCAL_LOAD = [ + "atomic_load", + "load", + "load", + "next", + "::try_advance", +] +_LOCAL_FREE = [ + "free", + "dealloc", + "drop", + "drop_in_place>", + "::drop", + "::collect", +] +_TLS_FREE = [ + "free", + "_dl_deallocate_tls", + "::drop", + "drop_in_place", + "drop_in_place>", + "core::ptr::drop_in_place::>", + *_USER, +] +_TLS_OWN_ACCESS = [ + "replace", + "try_borrow_mut>", + "{closure#0}", + "with>, fixture::worker::{closure_env#0}>", +] +_TLS_DTOR_LIST = [ + "replace", + "try_borrow_mut>", + "run", + "std::sys::thread_local::guard::key::enable::run", +] + + +def test_the_crossbeam_deque_buffer_race_is_suppressed_at_its_top_frame_only(): + slot = "write_volatile>" + assert _suppressed([[slot, ">::write"], _USER]) + assert _suppressed([["::fill", slot], ["::fill"]]) is None + + +def test_oneshot_message_handover_is_suppressed(): + assert _suppressed([_ONESHOT_TAKE, _ONESHOT_WRITE]) + assert _suppressed([_ONESHOT_FREE, _ONESHOT_WRITE]) + + +def test_crossbeam_epoch_reclamation_is_suppressed(): + assert _suppressed([_NODE_FREE, _BAG_READ]) + assert _suppressed([_LOCAL_FREE, _LOCAL_LOAD]) + + +def test_glibc_tls_teardown_is_suppressed(): + assert _suppressed([_TLS_FREE, _TLS_DTOR_LIST]) + assert _suppressed([_TLS_FREE, _TLS_OWN_ACCESS]) + + +# --- what they must not hide: your code, run by the dependency ----------------- + +_RECEIPT_DROP = [ + "::drop", + "drop_in_place", + "assume_init_drop", + "{closure#0}", + "with_message_mut>", + "drop_message", + " as core::ops::drop::Drop>::drop", + "drop_in_place>", + *_USER, +] +_DEFERRED_CLOSURE = [ + "{closure#0}", + "::new::call::", + "call", + "drop", + "drop_in_place", + "drop_in_place", + "drop", + "::collect", + "::flush", + *_USER, +] +_BAG_DROP_IN_LOCAL = [ + "{closure#0}", + "::new::call::", + "drop_in_place", + "drop_in_place", + "drop_in_place>", + "::drop", +] +_TLS_USER_DTOR = [ + "::drop", + "drop_in_place", + "destroy", + "run", + "std::sys::thread_local::guard::key::enable::run", +] + + +def test_a_race_in_a_message_dropped_by_oneshot_is_still_reported(): + assert _suppressed([_RECEIPT_DROP, _RECEIPT_DROP]) is None + + +def test_a_race_in_a_closure_run_by_the_epoch_collector_is_still_reported(): + assert _suppressed([_DEFERRED_CLOSURE, _DEFERRED_CLOSURE]) is None + assert _suppressed([_BAG_DROP_IN_LOCAL, _DEFERRED_CLOSURE]) is None + + +def test_a_race_in_a_thread_local_destructor_is_still_reported(): + assert _suppressed([_TLS_USER_DTOR, _TLS_USER_DTOR]) is None + + +def test_a_race_merely_on_a_thread_that_uses_the_dependency_is_still_reported(): + """Thread-creation stacks are matched by `race:` too; your callbacks in them are not.""" + mine = ["::put", *_USER] + assert _suppressed([mine, mine], threads=[_RECEIPT_DROP, _DEFERRED_CLOSURE]) is None diff --git a/tests/test_stress_config.py b/tests/test_stress_config.py index c25ce01..da05f69 100644 --- a/tests/test_stress_config.py +++ b/tests/test_stress_config.py @@ -85,20 +85,6 @@ def test_a_panic_only_under_concurrency_is_a_finding(): assert "--replay 3" in finding["message"] -def test_every_default_suppression_is_narrow_and_justified(): - """race_top only, each preceded by a comment: a broad `race:` would hide real - user races that run inside a dependency.""" - from ftcheck.ci.pipeline import DEFAULT_SUPPRESSIONS - - lines = DEFAULT_SUPPRESSIONS.read_text().splitlines() - entries = [(i, ln) for i, ln in enumerate(lines) if ln and not ln.startswith("#")] - assert entries, "the default file must not be empty" - for i, line in entries: - assert line.startswith("race_top:"), line - assert any(lines[j].startswith("#") for j in range(max(0, i - 3), i)), line - - - def test_panics_at_one_location_are_one_finding_located_there(): from ftcheck.stress import panic_findings, panic_locations