From 5d3207830790420468b1e4f88d6a607df9e3379d Mon Sep 17 00:00:00 2001 From: "Sergey \"Shnatsel\" Davidoff" Date: Sun, 19 Jul 2020 16:25:10 +0200 Subject: [PATCH 1/3] Remove the unsound custom Cell and replace it with Rc - the way it was before the introduction of a custom cell. Heavily based on the patch by Wim Looman https://gist.github.com/Nemo157/b495b84b5b2f3134d19ad71ba5002066 --- actix-service/src/and_then.rs | 17 ++++---- actix-service/src/and_then_apply_fn.rs | 22 +++++----- actix-service/src/apply_cfg.rs | 28 +++++++------ actix-service/src/cell.rs | 57 -------------------------- actix-service/src/lib.rs | 1 - actix-service/src/then.rs | 16 ++++---- 6 files changed, 43 insertions(+), 98 deletions(-) delete mode 100644 actix-service/src/cell.rs diff --git a/actix-service/src/and_then.rs b/actix-service/src/and_then.rs index 74b64b3892..bf402e8f01 100644 --- a/actix-service/src/and_then.rs +++ b/actix-service/src/and_then.rs @@ -1,16 +1,17 @@ use std::future::Future; use std::pin::Pin; use std::rc::Rc; +use std::cell::RefCell; use std::task::{Context, Poll}; use super::{Service, ServiceFactory}; -use crate::cell::Cell; + /// Service for the `and_then` combinator, chaining a computation onto the end /// of another service which completes successfully. /// /// This is created by the `ServiceExt::and_then` method. -pub(crate) struct AndThenService(Cell<(A, B)>); +pub(crate) struct AndThenService(Rc>); impl AndThenService { /// Create new `AndThen` combinator @@ -19,7 +20,7 @@ impl AndThenService { A: Service, B: Service, { - Self(Cell::new((a, b))) + Self(Rc::new(RefCell::new((a, b)))) } } @@ -40,7 +41,7 @@ where type Future = AndThenServiceResponse; fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { - let srv = self.0.get_mut(); + let mut srv = self.0.borrow_mut(); let not_ready = !srv.0.poll_ready(cx)?.is_ready(); if !srv.1.poll_ready(cx)?.is_ready() || not_ready { Poll::Pending @@ -51,7 +52,7 @@ where fn call(&mut self, req: A::Request) -> Self::Future { AndThenServiceResponse { - state: State::A(self.0.get_mut().0.call(req), Some(self.0.clone())), + state: State::A(self.0.borrow_mut().0.call(req), Some(self.0.clone())), } } } @@ -72,7 +73,7 @@ where A: Service, B: Service, { - A(#[pin] A::Future, Option>), + A(#[pin] A::Future, Option>>), B(#[pin] B::Future), Empty, } @@ -90,9 +91,9 @@ where match this.state.as_mut().project() { StateProj::A(fut, b) => match fut.poll(cx)? { Poll::Ready(res) => { - let mut b = b.take().unwrap(); + let b = b.take().unwrap(); this.state.set(State::Empty); // drop fut A - let fut = b.get_mut().1.call(res); + let fut = b.borrow_mut().1.call(res); this.state.set(State::B(fut)); self.poll(cx) } diff --git a/actix-service/src/and_then_apply_fn.rs b/actix-service/src/and_then_apply_fn.rs index de0cfac9c7..105d7ed77c 100644 --- a/actix-service/src/and_then_apply_fn.rs +++ b/actix-service/src/and_then_apply_fn.rs @@ -2,9 +2,9 @@ use std::future::Future; use std::marker::PhantomData; use std::pin::Pin; use std::rc::Rc; +use std::cell::RefCell; use std::task::{Context, Poll}; -use crate::cell::Cell; use crate::{Service, ServiceFactory}; /// `Apply` service combinator @@ -16,7 +16,7 @@ where Fut: Future>, Err: From + From, { - srv: Cell<(A, B, F)>, + srv: Rc>, r: PhantomData<(Fut, Res, Err)>, } @@ -31,7 +31,7 @@ where /// Create new `Apply` combinator pub(crate) fn new(a: A, b: B, f: F) -> Self { Self { - srv: Cell::new((a, b, f)), + srv: Rc::new(RefCell::new((a, b, f))), r: PhantomData, } } @@ -67,7 +67,7 @@ where type Future = AndThenApplyFnFuture; fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { - let inner = self.srv.get_mut(); + let mut inner = self.srv.borrow_mut(); let not_ready = inner.0.poll_ready(cx)?.is_pending(); if inner.1.poll_ready(cx)?.is_pending() || not_ready { Poll::Pending @@ -77,7 +77,7 @@ where } fn call(&mut self, req: A::Request) -> Self::Future { - let fut = self.srv.get_mut().0.call(req); + let fut = self.srv.borrow_mut().0.call(req); AndThenApplyFnFuture { state: State::A(fut, Some(self.srv.clone())), } @@ -108,7 +108,7 @@ where Err: From, Err: From, { - A(#[pin] A::Future, Option>), + A(#[pin] A::Future, Option>>), B(#[pin] Fut), Empty, } @@ -129,10 +129,10 @@ where match this.state.as_mut().project() { StateProj::A(fut, b) => match fut.poll(cx)? { Poll::Ready(res) => { - let mut b = b.take().unwrap(); + let b = b.take().unwrap(); this.state.set(State::Empty); - let b = b.get_mut(); - let fut = (&mut b.2)(res, &mut b.1); + let (_, b, f) = &mut *b.borrow_mut(); + let fut = f(res, b); this.state.set(State::B(fut)); self.poll(cx) } @@ -255,11 +255,11 @@ where if this.a.is_some() && this.b.is_some() { Poll::Ready(Ok(AndThenApplyFn { - srv: Cell::new(( + srv: Rc::new(RefCell::new(( this.a.take().unwrap(), this.b.take().unwrap(), this.f.clone(), - )), + ))), r: PhantomData, })) } else { diff --git a/actix-service/src/apply_cfg.rs b/actix-service/src/apply_cfg.rs index cee7b8c7e1..1fa4f0eebd 100644 --- a/actix-service/src/apply_cfg.rs +++ b/actix-service/src/apply_cfg.rs @@ -2,8 +2,9 @@ use std::future::Future; use std::marker::PhantomData; use std::pin::Pin; use std::task::{Context, Poll}; +use std::rc::Rc; +use std::cell::RefCell; -use crate::cell::Cell; use crate::{Service, ServiceFactory}; /// Convert `Fn(Config, &mut Service1) -> Future` fn to a service factory @@ -26,7 +27,7 @@ where S: Service, { ApplyConfigService { - srv: Cell::new((srv, f)), + srv: Rc::new(RefCell::new((srv, f))), _t: PhantomData, } } @@ -53,7 +54,7 @@ where S: Service, { ApplyConfigServiceFactory { - srv: Cell::new((factory, f)), + srv: Rc::new(RefCell::new((factory, f))), _t: PhantomData, } } @@ -66,7 +67,7 @@ where R: Future>, S: Service, { - srv: Cell<(T, F)>, + srv: Rc>, _t: PhantomData<(C, R, S)>, } @@ -102,10 +103,8 @@ where type Future = R; fn new_service(&self, cfg: C) -> Self::Future { - unsafe { - let srv = self.srv.get_mut_unsafe(); - (srv.1)(cfg, &mut srv.0) - } + let (t, f) = &mut *self.srv.borrow_mut(); + f(cfg, t) } } @@ -117,7 +116,7 @@ where R: Future>, S: Service, { - srv: Cell<(T, F)>, + srv: Rc>, _t: PhantomData<(C, R, S)>, } @@ -157,7 +156,7 @@ where ApplyConfigServiceFactoryResponse { cfg: Some(cfg), store: self.srv.clone(), - state: State::A(self.srv.get_ref().0.new_service(())), + state: State::A(self.srv.borrow().0.new_service(())), } } } @@ -172,7 +171,7 @@ where S: Service, { cfg: Option, - store: Cell<(T, F)>, + store: Rc>, #[pin] state: State, } @@ -213,8 +212,11 @@ where }, StateProj::B(srv) => match srv.poll_ready(cx)? { Poll::Ready(_) => { - let fut = (this.store.get_mut().1)(this.cfg.take().unwrap(), srv); - this.state.set(State::C(fut)); + { + let (_, f) = &mut *this.store.borrow_mut(); + let fut = f(this.cfg.take().unwrap(), srv); + this.state.set(State::C(fut)); + } self.poll(cx) } Poll::Pending => Poll::Pending, diff --git a/actix-service/src/cell.rs b/actix-service/src/cell.rs deleted file mode 100644 index 3bcac0be06..0000000000 --- a/actix-service/src/cell.rs +++ /dev/null @@ -1,57 +0,0 @@ -//! Custom cell impl, internal use only -use std::task::{Context, Poll}; -use std::{cell::UnsafeCell, fmt, rc::Rc}; - -pub(crate) struct Cell { - inner: Rc>, -} - -impl Clone for Cell { - fn clone(&self) -> Self { - Self { - inner: self.inner.clone(), - } - } -} - -impl fmt::Debug for Cell { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.inner.fmt(f) - } -} - -impl Cell { - pub(crate) fn new(inner: T) -> Self { - Self { - inner: Rc::new(UnsafeCell::new(inner)), - } - } - - pub(crate) fn get_ref(&self) -> &T { - unsafe { &*self.inner.as_ref().get() } - } - - pub(crate) fn get_mut(&mut self) -> &mut T { - unsafe { &mut *self.inner.as_ref().get() } - } - - #[allow(clippy::mut_from_ref)] - pub(crate) unsafe fn get_mut_unsafe(&self) -> &mut T { - &mut *self.inner.as_ref().get() - } -} - -impl crate::Service for Cell { - type Request = T::Request; - type Response = T::Response; - type Error = T::Error; - type Future = T::Future; - - fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { - self.get_mut().poll_ready(cx) - } - - fn call(&mut self, req: Self::Request) -> Self::Future { - self.get_mut().call(req) - } -} diff --git a/actix-service/src/lib.rs b/actix-service/src/lib.rs index 90d4c79066..68a5dbc68c 100644 --- a/actix-service/src/lib.rs +++ b/actix-service/src/lib.rs @@ -12,7 +12,6 @@ mod and_then_apply_fn; mod apply; mod apply_cfg; pub mod boxed; -mod cell; mod fn_service; mod map; mod map_config; diff --git a/actix-service/src/then.rs b/actix-service/src/then.rs index 1286e742a7..8c7bfa85db 100644 --- a/actix-service/src/then.rs +++ b/actix-service/src/then.rs @@ -1,16 +1,16 @@ use std::future::Future; use std::pin::Pin; use std::rc::Rc; +use std::cell::RefCell; use std::task::{Context, Poll}; use super::{Service, ServiceFactory}; -use crate::cell::Cell; /// Service for the `then` combinator, chaining a computation onto the end of /// another service. /// /// This is created by the `Pipeline::then` method. -pub(crate) struct ThenService(Cell<(A, B)>); +pub(crate) struct ThenService(Rc>); impl ThenService { /// Create new `.then()` combinator @@ -19,7 +19,7 @@ impl ThenService { A: Service, B: Service, Error = A::Error>, { - Self(Cell::new((a, b))) + Self(Rc::new(RefCell::new((a, b)))) } } @@ -40,7 +40,7 @@ where type Future = ThenServiceResponse; fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { - let srv = self.0.get_mut(); + let mut srv = self.0.borrow_mut(); let not_ready = !srv.0.poll_ready(cx)?.is_ready(); if !srv.1.poll_ready(cx)?.is_ready() || not_ready { Poll::Pending @@ -51,7 +51,7 @@ where fn call(&mut self, req: A::Request) -> Self::Future { ThenServiceResponse { - state: State::A(self.0.get_mut().0.call(req), Some(self.0.clone())), + state: State::A(self.0.borrow_mut().0.call(req), Some(self.0.clone())), } } } @@ -72,7 +72,7 @@ where A: Service, B: Service>, { - A(#[pin] A::Future, Option>), + A(#[pin] A::Future, Option>>), B(#[pin] B::Future), Empty, } @@ -90,9 +90,9 @@ where match this.state.as_mut().project() { StateProj::A(fut, b) => match fut.poll(cx) { Poll::Ready(res) => { - let mut b = b.take().unwrap(); + let b = b.take().unwrap(); this.state.set(State::Empty); // drop fut A - let fut = b.get_mut().1.call(res); + let fut = b.borrow_mut().1.call(res); this.state.set(State::B(fut)); self.poll(cx) } From 11a5cb3ae15a9b255a79520d3c552f758c013c78 Mon Sep 17 00:00:00 2001 From: "Sergey \"Shnatsel\" Davidoff" Date: Sun, 19 Jul 2020 21:09:39 +0200 Subject: [PATCH 2/3] Add changelog entry --- actix-service/CHANGES.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/actix-service/CHANGES.md b/actix-service/CHANGES.md index 137e402bd8..e7c1fefa80 100644 --- a/actix-service/CHANGES.md +++ b/actix-service/CHANGES.md @@ -1,5 +1,11 @@ # Changes +## Unreleased + +### Fixed + +* Removed unsound custom Cell implementation that allowed obtaining several mutable references to the same data, which is undefined behavior in Rust and could lead to violations of memory safety. External code could obtain several mutable references to the same data through service combinators. Attempts to acquire several mutable references to the same data will instead result in a panic. + ## [1.0.5] - 2020-01-16 ### Fixed From bc17dc122f5d7e6c7d1e04e6555141c39055c836 Mon Sep 17 00:00:00 2001 From: "Sergey \"Shnatsel\" Davidoff" Date: Sun, 19 Jul 2020 21:52:12 +0200 Subject: [PATCH 3/3] Dummy commit to re-run CI