Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
actix-service Cell::get_mut() is unsound

@Shnatsel wrote...

The following code is unsound:

https://github.com/actix/actix-net/blob/7c5fa25b23a802b27e8066caf4e01e3c2cedeb35/actix-service/src/cell.rs#L34-L36

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 here

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

@cdbattags wrote...

Issue #83 contained some pretty good discussion surrounding unsafe rust and I don't think we should ignore or delete a constructive conversation.

@fafhrd91, please please please can we try not to use phrases like "this patch is boring"?

@fafhrd91 wrote...

Next stage will be deleting organization. Please, do not continue

@cdbattags wrote...

Why the hostility though? I know it's hard over text/internet/GitHub but I'm coming from a place of wanting this project to succeed.

@fafhrd91 wrote...

boring, because nobody actually wants to fix existing code, everyone just want to remove unsafe.

it is fixed, Actix-service 1.0.5 is released.

@cdbattags wrote...

Thank you, Nikolay! I think it was just a misunderstanding of tone.

I appreciate everything you've done for this project this far and I think this is the future of server-side web development for sure.

Any sort of list of TODOs for the actix ecosystem right now?

@fafhrd91 wrote...

It is not very interesting to continue development. I will work only on functionality that is required for my job.

@cdbattags wrote...

@carllerche am I right to assume that tokio org would potentially take this project under its wing?

@fafhrd91 wrote...

We use it for large project. If this project goes to other org then we’ll change to fully private development.

@cdbattags wrote...

Maybe time to pass it off then? I think most of us want to do right by you and the project and get things under the org umbrella but Rust for sure needs a defacto web server (i.e. Express for the Node ecosystem) and that'll require much more continuous dev down the road, no?

Also, might I ask what's Microsoft's current stake in this project?

@owner

This comment has been minimized.

Copy link

@owner owner commented Jan 19, 2020

What the fuck is going on?

@ivbondarev

This comment has been minimized.

Copy link

@ivbondarev ivbondarev commented Jan 19, 2020

🥇

@RumataEstor

This comment has been minimized.

Copy link

@RumataEstor RumataEstor commented Jan 22, 2020

If I used it for a larger project, I would be grateful for the bugs found, especially security related bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.