Undefined behavior: do_in_place_scope tries to drop scope when scope reference passed to user is still alive

[SOF3]

As my project is a bit complex, I have not been successful in creating a minimal reproducing example yet, but an unstripped example run can be found below.

GitHub CI run: https://github.com/SOF3/dynec/actions/runs/3861470087/jobs/6582464784#step:4:487

Source: https://github.com/SOF3/dynec/blob/2e83e3433b54977277a7ba340ac216a60b7da526/src/scheduler/executor.rs#L80-L121

Miri output:

error: Undefined Behavior: not granting access to tag <18216744> because that would remove [SharedReadOnly for <18182613>] which is protected because it is an argument of call 4729027
    --> /home/runner/.rustup/toolchains/nightly-2022-11-22-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/sync.rs:1706:13
     |
1706 |     fn drop(&mut self) {
     |             ^^^^^^^^^ not granting access to tag <18216744> because that would remove [SharedReadOnly for <18182613>] which is protected because it is an argument of call 4729027
     |
     = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
     = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <18216744> was created by a SharedReadWrite retag at offsets [0x0..0x8]
    --> src/scheduler/executor.rs:80:13
     |
80   | /             pool.in_place_scope(|scope| {
81   | |                 let (main_ealloc_shard, worker_ealloc_shards) = ealloc_shards
82   | |                     .split_last_mut()
83   | |                     .expect("ealloc_shards.len() == self.concurrency + 1");
...    |
120  | |                 )
121  | |             });
     | |______________^
help: <18182613> is this argument
    --> /home/runner/.rustup/toolchains/nightly-2022-11-22-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/unix/thread.rs:108:17
     |
108  |                 Box::from_raw(main as *mut Box<dyn FnOnce()>)();
     |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     = note: BACKTRACE:
     = note: inside `<std::sync::Arc<rayon_core::registry::Registry> as std::ops::Drop>::drop` at /home/runner/.rustup/toolchains/nightly-2022-11-22-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/sync.rs:1706:13
     = note: inside `std::ptr::drop_in_place::<std::sync::Arc<rayon_core::registry::Registry>> - shim(Some(std::sync::Arc<rayon_core::registry::Registry>))` at /home/runner/.rustup/toolchains/nightly-2022-11-22-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:490:1
     = note: inside `std::ptr::drop_in_place::<rayon_core::scope::ScopeBase<'_>> - shim(Some(rayon_core::scope::ScopeBase<'_>))` at /home/runner/.rustup/toolchains/nightly-2022-11-22-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:490:1
     = note: inside `std::ptr::drop_in_place::<rayon::Scope<'_>> - shim(Some(rayon::Scope<'_>))` at /home/runner/.rustup/toolchains/nightly-2022-11-22-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:490:1
     = note: inside `rayon_core::scope::do_in_place_scope::<'_, [closure@src/scheduler/executor.rs:80:33: 80:40], ()>` at /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.10.1/src/scope/mod.rs:441:1
     = note: inside `rayon::ThreadPool::in_place_scope::<'_, [closure@src/scheduler/executor.rs:80:33: 80:40], ()>` at /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.10.1/src/thread_pool/mod.rs:284:9
note: inside `scheduler::executor::Executor::execute_full_cycle::<tracer::Aggregate<(tracer::Log, scheduler::tests::RunCounterTracer, tracer::Aggregate<(scheduler::tests::UnmarkCounterTracer, scheduler::tests::MaxConcurrencyTracer)>)>>` at src/scheduler/executor.rs:80:13
    --> src/scheduler/executor.rs:80:13
     |
80   | /             pool.in_place_scope(|scope| {
81   | |                 let (main_ealloc_shard, worker_ealloc_shards) = ealloc_shards
82   | |                     .split_last_mut()
83   | |                     .expect("ealloc_shards.len() == self.concurrency + 1");
...    |
120  | |                 )
121  | |             });
     | |______________^

[SOF3]

I have a bit of unsafe code in the linked project, but I believe none of those unsafe code are triggered in the unit test that triggers this UB. The error seems to suggest that scope was leaked, but I didn't do anything about it other than calling scope.spawn like normal.

[cuviper]

I believe this is a variant of #938 and #952, especially #952 (comment). Basically, when we call any Latch::set(&self), its actions cause the thread that owns the latch to unblock and proceed to drop it. If we're unlucky though, we'll still be in set with a now-dangling reference on our hands.

This should be ok in practice since rust-lang/rust#98017 stopped marking references to !Freeze types (containing any UnsafeCell) as LLVM dereferenceable. However, IIRC current miri rules for Stacked Borrows are only allowing that dangling for the part of the struct that is within UnsafeCell, so dropping other fields is flagged as UB. But do note that the message itself states that these rules are still experimental!

[cuviper]

I hope that #1011 will fix this, but it would be great if you could test that with your minimal example, or even with your complete project.

[SOF3]

It seems miri no longer complains about the UB after using the commit in #1011 indeed. However I can't be sure since miri takes too long and never exits, but no errors are reported anymore.