From 982ac647b0d084d5b7900f989caebfafc4bb69e4 Mon Sep 17 00:00:00 2001 From: Robsdedude Date: Fri, 29 Nov 2024 09:30:19 +0100 Subject: [PATCH 01/13] Towards soundness of PyByteArrayMethods::to_vec In free-threaded Python, to_vec needs to make sure to run inside a critical section so that no other Python thread is mutating the bytearray causing UB. See also https://github.com/PyO3/pyo3/issues/4736 --- src/types/bytearray.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/types/bytearray.rs b/src/types/bytearray.rs index d1bbd0ac7e4..3294578ad66 100644 --- a/src/types/bytearray.rs +++ b/src/types/bytearray.rs @@ -2,6 +2,7 @@ use crate::err::{PyErr, PyResult}; use crate::ffi_ptr_ext::FfiPtrExt; use crate::instance::{Borrowed, Bound}; use crate::py_result_ext::PyResultExt; +use crate::sync::with_critical_section; use crate::types::any::PyAnyMethods; use crate::{ffi, PyAny, Python}; use std::slice; @@ -152,7 +153,7 @@ pub trait PyByteArrayMethods<'py>: crate::sealed::Sealed { /// using the slice. /// /// As a result, this slice should only be used for short-lived operations without executing any - /// Python code, such as copying into a Vec. + /// Python code, such as copying into a Vec. For free-threaded Python support see also [`with_critical_section`]. /// /// # Examples /// @@ -295,7 +296,7 @@ impl<'py> PyByteArrayMethods<'py> for Bound<'py, PyByteArray> { } fn to_vec(&self) -> Vec { - unsafe { self.as_bytes() }.to_vec() + with_critical_section(self, || unsafe { self.as_bytes() }.to_vec()) } fn resize(&self, len: usize) -> PyResult<()> { From 15e602be9cf95bdf6dbebd1a51a0344260bca0ce Mon Sep 17 00:00:00 2001 From: Robsdedude Date: Fri, 29 Nov 2024 09:44:07 +0100 Subject: [PATCH 02/13] Add changelog entry --- newsfragments/4742.fixed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/4742.fixed.md diff --git a/newsfragments/4742.fixed.md b/newsfragments/4742.fixed.md new file mode 100644 index 00000000000..1ec5ba35441 --- /dev/null +++ b/newsfragments/4742.fixed.md @@ -0,0 +1 @@ +Improved soundness of `PyByteArray::to_vec`. From b6cd9a6c8773ffeb7ae9f52bbb8bd6c20202ed55 Mon Sep 17 00:00:00 2001 From: Robsdedude Date: Fri, 29 Nov 2024 13:17:02 +0100 Subject: [PATCH 03/13] Reword changelog entry Co-authored-by: David Hewitt --- newsfragments/4742.fixed.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/newsfragments/4742.fixed.md b/newsfragments/4742.fixed.md index 1ec5ba35441..7bfe38b0c38 100644 --- a/newsfragments/4742.fixed.md +++ b/newsfragments/4742.fixed.md @@ -1 +1 @@ -Improved soundness of `PyByteArray::to_vec`. +Use critical section in `PyByteArray::to_vec` on freethreaded build to replicate GIL-enabled "soundness". From 5630668bd945a2476389808eda0b8f21a382b18d Mon Sep 17 00:00:00 2001 From: Robsdedude Date: Sun, 20 Jul 2025 12:34:14 +0200 Subject: [PATCH 04/13] Add test --- src/types/bytearray.rs | 135 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 135 insertions(+) diff --git a/src/types/bytearray.rs b/src/types/bytearray.rs index 5be7aab53aa..866e5667de9 100644 --- a/src/types/bytearray.rs +++ b/src/types/bytearray.rs @@ -311,8 +311,16 @@ impl<'py> TryFrom<&Bound<'py, PyAny>> for Bound<'py, PyByteArray> { #[cfg(test)] mod tests { + use crate::instance::Py; + use crate::sync::{with_critical_section, MutexExt}; use crate::types::{PyAnyMethods, PyByteArray, PyByteArrayMethods}; use crate::{exceptions, Bound, PyAny, PyObject, Python}; + use pyo3_ffi::c_str; + use std::sync::atomic::{AtomicBool, Ordering}; + use std::sync::Mutex; + use std::thread; + use std::thread::ScopedJoinHandle; + use std::time::Duration; #[test] fn test_len() { @@ -445,4 +453,131 @@ mod tests { .is_instance_of::(py)); }) } + + #[test] + fn test_data_integrity_in_critical_section() { + const SIZE: usize = 200_000_000; + const DATA_VALUE: u8 = 42; + const GARBAGE_VALUE: u8 = 13; + + fn make_byte_array(py: Python<'_>, size: usize, value: u8) -> Bound<'_, PyByteArray> { + PyByteArray::new_with(py, size, |b| { + b.fill(value); + Ok(()) + }) + .unwrap() + } + + let data: Mutex> = Mutex::new(Python::attach(|py| { + make_byte_array(py, SIZE, DATA_VALUE).unbind() + })); + + fn get_data<'py>( + data: &Mutex>, + py: Python<'py>, + ) -> Bound<'py, PyByteArray> { + data.lock_py_attached(py).unwrap().bind(py).clone() + } + + fn set_data(data: &Mutex>, new: Bound<'_, PyByteArray>) { + let py = new.py(); + *data.lock_py_attached(py).unwrap() = new.unbind() + } + + let running = AtomicBool::new(true); + let extending = AtomicBool::new(false); + + // continuously extends and resets the bytearray in data + let worker1 = || { + while running.load(Ordering::Relaxed) { + Python::attach(|py| { + let byte_array = get_data(&data, py); + extending.store(true, Ordering::SeqCst); + byte_array + .call_method("extend", (&byte_array,), None) + .unwrap(); + extending.store(false, Ordering::SeqCst); + set_data(&data, make_byte_array(py, SIZE, DATA_VALUE)); + }); + } + }; + + // continuously checks the integrity of bytearray in data + let worker2 = || { + while running.load(Ordering::Relaxed) { + if !extending.load(Ordering::SeqCst) { + // wait until we have a chance to read inconsistent state + continue; + } + Python::attach(|py| { + let read = get_data(&data, py); + if read.len() == SIZE { + // extend is still not done => wait even more + return; + } + with_critical_section(&read, || { + // SAFETY: we are in a critical section + // This is the whole point of the test: make sure that a + // critical section is sufficient to ensure that the data + // read is consistent. + unsafe { + let bytes = read.as_bytes(); + assert!(bytes.iter().rev().take(50).all(|v| *v == DATA_VALUE + && bytes.iter().take(50).all(|v| *v == DATA_VALUE))); + } + }); + }) + } + }; + + // write unrelated data to the memory for extra stress + let worker3 = || { + while running.load(Ordering::Relaxed) { + Python::attach(|py| { + let arrays = (0..5) + .map(|_| { + make_byte_array(py, SIZE, GARBAGE_VALUE); + }) + .collect::>(); + drop(arrays); + py.run(c_str!(r#"import gc; gc.collect()"#), None, None) + .unwrap(); + }); + } + }; + + thread::scope(|s| { + let mut handle1 = Some(s.spawn(worker1)); + let mut handle2 = Some(s.spawn(worker2)); + let mut handle3 = Some(s.spawn(worker3)); + let mut handles = [&mut handle1, &mut handle2, &mut handle3]; + + let t0 = std::time::Instant::now(); + while t0.elapsed() < Duration::from_secs(10) { + for handle in &mut handles { + if handle + .as_ref() + .map(ScopedJoinHandle::is_finished) + .unwrap_or(false) + { + handle + .take() + .unwrap() + .join() + .inspect_err(|_| running.store(false, Ordering::Relaxed)) + .unwrap() + } + } + if handles.iter().all(|handle| handle.is_none()) { + break; + } + } + running.store(false, Ordering::Relaxed); + for handle in &mut handles { + if let Some(handle) = handle.take() { + handle.join().unwrap() + } + } + }); + } } From 3819f788caf555befd7364fdb18c846cca92c489 Mon Sep 17 00:00:00 2001 From: Robsdedude Date: Sun, 20 Jul 2025 12:59:25 +0200 Subject: [PATCH 05/13] Skip test failing for unsound CPython 3.13 --- src/types/bytearray.rs | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/types/bytearray.rs b/src/types/bytearray.rs index 866e5667de9..137abba2b30 100644 --- a/src/types/bytearray.rs +++ b/src/types/bytearray.rs @@ -311,16 +311,8 @@ impl<'py> TryFrom<&Bound<'py, PyAny>> for Bound<'py, PyByteArray> { #[cfg(test)] mod tests { - use crate::instance::Py; - use crate::sync::{with_critical_section, MutexExt}; use crate::types::{PyAnyMethods, PyByteArray, PyByteArrayMethods}; use crate::{exceptions, Bound, PyAny, PyObject, Python}; - use pyo3_ffi::c_str; - use std::sync::atomic::{AtomicBool, Ordering}; - use std::sync::Mutex; - use std::thread; - use std::thread::ScopedJoinHandle; - use std::time::Duration; #[test] fn test_len() { @@ -454,8 +446,21 @@ mod tests { }) } + // CPython 3.13t is unsound => test fails + #[cfg(any(Py_3_14, not(all(Py_3_13, Py_GIL_DISABLED))))] #[test] fn test_data_integrity_in_critical_section() { + use crate::instance::Py; + use crate::sync::{with_critical_section, MutexExt}; + + use pyo3_ffi::c_str; + + use std::sync::atomic::{AtomicBool, Ordering}; + use std::sync::Mutex; + use std::thread; + use std::thread::ScopedJoinHandle; + use std::time::Duration; + const SIZE: usize = 200_000_000; const DATA_VALUE: u8 = 42; const GARBAGE_VALUE: u8 = 13; From b22fbbc39760bbbce5fa83846793b57ddf72a326 Mon Sep 17 00:00:00 2001 From: Robsdedude Date: Sun, 20 Jul 2025 15:33:41 +0200 Subject: [PATCH 06/13] Improve docs --- src/types/bytearray.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/types/bytearray.rs b/src/types/bytearray.rs index 137abba2b30..267590690d0 100644 --- a/src/types/bytearray.rs +++ b/src/types/bytearray.rs @@ -125,19 +125,21 @@ pub trait PyByteArrayMethods<'py>: crate::sealed::Sealed { /// using the slice. /// /// As a result, this slice should only be used for short-lived operations without executing any - /// Python code, such as copying into a Vec. For free-threaded Python support see also [`with_critical_section`]. + /// Python code, such as copying into a Vec. + /// For free-threaded Python support see also [`with_critical_section`]. /// /// # Examples /// /// ```rust /// use pyo3::prelude::*; /// use pyo3::exceptions::PyRuntimeError; + /// use pyo3::sync::with_critical_section; /// use pyo3::types::PyByteArray; /// /// #[pyfunction] /// fn a_valid_function(bytes: &Bound<'_, PyByteArray>) -> PyResult<()> { - /// let section = { - /// // SAFETY: We promise to not let the interpreter regain control + /// let section = with_critical_section(bytes, || { + /// // SAFETY: We promise to not let the interpreter regain control over the bytearray /// // or invoke any PyO3 APIs while using the slice. /// let slice = unsafe { bytes.as_bytes() }; /// @@ -147,7 +149,7 @@ pub trait PyByteArrayMethods<'py>: crate::sealed::Sealed { /// .get(6..11) /// .ok_or_else(|| PyRuntimeError::new_err("input is not long enough"))?; /// Vec::from(section) - /// }; + /// }); /// /// // Now we can do things with `section` and call PyO3 APIs again. /// // ... @@ -189,6 +191,9 @@ pub trait PyByteArrayMethods<'py>: crate::sealed::Sealed { /// # #[allow(dead_code)] /// #[pyfunction] /// fn bug(py: Python<'_>, bytes: &Bound<'_, PyByteArray>) { + /// // No critical section is being used. + /// // This means that for no-gil Python another thread could be modifying the + /// // bytearray concurrently and thus invalidate `slice` any time. /// let slice = unsafe { bytes.as_bytes() }; /// /// // This explicitly yields control back to the Python interpreter... From 42167884bab305b96077f963a3ea307845403952 Mon Sep 17 00:00:00 2001 From: Robsdedude Date: Sun, 20 Jul 2025 15:33:59 +0200 Subject: [PATCH 07/13] Add safety comment to unsafe invocation --- src/types/bytearray.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/types/bytearray.rs b/src/types/bytearray.rs index 267590690d0..10dcc05c96d 100644 --- a/src/types/bytearray.rs +++ b/src/types/bytearray.rs @@ -273,7 +273,14 @@ impl<'py> PyByteArrayMethods<'py> for Bound<'py, PyByteArray> { } fn to_vec(&self) -> Vec { - with_critical_section(self, || unsafe { self.as_bytes() }.to_vec()) + with_critical_section(self, || { + // SAFETY: + // * `self` is a `Bound` object, which guarantees that the Python GIL is held. + // * For no-gil Python, a critical section is used in lieu of the GIL. + // * We don't interact with the interpreter + // * We don't mutate the underlying slice + unsafe { self.as_bytes() }.to_vec() + }) } fn resize(&self, len: usize) -> PyResult<()> { From 2def97f5407cf7a1a4d8ebf2116b89418fe1bf12 Mon Sep 17 00:00:00 2001 From: Robsdedude Date: Sun, 20 Jul 2025 15:34:27 +0200 Subject: [PATCH 08/13] Limit test by time AND number of rounds --- src/types/bytearray.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/types/bytearray.rs b/src/types/bytearray.rs index 10dcc05c96d..0b8bc522e2e 100644 --- a/src/types/bytearray.rs +++ b/src/types/bytearray.rs @@ -506,7 +506,8 @@ mod tests { // continuously extends and resets the bytearray in data let worker1 = || { - while running.load(Ordering::Relaxed) { + let mut rounds = 0; + while running.load(Ordering::Relaxed) && rounds < 25 { Python::attach(|py| { let byte_array = get_data(&data, py); extending.store(true, Ordering::SeqCst); @@ -515,6 +516,7 @@ mod tests { .unwrap(); extending.store(false, Ordering::SeqCst); set_data(&data, make_byte_array(py, SIZE, DATA_VALUE)); + rounds += 1; }); } }; @@ -585,7 +587,7 @@ mod tests { .unwrap() } } - if handles.iter().all(|handle| handle.is_none()) { + if handles.iter().any(|handle| handle.is_none()) { break; } } From dc15f4532378836874c49e98f76bdedea95da5fa Mon Sep 17 00:00:00 2001 From: Robsdedude Date: Sun, 20 Jul 2025 15:48:22 +0200 Subject: [PATCH 09/13] Test: fix MSRV compatibility & simplify scenario --- src/types/bytearray.rs | 39 +++++++++------------------------------ 1 file changed, 9 insertions(+), 30 deletions(-) diff --git a/src/types/bytearray.rs b/src/types/bytearray.rs index 0b8bc522e2e..9ab7cf62e74 100644 --- a/src/types/bytearray.rs +++ b/src/types/bytearray.rs @@ -465,8 +465,6 @@ mod tests { use crate::instance::Py; use crate::sync::{with_critical_section, MutexExt}; - use pyo3_ffi::c_str; - use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Mutex; use std::thread; @@ -475,7 +473,6 @@ mod tests { const SIZE: usize = 200_000_000; const DATA_VALUE: u8 = 42; - const GARBAGE_VALUE: u8 = 13; fn make_byte_array(py: Python<'_>, size: usize, value: u8) -> Bound<'_, PyByteArray> { PyByteArray::new_with(py, size, |b| { @@ -507,7 +504,7 @@ mod tests { // continuously extends and resets the bytearray in data let worker1 = || { let mut rounds = 0; - while running.load(Ordering::Relaxed) && rounds < 25 { + while running.load(Ordering::SeqCst) && rounds < 25 { Python::attach(|py| { let byte_array = get_data(&data, py); extending.store(true, Ordering::SeqCst); @@ -523,7 +520,7 @@ mod tests { // continuously checks the integrity of bytearray in data let worker2 = || { - while running.load(Ordering::Relaxed) { + while running.load(Ordering::SeqCst) { if !extending.load(Ordering::SeqCst) { // wait until we have a chance to read inconsistent state continue; @@ -545,22 +542,6 @@ mod tests { && bytes.iter().take(50).all(|v| *v == DATA_VALUE))); } }); - }) - } - }; - - // write unrelated data to the memory for extra stress - let worker3 = || { - while running.load(Ordering::Relaxed) { - Python::attach(|py| { - let arrays = (0..5) - .map(|_| { - make_byte_array(py, SIZE, GARBAGE_VALUE); - }) - .collect::>(); - drop(arrays); - py.run(c_str!(r#"import gc; gc.collect()"#), None, None) - .unwrap(); }); } }; @@ -568,8 +549,7 @@ mod tests { thread::scope(|s| { let mut handle1 = Some(s.spawn(worker1)); let mut handle2 = Some(s.spawn(worker2)); - let mut handle3 = Some(s.spawn(worker3)); - let mut handles = [&mut handle1, &mut handle2, &mut handle3]; + let mut handles = [&mut handle1, &mut handle2]; let t0 = std::time::Instant::now(); while t0.elapsed() < Duration::from_secs(10) { @@ -579,19 +559,18 @@ mod tests { .map(ScopedJoinHandle::is_finished) .unwrap_or(false) { - handle - .take() - .unwrap() - .join() - .inspect_err(|_| running.store(false, Ordering::Relaxed)) - .unwrap() + let res = handle.take().unwrap().join(); + if res.is_err() { + running.store(false, Ordering::SeqCst); + } + res.unwrap(); } } if handles.iter().any(|handle| handle.is_none()) { break; } } - running.store(false, Ordering::Relaxed); + running.store(false, Ordering::SeqCst); for handle in &mut handles { if let Some(handle) = handle.take() { handle.join().unwrap() From 8c4fd92dce7edecb23f4486c11b3a1d41e29d0df Mon Sep 17 00:00:00 2001 From: Robsdedude Date: Sun, 20 Jul 2025 16:13:54 +0200 Subject: [PATCH 10/13] Fix example code --- src/types/bytearray.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/types/bytearray.rs b/src/types/bytearray.rs index 9ab7cf62e74..67c4ef02cd7 100644 --- a/src/types/bytearray.rs +++ b/src/types/bytearray.rs @@ -145,11 +145,10 @@ pub trait PyByteArrayMethods<'py>: crate::sealed::Sealed { /// /// // Copy only a section of `bytes` while avoiding /// // `to_vec` which copies the entire thing. - /// let section = slice - /// .get(6..11) - /// .ok_or_else(|| PyRuntimeError::new_err("input is not long enough"))?; - /// Vec::from(section) - /// }); + /// slice.get(6..11) + /// .map(Vec::from) + /// .ok_or_else(|| PyRuntimeError::new_err("input is not long enough")) + /// })?; /// /// // Now we can do things with `section` and call PyO3 APIs again. /// // ... From df74c80300994d5c1dcfd7b641e111a2b7b96c56 Mon Sep 17 00:00:00 2001 From: Robsdedude Date: Sun, 20 Jul 2025 23:09:52 +0200 Subject: [PATCH 11/13] Further reduce required test duration --- src/types/bytearray.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/types/bytearray.rs b/src/types/bytearray.rs index 67c4ef02cd7..bbab8030a73 100644 --- a/src/types/bytearray.rs +++ b/src/types/bytearray.rs @@ -470,7 +470,7 @@ mod tests { use std::thread::ScopedJoinHandle; use std::time::Duration; - const SIZE: usize = 200_000_000; + const SIZE: usize = 1_000_000; const DATA_VALUE: u8 = 42; fn make_byte_array(py: Python<'_>, size: usize, value: u8) -> Bound<'_, PyByteArray> { @@ -503,7 +503,7 @@ mod tests { // continuously extends and resets the bytearray in data let worker1 = || { let mut rounds = 0; - while running.load(Ordering::SeqCst) && rounds < 25 { + while running.load(Ordering::SeqCst) && rounds < 50 { Python::attach(|py| { let byte_array = get_data(&data, py); extending.store(true, Ordering::SeqCst); @@ -551,7 +551,7 @@ mod tests { let mut handles = [&mut handle1, &mut handle2]; let t0 = std::time::Instant::now(); - while t0.elapsed() < Duration::from_secs(10) { + while t0.elapsed() < Duration::from_secs(1) { for handle in &mut handles { if handle .as_ref() From 7d82de96f7520d97eedfec4d15c5766618bbd2c4 Mon Sep 17 00:00:00 2001 From: Robsdedude Date: Thu, 24 Jul 2025 09:31:49 +0200 Subject: [PATCH 12/13] Skip threaded test on wasm --- src/types/bytearray.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/types/bytearray.rs b/src/types/bytearray.rs index bbab8030a73..64aaca4549e 100644 --- a/src/types/bytearray.rs +++ b/src/types/bytearray.rs @@ -459,6 +459,8 @@ mod tests { // CPython 3.13t is unsound => test fails #[cfg(any(Py_3_14, not(all(Py_3_13, Py_GIL_DISABLED))))] + // No threading support on wasm + #[cfg(not(target_family = "wasm"))] #[test] fn test_data_integrity_in_critical_section() { use crate::instance::Py; From 53c990ad2f76ada3e6ed89c99cce2aecd06e50c1 Mon Sep 17 00:00:00 2001 From: Robsdedude Date: Thu, 24 Jul 2025 11:48:16 +0200 Subject: [PATCH 13/13] Rewrite cfg attribute for llvm-cov to not turn into llvm-cough --- src/types/bytearray.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/types/bytearray.rs b/src/types/bytearray.rs index 64aaca4549e..da3eb3b2b20 100644 --- a/src/types/bytearray.rs +++ b/src/types/bytearray.rs @@ -457,10 +457,12 @@ mod tests { }) } - // CPython 3.13t is unsound => test fails - #[cfg(any(Py_3_14, not(all(Py_3_13, Py_GIL_DISABLED))))] - // No threading support on wasm - #[cfg(not(target_family = "wasm"))] + // * wasm has no threading support + // * CPython 3.13t is unsound => test fails + #[cfg(all( + not(target_family = "wasm"), + any(Py_3_14, not(all(Py_3_13, Py_GIL_DISABLED))) + ))] #[test] fn test_data_integrity_in_critical_section() { use crate::instance::Py;