DynFuture is unsound

[taiki-e]

Describe the bug

https://github.com/AldaronLau/pasts/blob/675bd309d609111fac52889602e31c9609e7f2ea/src/dyn_future.rs#L21-L22

This is Pin<&mut Type> to Pin<Field> projection and is unsound if dyn Future is not Unpin (you can move dyn Future after DynFuture dropped).

repro: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=b2564e36d16d7b2a8f14f763a9477a85

The correct projection is Pin<&mut Type> to Pin<&mut Field>. In DynFuture, it is Pin<&mut DynFuture<'_, T>> to Pin<&mut &mut dyn Future>, and it needs to add dyn Future: Unpin bounds to convert Pin<&mut &mut dyn Future> to Pin<&mut dyn Future>.

Solution
Change DynFuture from &'a mut dyn Future<Output = T> to &'a mut (dyn Future<Output = T> + Unpin).
https://github.com/AldaronLau/pasts/blob/675bd309d609111fac52889602e31c9609e7f2ea/src/dyn_future.rs#L14

Additional context
I have fixed a similar bug on tokio in the past: tokio-rs/tokio#2612
Also, #2, previously reported by @udoprog and already fixed by @AldaronLau, seems to be the same problem as this.

[AldaronLau]

@taiki-e Thanks for finding this! This change would break at least the counter.rs example, which uses DynFuture as a replacement for the pin_mut() macro from pin_utils. I'm not sure yet how to rewrite the example without using Boxes, if possible, or find an alternative solution, if it exists, but I will have to do one of them.

[taiki-e]

Note that the code that is broken by this change is the code that is actually affected by this unsoundness.
If you don't want to depends on pin-utils, consider providing a macro equivalent to pin_utils::pin_mut (i.e., a macro that does stack pinning). That's what futures, tokio, smol, futures-lite, etc. do.
(As far as I know, there is no common way to safe abstract for pinning not Unpin value other than macro-based stack pinning or allocating it to a heap.)

BTW why don't you want to use the pin_mut macro in examples? pin_mut macro is completely sound (other pin-utils's macros are unsafe, but are deprecated and will be removed) and pin-utils itself is a very light dependency.

[taiki-e]

As far as I know, there is no common way to safe abstract for pinning not Unpin value other than macro-based stack pinning or allocating it to a heap.

Well, I think the following function can do this, it won't probably be very convenient if compared to macro.

pub fn pin<T, U>(mut v: T, f: impl FnOnce(Pin<&mut T>) -> U) -> U {
   let v = unsafe { Pin::new_unchecked(&mut v) };
    f(v)
}

[AldaronLau]

The original idea behind pasts was to make something that works like how the futures crate works but without any dependencies. Later, I decided it would be interesting to reimplement using traits instead of macros. I was pretty happy with how it turned out (except for the unsoundness issues discovered that I have now fixed, except for this one). The reason I don't like pin_mut!() is because it inserts unsafe code (although sound), into functions making it impossible to use #[forbid(unsafe_code)] in higher level applications. I have already been criticized a bit from other rustaceans for caring about this, but I still believe it's important to keep this as an option. Originally, I had implemented pin_mut!() in pasts code, but using a hidden function (#[doc(hidden)]) marked as safe that was called from the macro. Of course actually using the function outside the macro is unsound, and I decided later that it wasn't an ideal solution and should be considered bad practice as it's an unsound public API despite being hidden.

I'm currently leaning towards using your function, I'll experiment with it and see how inconvenient it is.

[taiki-e]

into functions making it impossible to use #[forbid(unsafe_code)] in higher level applications.

Hmm, is this true even with the latest compilers? IIRC, in 1.29 and later compilers, macros that generate unsafe code are also compatible with forbid(unsafe_code), unless generate allow/warn/deny(unsafe_code).

Actually, pin-utils has a test to check compatible to forbid(unsafe_code)
(FWIW, unsafe_{pinned, unpinned} are not safe, so there is an explicit allow(unsafe_code) to make it incompatible with forbid(unsafe_code).)

[AldaronLau]

You appear to be correct. This was not the case last I checked.

[AldaronLau]

I have completely removed DynFut, and replaced it with the task!() macro. After having a look at the other unsafe code in the library, I noticed more unsoundness issues in the executor, which could also be fixed with a macros. I have reworked the entire public API, the changes are now on master. I ended up removing a lot of unsafe in the process, so hopefully less chances for unsoundness. Since DynFut no longer exists, I'm closing this issue and will release a new version soon. @taiki-e Thanks again for finding this issue!

[taiki-e]

@AldaronLau Thanks for the fix!