From b36fdc46db7b606615c417d538059d30427b82fd Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 17 Jul 2019 18:45:17 -0400 Subject: [PATCH] Remove several usages of 'unsafe' (#968) * Replace UnsafeCell in DateServiceInner with Cell The previous API was extremely dangerous - calling `get_ref()` followed by `reset()` would trigger instant UB, without requiring any `unsafe` blocks in the caller. By making DateInner `Copy`, we can use a normal `Cell` instead of an `UnsafeCell`. This makes it impossible to cause UB (or even panic) with the API. * Split unsafe block HttpServiceHandlerResponse Also add explanation of the safety of the usage of `unsafe` * Replace UnsafeCell with RefCell in PayloadRef This ensures that a mistake in the usage of 'get_mut' will cause a panic, not undefined behavior. --- actix-http/src/config.rs | 24 ++++++++++++------------ actix-http/src/service.rs | 22 ++++++++++++---------- actix-multipart/src/server.rs | 29 +++++++++++++---------------- 3 files changed, 37 insertions(+), 38 deletions(-) diff --git a/actix-http/src/config.rs b/actix-http/src/config.rs index aba50a81..7d0e2763 100644 --- a/actix-http/src/config.rs +++ b/actix-http/src/config.rs @@ -1,4 +1,4 @@ -use std::cell::UnsafeCell; +use std::cell::Cell; use std::fmt; use std::fmt::Write; use std::rc::Rc; @@ -172,6 +172,7 @@ impl ServiceConfig { } } +#[derive(Copy, Clone)] struct Date { bytes: [u8; DATE_VALUE_LENGTH], pos: usize, @@ -205,28 +206,28 @@ impl fmt::Write for Date { struct DateService(Rc); struct DateServiceInner { - current: UnsafeCell>, + current: Cell>, } impl DateServiceInner { fn new() -> Self { DateServiceInner { - current: UnsafeCell::new(None), + current: Cell::new(None), } } - fn get_ref(&self) -> &Option<(Date, Instant)> { - unsafe { &*self.current.get() } + fn get(&self) -> Option<(Date, Instant)> { + self.current.get() } fn reset(&self) { - unsafe { (&mut *self.current.get()).take() }; + self.current.set(None); } fn update(&self) { let now = Instant::now(); let date = Date::new(); - *(unsafe { &mut *self.current.get() }) = Some((date, now)); + self.current.set(Some((date, now))); } } @@ -236,7 +237,7 @@ impl DateService { } fn check_date(&self) { - if self.0.get_ref().is_none() { + if self.0.get().is_none() { self.0.update(); // periodic date update @@ -252,14 +253,13 @@ impl DateService { fn now(&self) -> Instant { self.check_date(); - self.0.get_ref().as_ref().unwrap().1 + self.0.get().unwrap().1 } - fn date(&self) -> &Date { + fn date(&self) -> Date { self.check_date(); - let item = self.0.get_ref().as_ref().unwrap(); - &item.0 + self.0.get().unwrap().0 } } diff --git a/actix-http/src/service.rs b/actix-http/src/service.rs index be021b25..09b8077b 100644 --- a/actix-http/src/service.rs +++ b/actix-http/src/service.rs @@ -466,16 +466,18 @@ where State::Unknown(ref mut data) => { if let Some(ref mut item) = data { loop { - unsafe { - let b = item.1.bytes_mut(); - let n = try_ready!(item.0.poll_read(b)); - if n == 0 { - return Ok(Async::Ready(())); - } - item.1.advance_mut(n); - if item.1.len() >= HTTP2_PREFACE.len() { - break; - } + // Safety - we only write to the returned slice. + let b = unsafe { item.1.bytes_mut() }; + let n = try_ready!(item.0.poll_read(b)); + if n == 0 { + return Ok(Async::Ready(())); + } + // Safety - we know that 'n' bytes have + // been initialized via the contract of + // 'poll_read' + unsafe { item.1.advance_mut(n) }; + if item.1.len() >= HTTP2_PREFACE.len() { + break; } } } else { diff --git a/actix-multipart/src/server.rs b/actix-multipart/src/server.rs index c28a8d6a..70eb61cb 100644 --- a/actix-multipart/src/server.rs +++ b/actix-multipart/src/server.rs @@ -1,5 +1,5 @@ //! Multipart payload support -use std::cell::{Cell, RefCell, UnsafeCell}; +use std::cell::{Cell, RefCell, RefMut}; use std::marker::PhantomData; use std::rc::Rc; use std::{cmp, fmt}; @@ -112,7 +112,7 @@ impl Stream for Multipart { Err(err) } else if self.safety.current() { let mut inner = self.inner.as_mut().unwrap().borrow_mut(); - if let Some(payload) = inner.payload.get_mut(&self.safety) { + if let Some(mut payload) = inner.payload.get_mut(&self.safety) { payload.poll_stream()?; } inner.poll(&self.safety) @@ -265,12 +265,12 @@ impl InnerMultipart { } } - let headers = if let Some(payload) = self.payload.get_mut(safety) { + let headers = if let Some(mut payload) = self.payload.get_mut(safety) { match self.state { // read until first boundary InnerState::FirstBoundary => { match InnerMultipart::skip_until_boundary( - payload, + &mut *payload, &self.boundary, )? { Some(eof) => { @@ -286,7 +286,7 @@ impl InnerMultipart { } // read boundary InnerState::Boundary => { - match InnerMultipart::read_boundary(payload, &self.boundary)? { + match InnerMultipart::read_boundary(&mut *payload, &self.boundary)? { None => return Ok(Async::NotReady), Some(eof) => { if eof { @@ -303,7 +303,7 @@ impl InnerMultipart { // read field headers for next field if self.state == InnerState::Headers { - if let Some(headers) = InnerMultipart::read_headers(payload)? { + if let Some(headers) = InnerMultipart::read_headers(&mut *payload)? { self.state = InnerState::Boundary; headers } else { @@ -411,7 +411,7 @@ impl Stream for Field { fn poll(&mut self) -> Poll, Self::Error> { if self.safety.current() { let mut inner = self.inner.borrow_mut(); - if let Some(payload) = inner.payload.as_ref().unwrap().get_mut(&self.safety) + if let Some(mut payload) = inner.payload.as_ref().unwrap().get_mut(&self.safety) { payload.poll_stream()?; } @@ -582,12 +582,12 @@ impl InnerField { return Ok(Async::Ready(None)); } - let result = if let Some(payload) = self.payload.as_ref().unwrap().get_mut(s) { + let result = if let Some(mut payload) = self.payload.as_ref().unwrap().get_mut(s) { if !self.eof { let res = if let Some(ref mut len) = self.length { - InnerField::read_len(payload, len)? + InnerField::read_len(&mut *payload, len)? } else { - InnerField::read_stream(payload, &self.boundary)? + InnerField::read_stream(&mut *payload, &self.boundary)? }; match res { @@ -618,7 +618,7 @@ impl InnerField { } struct PayloadRef { - payload: Rc>, + payload: Rc>, } impl PayloadRef { @@ -628,15 +628,12 @@ impl PayloadRef { } } - fn get_mut<'a, 'b>(&'a self, s: &'b Safety) -> Option<&'a mut PayloadBuffer> + fn get_mut<'a, 'b>(&'a self, s: &'b Safety) -> Option> where 'a: 'b, { - // Unsafe: Invariant is inforced by Safety Safety is used as ref counter, - // only top most ref can have mutable access to payload. if s.current() { - let payload: &mut PayloadBuffer = unsafe { &mut *self.payload.get() }; - Some(payload) + Some(self.payload.borrow_mut()) } else { None }