Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close TLS-based soundness holes by running closure on a newly created thread. #3646

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ chrono-tz = ">= 0.6, < 0.10"
trybuild = ">=1.0.70"
proptest = { version = "1.0", default-features = false, features = ["std"] }
send_wrapper = "0.6"
scoped-tls-hkt = "0.1"
serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0.61"
rayon = "1.6.1"
Expand Down
2 changes: 1 addition & 1 deletion examples/word-count/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ fn search_sequential(contents: &str, needle: &str) -> usize {

#[pyfunction]
fn search_sequential_allow_threads(py: Python<'_>, contents: &str, needle: &str) -> usize {
py.allow_threads(|| search_sequential(contents, needle))
py.allow_threads().with(|| search_sequential(contents, needle))
}

/// Count the occurrences of needle in line, case insensitive
Expand Down
2 changes: 1 addition & 1 deletion guide/src/async-await.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ where
fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
let waker = cx.waker();
Python::with_gil(|gil| {
gil.allow_threads(|| pin!(&mut self.0).poll(&mut Context::from_waker(waker)))
gil.allow_threads().with(|| pin!(&mut self.0).poll(&mut Context::from_waker(waker)))
})
}
}
Expand Down
4 changes: 0 additions & 4 deletions guide/src/features.md
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,6 @@ Most users should only need a single `#[pymethods]` per `#[pyclass]`. In additio

