This is internal code. There is no repeated call to get_mut() anywhere in code

Please, don’t start

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.

@fafhrd91, I don't think "unsound" was meant to be personal or offensive.

Why close this so quickly?

I need unit test that shows UB. Unit test must use public api.

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)

I think it's worth to fix, re-opening.

@Nemo157 @repnop nice! finally some real code!

Hehehe thanks @fafhrd91, @Nemo157 and @repnop!

should be fixed in master

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).

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

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?

@Nemo157 new playground panics with BorrowMutError, is that what should happen?

@repnop i dont see how this could be fixed internally

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. (The first is within the AndThenServiceResponse::poll stack frame, at and_then.rs:97, the second is at the shown snippet where MIRI is raising an error).

Would it be possible to simply change it to Rc<RefCell<T>>? Alternatively, could it be made unsafe perhaps? This would mean that the safety would have to be evaluated at the call-site.

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).

this patch is boring

this patch is boring

So is resolving silent data corruption.

@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?