Surprising soundness trouble around `PollFn`

42

Please keep this constructive.

It's literally a safe function returning a struct with safe methods. Keep down your hyperbole and don't try to redefine the terminology to suit your point.

It's one of the simplest APIs in Rust, which can be written by almost every person writing Rust. If you argue that something that trivial need jumps through hoops to avoid unsafety, then sorry, but Rust's value proposition doesn't hold. I, and most people writing Rust, fully expect that if we write safe code (and poll_fn is 100% safe), then we can be certain that it won't cause safety issues. poll_fn is way too trivial_ to be a bad API. If you say that it's bad, it means that the basic building blocks of async Rust are bad. Pin is a bad API? Unpin is a bad API? I could even agree with that, but it can't be changed at this point.

But of course the above point is entirely moot, because nobody is writing the kind of code you talk about. Why? Stay tuned!

It's far and beyond not the common case. It's very much a tiny minority. Of that minority, basically everyone does it right and pins the !Unpin capture right outside the closure. There are only two crates which did it wrong: futures and tokio. Specifically, tokio::{join, try_join, select} and futures::{join, try_join}. And I'm quite certain that if they implemented those primitives as functions, rather than macros, then they would do everything right, like dozens of other crates, because those unsafe calls within the closure would stick out like a sore thumb.

Don't believe me? Check it yourself. Grep for \bpoll_fn\s*\( in the crates, excluding comments and string literals.

Here are the results of running the search on a simple project with the following typical dependencies: async-executor, async-io, async-std, blocking, futures, futures-macro, h2, hyper, stdlib, tokio, tonic, tower, tower-http.

This includes the two major async runtimes, and the http protocol. There are 80 total uses of poll_fn (this is a text search, so it includes all implementations of poll_fn). Of these,

  • 54 closures don't use Pin at all!
  • 9 cases where the value is pinned before moving into the closure.
  • 2 cases where we pin inside the closure an owned value which was created inside the closure itself.
  • 11 cases where the captured value is Unpin, and so can be safely pinned with Pin::new.
  • and 5 cases with the UB: the macros in tokio and futures.

Ok, maybe that project isn't that representative? How about something more real-world? Say, tauri? Nah, it uses a subset of those crates. Ok, deno? There are only 9 poll_fn calls in the entire 120,000-line codebase, excluding dependencies. None of those calls uses pinning.

Ok, let's look at Gecko. All poll_fn calls are in third-party dependencies, most of which we already counted above. Still, the count is around

  • 141 closures don't use Pin at all!
  • 4 cases where the value is pinned before moving into the closure.
  • 6 cases where we pin inside the closure an owned value which was created inside the closure itself.
  • 12 cases where the captured value is Unpin, and so can be safely pinned with Pin::new.
  • and 5 familiar errors in tokio and futures.

I may have miscounted a couple of functions, but I'm certain that the only UB-exploiting ones are the already familiar macros. As you see, 84% of function calls don't use any pinning. This is expected if you think why you would even use a poll_fn. In most cases you have some custom polling logic which doesn't involve any pre-existing pollables at all. In a smaller but still large number of cases you want to delegate to some other pollable object (stream, channel etc), which isn't itself a future. And of course those objects are Unpin. Why would you bother dragging along !Unpin data and suffer the ergonomic and safety issues?

And again, basically all people who need to pin a capture have done so soundly. Why wouldn't you use pin! outside the closure, when it's safe and easy to do? The exception - those pesky macros, which somehow have escaped proper audit for 5 years. You want to remove error-prone APIs? Do something with the damn macro system! It's impossible to analyze!

You're saying that it's a low-level API, great. So we can expect to see it more in low-level crates. Let's take everything that lib.rs gives us for the "async runtime" request. Add all crates to an empty project, drop a couple which have irresolvable dependency conflicts. We are left with about 100 crates. Of these, only around 40 ever call poll_fn (again, regardless of where it comes from). There are total 322 poll_fn calls. However, some heavy users were already counted above (e.g. tokio and async_std).

I won't be counting here. I have just looked through all use cases in new crates, and I can say that basically all of them either don't use pinning at all (again, vast majority), or use Pin::new, which is safe. There are just two exceptions: futures_micro and monoio. Both cases have errors in the macros. Both macros are very obviously ripped off the corresponding macros in futures or tokio.

I don't have the infrastructure to analyze the entirety of crates.io, but I'm quite certain that the results will reproduce.

So we have basically one culprit (which likely was tokio, because it's the flagship of async), which was copied without much thought in a couple of places. Everyone else uses the API soundly. Nobody depends on unsound and brittle "not-really-pinning" of the closure's captures.

So who exactly do you expect to protect from themselves with this API break? And why would you encourage unsound code, when nobody depends or wants to depend on it?

4 Likes