Freeing the receiver (&self) in CancellationTokenState is probably UB

[saethlin]

Version
latest commit, and/or anything after #2263

Description
decrement_refcount and remove_parent_ref in tokio-util/src/sync/cancellation_token.rs are probably unsound because they free the receiver, which causes the receiver to become a dangling reference:

tokio/tokio-util/src/sync/cancellation_token.rs

Lines 551 to 581 in 83477c7

	fn decrement_refcount(&self, current_state: StateSnapshot) -> StateSnapshot {
	let current_state = self.atomic_update_state(current_state, |mut state: StateSnapshot| {
	state.refcount -= 1;
	state
	});
	// Drop the State if it is not referenced anymore
	if !current_state.has_refs() {
	// Safety: `CancellationTokenState` is always stored in refcounted
	// Boxes
	let _ = unsafe { Box::from_raw(self as *const Self as *mut Self) };
	}
	current_state
	}
	fn remove_parent_ref(&self, current_state: StateSnapshot) -> StateSnapshot {
	let current_state = self.atomic_update_state(current_state, |mut state: StateSnapshot| {
	state.has_parent_ref = false;
	state
	});
	// Drop the State if it is not referenced anymore
	if !current_state.has_refs() {
	// Safety: `CancellationTokenState` is always stored in refcounted
	// Boxes
	let _ = unsafe { Box::from_raw(self as *const Self as *mut Self) };
	}
	current_state
	}

I tried this code:

In tokio-util:

RUST_BACKTRACE=0 MIRIFLAGS="-Zmiri-disable-isolation -Zmiri-panic-on-unsupported" cargo miri test --no-fail-fast

Add --all-features if you don't want a pile of compile errors too. -Zmiri-panic-on-unsupported with --no-fail-fast ask cargo miri test to power through any code that Miri doesn't support.

I expected to see this happen: No errors

Instead, this happened: Miri reports UB under Stacked Borrows

I'm including my prototype diagnostics for this. They're a bit unpolished, and SB without raw pointer tracking is exceedingly hard to diagnose, and this a protector error which are frustratingly nonlocal. So I'm trying my best here, but I know it's still not awesome.

If I turn on raw pointer tracking to get a better error, I find other errors before this one along the same code path in tokio-util. I'm reporting this one because I think it's more concerning.

     Running tests/sync_cancellation_token.rs (/tmp/tokio/target/miri/x86_64-unknown-linux-gnu/debug/deps/sync_cancellation_token-c791a3e46ae795b7)

running 6 tests
test cancel_child_token_through_parent ... error: Undefined Behavior: not granting access to tag <untagged> because incompatible item is protected: [SharedReadOnly for <173321> (call 51979)]
   --> /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/alloc/src/boxed.rs:985:9
    |
985 |         Box(unsafe { Unique::new_unchecked(raw) }, alloc)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag <untagged> because incompatible item is protected: [SharedReadOnly for <173321> (call 51979)]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the 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: tag was most recently created at offsets [0x0..0x50]
   --> /tmp/tokio/tokio-util/src/sync/cancellation_token.rs:561:44
    |
561 |             let _ = unsafe { Box::from_raw(self as *const Self as *mut Self) };
    |                                            ^^^^
help: tag was later invalidated at offsets [0x0..0x50]
   --> /tmp/tokio/tokio-util/src/sync/cancellation_token.rs:561:30
    |
561 |             let _ = unsafe { Box::from_raw(self as *const Self as *mut Self) };
    |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: this tag was also created here at offsets [0x0..0x50]
   --> /tmp/tokio/tokio-util/src/sync/cancellation_token.rs:252:31
    |
252 |         let child_token_ptr = Box::into_raw(child_token_state);
    |                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: <173294> was protected due to a tag which was created here
   --> /tmp/tokio/tokio-util/src/sync/cancellation_token.rs:122:25
    |
122 |         current_state = inner.decrement_refcount(current_state);
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: this protector is live for this call
   --> /tmp/tokio/tokio-util/src/sync/cancellation_token.rs:551:5
    |
551 | /     fn decrement_refcount(&self, current_state: StateSnapshot) -> StateSnapshot {
552 | |         let current_state = self.atomic_update_state(current_state, |mut state: StateSnapshot| {
553 | |             state.refcount -= 1;
554 | |             state
...   |
564 | |         current_state
565 | |     }
    | |_____^
    = note: inside `std::boxed::Box::<tokio_util::sync::cancellation_token::CancellationTokenState>::from_raw_in` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/alloc/src/boxed.rs:985:9
    = note: inside `std::boxed::Box::<tokio_util::sync::cancellation_token::CancellationTokenState>::from_raw` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/alloc/src/boxed.rs:929:18
note: inside `tokio_util::sync::cancellation_token::CancellationTokenState::decrement_refcount` at /tmp/tokio/tokio-util/src/sync/cancellation_token.rs:561:30
   --> /tmp/tokio/tokio-util/src/sync/cancellation_token.rs:561:30
    |
561 |             let _ = unsafe { Box::from_raw(self as *const Self as *mut Self) };
    |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `<tokio_util::sync::CancellationToken as std::ops::Drop>::drop` at /tmp/tokio/tokio-util/src/sync/cancellation_token.rs:122:25
   --> /tmp/tokio/tokio-util/src/sync/cancellation_token.rs:122:25
    |
122 |         current_state = inner.decrement_refcount(current_state);
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: inside `std::ptr::drop_in_place::<tokio_util::sync::CancellationToken> - shim(Some(tokio_util::sync::CancellationToken))` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:448:1
note: inside `cancel_child_token_through_parent` at tokio-util/tests/sync_cancellation_token.rs:78:1
   --> tokio-util/tests/sync_cancellation_token.rs:78:1
    |
78  | }
    | ^
note: inside closure at tokio-util/tests/sync_cancellation_token.rs:43:1
   --> tokio-util/tests/sync_cancellation_token.rs:43:1
    |
42  |   #[test]
    |   ------- in this procedural macro expansion
43  | / fn cancel_child_token_through_parent() {
44  | |     let (waker, wake_counter) = new_count_waker();
45  | |     let token = CancellationToken::new();
46  | |
...   |
77  | |     );
78  | | }
    | |_^
    = 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 previous error

[Darksonn]

Yes, that is definitely unsound. You can't just free something when there are still references to it.

[Darksonn]

Closed by #4652.