-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Description
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