This seems to be causing panics in tokio's linked_list

[awused]

This crate really fills a hole my application needs, so thanks for making it.

I think !Unpin futures are inadvertently moved during cancel() calls here, and changing that take() to drop_in_place() and write(None) seems to fix it.

if (*task.future.get()).is_some() {
    task.future.get().drop_in_place();
    task.future.get().write(None);

See tokio-rs/tokio#6597
The link to this repository on crates.io is also broken.

[StoicDeveloper]

Thanks for the info! I'll fix this as soon as possible.

[Darksonn]

Please double check whether the drop_in_place is necessary. The call to write(None) may be enough to call the destructor of the previous value.

[StoicDeveloper]

I'm publishing a new version which includes these changes, which fix:

1. moving an !Unpin future in cancel()
2. fix the repo link, which was broken, as pointed out in the linked tokio issue.

However I also ran the crate through Miri, and the results seem to indicate further unsoundness issues, although where they're coming from isn't clear. I would have difficulty believing that they stem from the futures_timer crate or the code that I stole from futures_unordered.

[StoicDeveloper]

Here is the Miri output:

test mapped_futures::bi_multi_map_futures::tests::bi_multi_map_futures ... warning: integer-to-pointer cast
  --> /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-timer-3.0.2/src/native/arc_list.rs:88:43
   |
88 |         let head = unsafe { Arc::from_raw(head as *const Node<T>) };
   |                                           ^^^^^^^^^^^^^^^^^^^^^^ integer-to-pointer cast
   |
   = help: This program is using integer-to-pointer casts or (equivalently) `ptr::from_exposed_addr`,
   = help: which means that Miri might miss pointer bugs in this program.
   = help: See https://doc.rust-lang.org/nightly/std/ptr/fn.from_exposed_addr.html for more details on that operation.
   = help: To ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead.
   = help: You can then pass the `-Zmiri-strict-provenance` flag to Miri, to ensure you are not relying on `from_exposed_addr` semantics.
   = help: Alternatively, the `-Zmiri-permissive-provenance` flag disables this warning.
   = note: BACKTRACE on thread `futures-timer`:
   = note: inside `futures_timer::native::arc_list::ArcList::<futures_timer::native::timer::ScheduledTimer>::pop` at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-timer-3.0.2/src/native/arc_list.rs:88:43: 88:65
   = note: inside `<futures_timer::native::timer::Timer as futures_util::Future>::poll` at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-timer-3.0.2/src/native/timer.rs:177:32: 177:42
   = note: inside `futures_timer::native::global::run` at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-timer-3.0.2/src/native/global.rs:63:17: 63:51
   = note: inside closure at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-timer-3.0.2/src/native/global.rs:28:28: 28:45
   = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<{closure@futures_timer::native::global::HelperThread::new::{closure#0}}, ()>` at /home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys_common/backtrace.rs:155:18: 155:21
   = note: inside closure at /home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/thread/mod.rs:528:17: 528:78
   = note: inside `<std::panic::AssertUnwindSafe<{closure@std::thread::Builder::spawn_unchecked_<'_, '_, {closure@futures_timer::native::global::HelperThread::new::{closure#0}}, ()>::{closure#1}::{closure#0}}> as std::ops::FnOnce<()>>::call_once` at /home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panic/unwind_safe.rs:272:9: 272:19
   = note: inside `std::panicking::r#try::do_call::<std::panic::AssertUnwindSafe<{closure@std::thread::Builder::spawn_unchecked_<'_, '_, {closure@futures_timer::native::global::HelperThread::new::{closure#0}}, ()>::{closure#1}::{closure#0}}>, ()>` at /home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:552:40: 552:43
   = note: inside `std::panicking::r#try::<(), std::panic::AssertUnwindSafe<{closure@std::thread::Builder::spawn_unchecked_<'_, '_, {closure@futures_timer::native::global::HelperThread::new::{closure#0}}, ()>::{closure#1}::{closure#0}}>>` at /home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:516:19: 516:88
   = note: inside `std::panic::catch_unwind::<std::panic::AssertUnwindSafe<{closure@std::thread::Builder::spawn_unchecked_<'_, '_, {closure@futures_timer::native::global::HelperThread::new::{closure#0}}, ()>::{closure#1}::{closure#0}}>, ()>` at /home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:146:14: 146:33
   = note: inside closure at /home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/thread/mod.rs:527:30: 529:16
   = note: inside `<{closure@std::thread::Builder::spawn_unchecked_<'_, '_, {closure@futures_timer::native::global::HelperThread::new::{closure#0}}, ()>::{closure#1}} as std::ops::FnOnce<()>>::call_once - shim(vtable)` at /home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5: 250:71
   = note: inside `<std::boxed::Box<dyn std::ops::FnOnce()> as std::ops::FnOnce<()>>::call_once` at /home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:2016:9: 2016:52
   = note: inside `<std::boxed::Box<std::boxed::Box<dyn std::ops::FnOnce()>> as std::ops::FnOnce<()>>::call_once` at /home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:2016:9: 2016:52
   = note: inside `std::sys::pal::unix::thread::Thread::new::thread_start` at /home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/pal/unix/thread.rs:108:17: 108:64

error: Undefined Behavior: out-of-bounds pointer use: 0x195ec0[noalloc] is a dangling pointer (it has no provenance)
    --> /home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/sync.rs:2904:47
     |
2904 |             Some(unsafe { WeakInner { strong: &(*ptr).strong, weak: &(*ptr).weak } })
     |                                               ^^^^^^^^^^^^^^ out-of-bounds pointer use: 0x195ec0[noalloc] is a dangling pointer (it has no provenance)
     |
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
     = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
     = note: BACKTRACE on thread `mapped_futures:`:
     = note: inside `std::sync::Weak::<futures_timer::native::timer::Inner>::inner` at /home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/sync.rs:2904:47: 2904:61
     = note: inside `<std::sync::Weak<futures_timer::native::timer::Inner> as std::clone::Clone>::clone` at /home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/sync.rs:2972:30: 2972:42
     = note: inside `<futures_timer::native::timer::TimerHandle as std::clone::Clone>::clone` at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-timer-3.0.2/src/native/timer.rs:43:5: 43:34
     = note: inside `<futures_timer::native::timer::TimerHandle as std::default::Default>::default` at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-timer-3.0.2/src/native/timer.rs:307:23: 307:37
     = note: inside `futures_timer::Delay::new` at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-timer-3.0.2/src/native/delay.rs:37:49: 37:67
note: inside `mapped_futures::bi_multi_map_futures::tests::insert_millis`
    --> src/mapped_futures/bi_multi_map_futures.rs:366:38
     |
366  |         futures.insert(key.0, key.1, Delay::new(Duration::from_millis(millis)));
     |                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `mapped_futures::bi_multi_map_futures::tests::bi_multi_map_futures`
    --> src/mapped_futures/bi_multi_map_futures.rs:373:9
     |
373  |         insert_millis(&mut futures, (1, 2), 60);
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure
    --> src/mapped_futures/bi_multi_map_futures.rs:370:30
     |
369  |     #[test]
     |     ------- in this procedural macro expansion
370  |     fn bi_multi_map_futures() {
     |                              ^
     = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error; 1 warning emitted

error: test failed, to rerun pass `--lib`

Caused by:
  process didn't exit successfully: `/home/user/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner /home/user/Projects/mapped_futures/target/miri/x86_64-unknown-linux-gnu/debug/deps/mapped_futures-88a4390b468af7e3` (exit status: 1)
note: test exited abnormally; to see the full output pass --nocapture to the harness.

[StoicDeveloper]

The patched version 0.1.10 is up now. I accidentally published 0.1.9 with breaking api changes to include changes suggested in another issue, so that version has also been yanked. If I include those changes, I suppose it'll be with a version 1.0. However, I suspect MappedFutures might be merged into FuturesUnordered by then, in which case this crate will have no further updates.

[awused]

Please double check whether the drop_in_place is necessary. The call to write(None) may be enough to call the destructor of the previous value.

Yeah it wasn't clear from the code snippet alone but that's a *mut Option<Fut> so write won't run the destructor.

However I also ran the crate through Miri, and the results seem to indicate further unsoundness issues, although where they're coming from isn't clear. I would have difficulty believing that they stem from the futures_timer crate or the code that I stole from futures_unordered.

That actually is a futures_timer bug, it was fixed in 3.0.3 with async-rs/futures-timer#66.

[Darksonn]

You probably have a memory safety bug in the case where the destructor of the future panics. You should do this instead:

*task.future.get() = None;

This syntax will run the destructor of the previous value and then write None. It writes None even if the destructor panics.

[awused]

You probably have a memory safety bug in the case where the destructor of the future panics. You should do this instead:

Yeah that's my bad. But I'm going to open a bug against the rust documentation because I feel like I'm not the first or last person to make this mistake based on reading the drop_in_place docs.

[StoicDeveloper]

Please double check whether the drop_in_place is necessary. The call to write(None) may be enough to call the destructor of the previous value.

Ah, unfortunately the 0.1.10 fix includes write() but judging from [the docs] for write() it doesn't call the destructor, and so 0.1.10 includes that memory leak, which was fixed now in 0.1.11, which now includes the leak-on-panic problem as discussed above. In hindsight, I should be less trigger-happy with cargo publish. I'll leave those versions up, since I believe a memory leak is at least not unsound. The next published version will aim to fix:

• the memory leak which occurs during a destructor panic
• update dev dependency on futures-timer
• any additional unsoundness issues revealed by miri

[StoicDeveloper]

Version 0.1.12 is now published, and it:

• fixes the memory leak
• updates the dev dependency

I also ran miri now that futures-timer is updated, and no additional unsoundness was discovered. Thank you everyone for your assistance and for bringing these issues to my attention.

[StoicDeveloper]

While checking the FuturesUnordered PR branch in Miri, additional unsoundness was found, which comes from the potential for a dropped task to still have a pointer in the ReadyToRunQueue, which will get dereferenced during polling. As a result, this crate should still be treated as unsound an not used for production code.

EDIT: I've determined that this issue was specific to the PR branch which attempts to merge MappedFutures into the FuturesUnordered project. There was some divergence between the two, which introduced unsoundness into that PR, but which does not exist in this crate

[StoicDeveloper]

I opened a new issue here: #5

[workingjubilee]

I'll leave those versions up, since I believe a memory leak is at least not unsound.

Correct. Leaky code is not great but preferable to unsound code.