See [the `#[pyclass]` implementation details](class.md#implementation-details) for more information.

### `nightly`

The `nightly` feature needs the nightly Rust compiler. This allows PyO3 to use the `auto_traits` and `negative_impls` features to fix the `Python::allow_threads` function.

### `resolve-config`

The `resolve-config` feature of the `pyo3-build-config` crate controls whether that crate's
Expand Down
4 changes: 4 additions & 0 deletions guide/src/migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@ Python::with_gil(|py| {
```
</details>

### `Python::allow_threads` was split into `Python::safe_allow_threads` and `Python::unsafe_allow_threads`

TODO

### `Iter(A)NextOutput` are deprecated
<details open>
<summary><small>Click to expand</small></summary>
Expand Down
2 changes: 1 addition & 1 deletion guide/src/parallelism.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ To enable parallel execution of this function, the [`Python::allow_threads`] met
# }
#[pyfunction]
fn search_sequential_allow_threads(py: Python<'_>, contents: &str, needle: &str) -> usize {
py.allow_threads(|| search_sequential(contents, needle))
py.allow_threads().with(|| search_sequential(contents, needle))
}
```

Expand Down
1 change: 1 addition & 0 deletions newsfragments/3646.changed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
`Python::allow_threads` now returns a builder-like object which allows choosing a strategy for running the closure, as the default one which now closes TLS-based soundness loopholes by dispatching the closure to a worker thread has significantly changed performance characteristics.
16 changes: 16 additions & 0 deletions pyo3-benches/benches/bench_gil.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use codspeed_criterion_compat::{criterion_group, criterion_main, Bencher, Criterion};
use std::hint::black_box;

use pyo3::prelude::*;

Expand All @@ -13,9 +14,24 @@ fn bench_dirty_acquire_gil(b: &mut Bencher<'_>) {
b.iter(|| Python::with_gil(|py| obj.clone_ref(py)));
}

fn bench_allow_threads(b: &mut Bencher<'_>) {
Python::with_gil(|py| {
py.allow_threads().with(|| ());
b.iter(|| py.allow_threads().with(|| black_box(42)));
});
}

fn bench_local_allow_threads(b: &mut Bencher<'_>) {
Python::with_gil(|py| {
b.iter(|| unsafe { py.allow_threads().local() }.with(|| black_box(42)));
});
}

fn criterion_benchmark(c: &mut Criterion) {
c.bench_function("clean_acquire_gil", bench_clean_acquire_gil);
c.bench_function("dirty_acquire_gil", bench_dirty_acquire_gil);
c.bench_function("allow_threads", bench_allow_threads);
c.bench_function("local_allow_threads", bench_local_allow_threads);
}

criterion_group!(benches, criterion_benchmark);
Expand Down
2 changes: 0 additions & 2 deletions src/err/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,6 @@ pub struct PyErr {
}

// The inner value is only accessed through ways that require proving the gil is held
#[cfg(feature = "nightly")]
unsafe impl crate::marker::Ungil for PyErr {}
unsafe impl Send for PyErr {}
unsafe impl Sync for PyErr {}

Expand Down
26 changes: 14 additions & 12 deletions src/gil.rs
Original file line number Diff line number Diff line change
Expand Up @@ -735,23 +735,25 @@ mod tests {

#[test]
fn test_allow_threads() {
assert!(!gil_is_acquired());
for _ in 0..10 {
assert!(!gil_is_acquired());

Python::with_gil(|py| {
assert!(gil_is_acquired());
Python::with_gil(|py| {
assert!(gil_is_acquired());

py.allow_threads(move || {
assert!(!gil_is_acquired());
py.allow_threads().with(move || {
assert!(!gil_is_acquired());

Python::with_gil(|_| assert!(gil_is_acquired()));
Python::with_gil(|_| assert!(gil_is_acquired()));

assert!(!gil_is_acquired());
});
assert!(!gil_is_acquired());
});

assert!(gil_is_acquired());
});
assert!(gil_is_acquired());
});

assert!(!gil_is_acquired());
assert!(!gil_is_acquired());
}
}

#[cfg(feature = "py-clone")]
Expand All @@ -763,7 +765,7 @@ mod tests {
let obj = get_object(py);
assert!(obj.get_refcnt(py) == 1);
// Clone the object without the GIL which should panic
py.allow_threads(|| obj.clone());
py.allow_threads().with(|| obj.clone());
});
}

Expand Down
2 changes: 0 additions & 2 deletions src/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -922,8 +922,6 @@ impl<T> IntoPy<PyObject> for Borrowed<'_, '_, T> {
pub struct Py<T>(NonNull<ffi::PyObject>, PhantomData<T>);

// The inner value is only accessed through ways that require proving the gil is held
#[cfg(feature = "nightly")]
unsafe impl<T> crate::marker::Ungil for Py<T> {}
unsafe impl<T> Send for Py<T> {}
unsafe impl<T> Sync for Py<T> {}

Expand Down
6 changes: 0 additions & 6 deletions src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#![warn(missing_docs)]
#![cfg_attr(feature = "nightly", feature(auto_traits, negative_impls))]
#![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))]
// Deny some lints in doctests.
// Use `#[allow(...)]` locally to override.
Expand Down Expand Up @@ -115,10 +114,6 @@
//! [`Py`]`<T>` for all `T` that implement [`Serialize`] and [`Deserialize`].
//! - [`smallvec`][smallvec]: Enables conversions between Python list and [smallvec]'s [`SmallVec`].
//!
//! ## Unstable features
//!
//! - `nightly`: Uses `#![feature(auto_traits, negative_impls)]` to define [`Ungil`] as an auto trait.
//
//! ## `rustc` environment flags
//!
//! PyO3 uses `rustc`'s `--cfg` flags to enable or disable code used for different Python versions.
Expand Down Expand Up @@ -314,7 +309,6 @@
//! [Python from Rust]: https://github.com/PyO3/pyo3#using-python-from-rust
//! [Rust from Python]: https://github.com/PyO3/pyo3#using-rust-from-python
//! [Features chapter of the guide]: https://pyo3.rs/latest/features.html#features-reference "Features Reference - PyO3 user guide"
//! [`Ungil`]: crate::marker::Ungil
pub use crate::class::*;
pub use crate::conversion::{AsPyPointer, FromPyObject, IntoPy, ToPyObject};
#[cfg(feature = "gil-refs")]
Expand Down
Loading
Loading