`Python::allow_threads` is unsound in the presence of `scoped-tls`.

[steffahn]

In analogy to

• Python::allow_threads is unsound in the presence of send_wrapper. #2141

we can smuggle Ungil data across Python::allow_threads using scoped-tls, too.

use pyo3::prelude::*;
use pyo3::types::PyString;
use scoped_tls::scoped_thread_local;

fn main() {
    Python::with_gil(|py| {
        let string = PyString::new(py, "foo");

        scoped_thread_local!(static WRAPPED: PyString);

        WRAPPED.set(string, || {
            py.allow_threads(|| {
                WRAPPED.with(|smuggled: &PyString| {
                    println!("{:?}", smuggled);
                });
            });
        });
    });
}

(results in segfault in my test)

Unlike #2141, this issue is virtually unsolvable, i.e. even the auto trait approach with the feature="nightly" enabled cannot catch this at all. There’s no property in the callback to allow_threads that can catch this. Really, the callback doesn’t even capture anything at all:

use pyo3::prelude::*;
use pyo3::types::PyString;
use scoped_tls::scoped_thread_local;

scoped_thread_local!(static WRAPPED: PyString);

fn callback() {
    WRAPPED.with(|smuggled: &PyString| {
        println!("{:?}", smuggled);
    });
}

fn main() {
    Python::with_gil(|py| {
        let string = PyString::new(py, "foo");

        WRAPPED.set(string, || {
            py.allow_threads(callback); // callback is an ordinary `fn() -> ()` item.
        });
    });
}

Again, there’s a “whose fault” question to be asked, whether the fact that the standard library’s thread-locals require T: 'static means there’s any guarantees that non-'static data isn’t allowed to be “smuggled” through thread-local storage. In my view, if you look at what kind of things it offers, scoped-tls seems even less unreasonable to be called “sound”, compared to sync_wrapper. Yet the consequences for Ungil are more detrimental.

[adamreichold]

From a first glance, I fear that saving the current approach might mean to never ever expose any GIL-bound references no matter how short-lived and have all operations explicitly require a GIL token. This would be highly unergonomic and imply that our upcoming GIL-bound smart pointers Py2<'py, T> would provide no benefit over their existing unbound counterpart Py<T> at all.

If we somehow could avoid at least storing our GIL-bound smart pointers within scoped-tls, but that library has no traits bounds at all. So I suspect this is coming down to either abandoning a safe allow_threads or socially establish that

whether the fact that the standard library’s thread-locals require T: 'static means there’s any guarantees that non-'static data isn’t allowed to be “smuggled” through thread-local storage.

should be upheld. Or at least require an extra unsafe trait bound for usage of types with scoped-tls.

In my view, if you look at what kind of things it offers, scoped-tls seems even less unreasonable to be called “sound”, compared to sync_wrapper. Yet the consequences for Ungil are more detrimental.

Better yet, there would be a case of unsoundness implied by usage of scoped-tls that does not involve the admittedly somewhat baroque architecture we built to accommodate Python in its GIL.

@steffahn Since you are obviously experienced in the finding these holes, did you ever look into scoped-tls from a blank slate perspective?

[adamreichold]

From a first glance, I fear that saving the current approach might mean to never ever expose any GIL-bound references no matter how short-lived and have all operations explicitly require a GIL token. This would be highly unergonomic and imply that our upcoming GIL-bound smart pointers Py2<'py, T> would provide no benefit over their existing unbound counterpart Py at all.

But this is not true, right? Because I could also use scoped-tls to smuggle the GIL token itself even if it has a generative lifetime attached?

[steffahn]

But this is not true, right? Because I could also use scoped-tls to smuggle the GIL token itself even if it has a generative lifetime attached?

Yes. At least in the extended version provided in the crate scoped-tls-hkt.

[steffahn]

use pyo3::prelude::*;
use scoped_tls_hkt::scoped_thread_local;

fn main() {
    Python::with_gil(|py| {
        scoped_thread_local!(static GIL: for<'a> Python<'a>);
        GIL.set(py, || {
            py.allow_threads(|| {
                GIL.with(|smuggled_py: Python<'_>| {
                    // profit
                });
            });
        });
    });
}

[davidhewitt]

Ouch, this is painful. Thanks @steffahn for finding such an interesting case!

My feeling here is that this is a much more severe mechanism to break allow_threads than the interaction with send_wrapper. At least with send_wrapper any smuggled data is part of the closure capture, so careful inspection around the crash could in theory identify the issue. This scoped-tls approach can enable you to have the smuggling be located in a completely separate part of the codebase to the allow_threads call, I think it would be much harder to resolve this.

It seems like the natural reaction is to declare allow_threads an unsafe function, but I think this invariant "you must be sure that scoped-tls and send-wrapper are not used to smuggle a Python marker into code invoked inside the closure" becomes extremely hard to uphold. Still, at this point it's likely a necessary change unless we can find a way to resolve the API.

