@Shnatsel wrote...
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 wrote...
This is internal code. There is no repeated call to get_mut() anywhere in code
@fafhrd91 wrote...
Please, don’t start
@Shnatsel wrote...
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 wrote...
@fafhrd91, I don't think "unsound" was meant to be personal or offensive.
Why close this so quickly?
@fafhrd91 wrote...
I need unit test that shows UB.
@Nemo157 wrote...
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 wrote...
I think it's worth to fix, re-opening.
@fafhrd91 wrote...
@Nemo157 @repnop nice! finally some real code!
@cdbattags wrote...
Hehehe thanks @fafhrd91, @Nemo157 and @repnop!
@fafhrd91 wrote...
should be fixed in master
@Nemo157 wrote...
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 wrote...
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 wrote...
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 wrote...
@Nemo157 new playground panics with BorrowMutError, is that what should happen?
@fafhrd91 wrote...
@repnop i dont see how this could be fixed internally
@Nemo157 wrote...
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 wrote...
Would it be possible to simply change it to Rc<RefCell<T>>?
@Nemo157 wrote...
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 wrote...
this patch is boring
@CJKay wrote...
this patch is boring
So is resolving silent data corruption.
@bbqsrc wrote...
@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 wrote...
@bbqsrc I understand your point, but that doesn't mean you should use offensive words.
What the fuck is going on?