<unknown> commented: The following code is unsound:
This uses Rc::as_ref() to obtain a reference to the underlying data, which does not guarantee uniqueness. It is possible to obtain several mutable references to the same memory location by calling this function repeatedly:
let mycell = Cell::new(vec![1,2,3]);
let ref1 = mysell.get_mut();
let ref2 = mysell.get_mut(); // obtained a second mutable reference; UB starts hereThis may lead to arbitrary memory errors triggered from safe code, the most common of which would be use-after-free. These two references do not need to exist in the same function to trigger undefined behavior, they only need to exist at the same point in time.
A proper way to implement Cell:get_mut() would be calling Rc::get_mut() which guarantees uniqueness instead of Rc::as_ref().
fafhrd91 commented: This is internal code. There is no repeated call to get_mut() anywhere in code
fafhrd91 commented: Please, don’t start
Shnatsel commented: These two references do not need to exist in the same function to trigger undefined behavior, they only need to exist at the same point in time.
An easy way to see if this is a problem in practice is replace .as_ref() with .get_mut().unwrap() and see if anything panics.
cdbattags commented: @fafhrd91, I don't think "unsound" was meant to be personal or offensive.
Why close this so quickly?
fafhrd91 commented: I need unit test that shows UB.
Nemo157 commented:
The unsoundness of the Cell API is publicly exposed through AndThenService::{clone, call}. The code in this playground exhibits MIRI detected UB from multiple unrelated &mut First borrows existing simultaneously while using only the safe public API of actix-service.
Running the code via cargo miri results in:
error: Miri evaluation error: not granting access to tag <untagged> because incompatible item is protected: [Unique for <7869> (call 4775)]
--> /Users/nemo157/.cargo/registry/src/github.com-1ecc6299db9ec823/actix-service-1.0.2/src/cell.rs:35:18
|
35 | unsafe { &mut *self.inner.as_ref().get() }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Miri evaluation error: not granting access to tag <untagged> because incompatible item is protected: [Unique for <7869> (call 4775)]
|
= note: inside call to `actix_service::cell::Cell::<(First, &mut Second)>::get_mut` at /Users/nemo157/.cargo/registry/src/github.com-1ecc6299db9ec823/actix-service-1.0.2/src/and_then.rs:54:29
= note: inside call to `<actix_service::and_then::AndThenService<First, &mut Second> as actix_service::Service>::call` at /Users/nemo157/.cargo/registry/src/github.com-1ecc6299db9ec823/actix-service-1.0.2/src/pipeline.rs:160:9
(@repnop identified that AndThenService allowed accessing these APIs and helped developing the PoC)
JohnTitor commented: I think it's worth to fix, re-opening.
fafhrd91 commented: @Nemo157 @repnop nice! finally some real code!
cdbattags commented: Hehehe thanks @fafhrd91, @Nemo157 and @repnop!
fafhrd91 commented: should be fixed in master
Nemo157 commented:
It's possible to get around the 'static bound by using Arc, see this playground (I've also removed the unnecessary &mut Second, that was where I was trying to trigger the UB but miri caught the duplicate &mut (First, &mut Second) before it even got to checking the &mut Second itself).
repnop commented:
its also worth noting that adding a + 'static bound is a breaking change. I think ideally this should be fixed internally in a way that doesn't break the public API, especially in a patch version if possible
Shnatsel commented: Undefined behavior occurs when external code attempts to obtain two mutable references to the same data. The lifetime of the data is irrelevant to this issue.
If Rc<RefCell> were used instead of a custom implementation it would panic on the provided testcase instead of triggering undefined behavior. It should be possible to bring the behavior of the current implementation in line with Rc<RefCell>.
If panicking is undesirable, it is possible to return Option or Result from this function instead of panicking, see RefCell::try_borrow_mut for an example of such API.
I wonder, what was the original rationale for migrating from Rc<RefCell> to a custom implementation?
fafhrd91 commented: @Nemo157 new playground panics with BorrowMutError, is that what should happen?
fafhrd91 commented: @repnop i dont see how this could be fixed internally
Nemo157 commented:
If you run the code from the playground under MIRI it fails before the BorrowMutError with:
error: Miri evaluation error: not granting access to tag <untagged> because incompatible item is protected: [Unique for <7923> (call 4813)]
--> /Users/nemo157/.cargo/git/checkouts/actix-net-8b378701d4b3767e/5940731/actix-service/src/cell.rs:35:18
|
35 | unsafe { &mut *self.inner.as_ref().get() }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Miri evaluation error: not granting access to tag <untagged> because incompatible item is protected: [Unique for <7923> (call 4813)]
|
= note: inside call to `actix_service::cell::Cell::<(First, Second)>::get_mut` at /Users/nemo157/.cargo/git/checkouts/actix-net-8b378701d4b3767e/5940731/actix-service/src/and_then.rs:54:29
= note: inside call to `<actix_service::and_then::AndThenService<First, Second> as actix_service::Service>::call` at /Users/nemo157/.cargo/git/checkouts/actix-net-8b378701d4b3767e/5940731/actix-service/src/pipeline.rs:160:9
= note: inside call to `<actix_service::pipeline::Pipeline<actix_service::and_then::AndThenService<First, Second>> as actix_service::Service>::call` at src/main.rs:36:18
= note: inside call to `<Second as actix_service::Service>::call` at /Users/nemo157/.cargo/git/checkouts/actix-net-8b378701d4b3767e/5940731/actix-service/src/and_then.rs:97:31
= note: inside call to `<actix_service::and_then::AndThenServiceResponse<First, Second> as core::future::future::Future>::poll` at src/main.rs:55:35
= note: inside call to `block_on::<actix_service::and_then::AndThenServiceResponse<First, Second>>` at src/main.rs:47:5
= note: inside call to `main` at /Users/nemo157/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/src/libstd/rt.rs:67:34
because at this point there exist two independent &mut (First, Second) on the stack referencing the same tuple.
Restioson commented:
Would it be possible to simply change it to Rc<RefCell<T>>?
Nemo157 commented:
As a PoC this patch applied to actix-net passes all tests, and when the second playground is run against it under Miri it soundly fails with thread 'main' panicked at 'already borrowed: BorrowMutError' from within the AndThenServiceResponse. Presumably this requires benchmarking/more exhaustive testing which I don't have time to do, but if someone wants to take the patch and get it merged feel free (I license it under Apache-2.0 OR MIT, though I don't consider it to be creative enough to be copyrightable).
fafhrd91 commented: this patch is boring
CJKay commented:
this patch is boring
So is resolving silent data corruption.
bbqsrc commented: @fafhrd91 seriously? Please just stop writing Rust. You do not respect semver, you do not respect soundness, so why are you using a language predominantly based around doing these things right?
JohnTitor commented: @bbqsrc I understand your point, but that doesn't mean you should use offensive words.