What would a "safe" solution look like? Can we somehow express Python as a borrow and py.allow_threads takes an exclusive borrow, thus preventing all gil-attached data and smuggling? Maybe, but I suspect that at least some of PyO3's existing APIs need changing to achieve this.

Alternatively, a future approach might involve contexts, where allow_threads removes the context, but this is obviously not available to us at present.

[adamreichold]

What would a "safe" solution look like? Can we somehow express Python as a borrow and py.allow_threads takes an exclusive borrow, thus preventing all gil-attached data and smuggling? Maybe, but I suspect that at least some of PyO3's existing APIs need changing to achieve this.

I think this would basically be equivalent to removing the Clone and Copy impls for Python and making it invariant over the lifetime? But I suspect especially the invariance part (requiring a "reborrowing" operation) would be quite painful from an ergonomics point of view?

[davidhewitt]

At least that I think, yes. I think it might also have implications for .py() methods to get a copy of Python, unless they are also expressed as borrows on &Python (or similar), but I haven't thought this through.

My feeling is that coming up with a workable proposal for that is going to need someone to dive deep into this for at least a few days.

How do you feel about accepting Python::allow_threads as unsafe in the short-mid term? Hopefully we can come up with a long-term solution to make it safe again.

Do we think this has implications for the proposed Py2<'_, PyAny> smart pointer? I think if we make allow_threads unsafe then there's no safety concerns about that API, and we are already aware that being able to assume the GIL is held is a significant ergonomics win, especially in trait implementations, so I believe that API still makes sense.

[davidhewitt]

(Given the Py2<'_, PyAny> change is mostly motivated by getting rid of the pool, and thus leading users to have proper treatment of ownership in their code, I think that it's a correct evolution of the API even if this issue might imply that it will eventually need to change again.)

[adamreichold]

How do you feel about accepting Python::allow_threads as unsafe in the short-mid term? Hopefully we can come up with a long-term solution to make it safe again.

My gut feeling is to not rush this (after all, we having been living with the send_wrapper soundness hole for quite some time). The reasons I see for keeping the status quo for now are:

• I am not sure whether scoped-tls poses a problem in itself, not just for PyO3. But I have not yet had time to look into constructing an issue without involving PyO3.
• As you wrote, it is basically impossible to check the required invariants if scoped-tls is involved due to the non-local reasoning required. (This non-locality is basically where my gut feeling that scoped-tls is generally a bad idea comes from.)
• I fear that marking allow_threads unsafe will "infect" a lot of code due to the closure-based API by effectively making the closure body an unsafe block as well, c.f. this playground. Hence, I think the resulting API will lead to more actual errors than the existing soundness issues.

[davidhewitt]

Sure thing. The last point in particular that unsafe { } will infect the closure is a very good point which I had not considered, agreed that can also create problems.

[davidhewitt]

Just for the sake of throwing out another middle-of-the-road solution, what if the method was renamed to unsafe_allow_threads for now? That would avoid the infective problem while also establishing some sort of social contract via the use of the term "unsafe".

I can see a very mixed reaction from the community to doing that (e.g. we might be accused of shipping code which doesn't trip #![forbid(unsafe_code)]), but it might be a practical choice.

[steffahn]

FYI, I came across this issue from a context of discussing the role of Send/Sync on IRLO. Asynchronous tasks have a similar problem as Ungil, as they would love to behave more like threads in putting up Send/Sync-like barriers, in order to allow non-thread-safe API (like Cell or Rc) to be used task-internally. But thread-locals (in that case thread-locals in general, not just scoped ones) are a major problem for that.

In my reply in that thread I argued that a Send-like trait for non-physical-thread-based memory boundaries, i.e. one that cannot be circumvented thread-internally with any form of thread locals, would be sufficient to offer a sound allow_threads, too. Arguably, one that has the same issue of overly cautiously requiring non-thread-safe data not to pass into the closure at all, not just non-python-memory-involving data, but at least it could be sound.

Of course that’s not any form of short-term solution; landing such a language feature would be a huge undertaking, unless we find a simpler way of keeping it backwards compatible.

---

Regarding invariance as a solution, look at this comment and my answer, which is an essentially-invariant situation. Invariant lifetimes can always be hidden behind a shorter covariant one using trait objects.

---

Regarding the unsafe scoping, I think that’s an unfortunate effect of current unsafe block syntax. One can employ workarounds such as requiring an unsafe-to-construct token to be passed to allow_threads like

py.allow_threads(unsafe { marker_name_tbd() }, || { … })

[davidhewitt]

Thank you, I'll try to digest that thread in the near future!

The unsafe-to-construct token is probably the better workaround than my unsafe_allow_threads suggestion 😂