From 9a9d4b182eff659894f32ddab360e3060c747e3c Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Thu, 3 Sep 2020 10:00:24 +0100 Subject: [PATCH] document all remaining unsafe usages (#1642) adds some debug assertions where appropriate --- actix-http/src/config.rs | 3 +- actix-http/src/h1/decoder.rs | 4 +- actix-http/src/h1/dispatcher.rs | 4 ++ actix-http/src/h1/encoder.rs | 69 +++++++++++++++++++++++++++------ actix-http/src/h2/dispatcher.rs | 8 ++-- actix-http/src/macros.rs | 5 ++- actix-http/src/ws/mask.rs | 15 ++++--- 7 files changed, 84 insertions(+), 24 deletions(-) diff --git a/actix-http/src/config.rs b/actix-http/src/config.rs index abf3d8ff9fc..b314d4c999a 100644 --- a/actix-http/src/config.rs +++ b/actix-http/src/config.rs @@ -17,7 +17,7 @@ const DATE_VALUE_LENGTH: usize = 29; pub enum KeepAlive { /// Keep alive in seconds Timeout(usize), - /// Relay on OS to shutdown tcp connection + /// Rely on OS to shutdown tcp connection Os, /// Disabled Disabled, @@ -209,6 +209,7 @@ impl Date { date.update(); date } + fn update(&mut self) { self.pos = 0; write!( diff --git a/actix-http/src/h1/decoder.rs b/actix-http/src/h1/decoder.rs index 8fdd11be553..8e891dc5ce2 100644 --- a/actix-http/src/h1/decoder.rs +++ b/actix-http/src/h1/decoder.rs @@ -76,12 +76,14 @@ pub(crate) trait MessageType: Sized { let name = HeaderName::from_bytes(&slice[idx.name.0..idx.name.1]).unwrap(); - // SAFETY: httparse checks header value is valid UTF-8 + // SAFETY: httparse already checks header value is only visible ASCII bytes + // from_maybe_shared_unchecked contains debug assertions so they are omitted here let value = unsafe { HeaderValue::from_maybe_shared_unchecked( slice.slice(idx.value.0..idx.value.1), ) }; + match name { header::CONTENT_LENGTH => { if let Ok(s) = value.to_str() { diff --git a/actix-http/src/h1/dispatcher.rs b/actix-http/src/h1/dispatcher.rs index 00b36562eaa..7c4de970744 100644 --- a/actix-http/src/h1/dispatcher.rs +++ b/actix-http/src/h1/dispatcher.rs @@ -314,11 +314,15 @@ where Poll::Ready(Err(err)) => return Err(DispatchError::Io(err)), } } + if written == write_buf.len() { + // SAFETY: setting length to 0 is safe + // skips one length check vs truncate unsafe { write_buf.set_len(0) } } else { write_buf.advance(written); } + Ok(false) } diff --git a/actix-http/src/h1/encoder.rs b/actix-http/src/h1/encoder.rs index eb8c337dd12..e16b4c3dc57 100644 --- a/actix-http/src/h1/encoder.rs +++ b/actix-http/src/h1/encoder.rs @@ -129,89 +129,133 @@ pub(crate) trait MessageType: Sized { .chain(extra_headers.inner.iter()); // write headers - let mut pos = 0; + let mut has_date = false; - let mut remaining = dst.capacity() - dst.len(); + let mut buf = dst.bytes_mut().as_mut_ptr() as *mut u8; + let mut remaining = dst.capacity() - dst.len(); + + // tracks bytes written since last buffer resize + // since buf is a raw pointer to a bytes container storage but is written to without the + // container's knowledge, this is used to sync the containers cursor after data is written + let mut pos = 0; + for (key, value) in headers { match *key { CONNECTION => continue, TRANSFER_ENCODING | CONTENT_LENGTH if skip_len => continue, - DATE => { - has_date = true; - } + DATE => has_date = true, _ => (), } + let k = key.as_str().as_bytes(); + let k_len = k.len(); + match value { map::Value::One(ref val) => { let v = val.as_ref(); let v_len = v.len(); - let k_len = k.len(); + + // key length + value length + colon + space + \r\n let len = k_len + v_len + 4; + if len > remaining { + // not enough room in buffer for this header; reserve more space + + // SAFETY: all the bytes written up to position "pos" are initialized + // the written byte count and pointer advancement are kept in sync unsafe { dst.advance_mut(pos); } + pos = 0; dst.reserve(len * 2); remaining = dst.capacity() - dst.len(); + + // re-assign buf raw pointer since it's possible that the buffer was + // reallocated and/or resized buf = dst.bytes_mut().as_mut_ptr() as *mut u8; } - // use upper Camel-Case + + // SAFETY: on each write, it is enough to ensure that the advancement of the + // cursor matches the number of bytes written unsafe { + // use upper Camel-Case if camel_case { write_camel_case(k, from_raw_parts_mut(buf, k_len)) } else { write_data(k, buf, k_len) } + buf = buf.add(k_len); + write_data(b": ", buf, 2); buf = buf.add(2); + write_data(v, buf, v_len); buf = buf.add(v_len); + write_data(b"\r\n", buf, 2); buf = buf.add(2); - pos += len; - remaining -= len; } + + pos += len; + remaining -= len; } + map::Value::Multi(ref vec) => { for val in vec { let v = val.as_ref(); let v_len = v.len(); - let k_len = k.len(); let len = k_len + v_len + 4; + if len > remaining { + // SAFETY: all the bytes written up to position "pos" are initialized + // the written byte count and pointer advancement are kept in sync unsafe { dst.advance_mut(pos); } pos = 0; dst.reserve(len * 2); remaining = dst.capacity() - dst.len(); + + // re-assign buf raw pointer since it's possible that the buffer was + // reallocated and/or resized buf = dst.bytes_mut().as_mut_ptr() as *mut u8; } - // use upper Camel-Case + + // SAFETY: on each write, it is enough to ensure that the advancement of + // the cursor matches the number of bytes written unsafe { if camel_case { write_camel_case(k, from_raw_parts_mut(buf, k_len)); } else { write_data(k, buf, k_len); } + buf = buf.add(k_len); + write_data(b": ", buf, 2); buf = buf.add(2); + write_data(v, buf, v_len); buf = buf.add(v_len); + write_data(b"\r\n", buf, 2); buf = buf.add(2); }; + pos += len; remaining -= len; } } } } + + // final cursor synchronization with the bytes container + // + // SAFETY: all the bytes written up to position "pos" are initialized + // the written byte count and pointer advancement are kept in sync unsafe { dst.advance_mut(pos); } @@ -477,7 +521,10 @@ impl<'a> io::Write for Writer<'a> { } } +/// # Safety +/// Callers must ensure that the given length matches given value length. unsafe fn write_data(value: &[u8], buf: *mut u8, len: usize) { + debug_assert_eq!(value.len(), len); copy_nonoverlapping(value.as_ptr(), buf, len); } diff --git a/actix-http/src/h2/dispatcher.rs b/actix-http/src/h2/dispatcher.rs index 534ce928a16..daa651f4d9e 100644 --- a/actix-http/src/h2/dispatcher.rs +++ b/actix-http/src/h2/dispatcher.rs @@ -227,9 +227,11 @@ where if !has_date { let mut bytes = BytesMut::with_capacity(29); self.config.set_date_header(&mut bytes); - res.headers_mut().insert(DATE, unsafe { - HeaderValue::from_maybe_shared_unchecked(bytes.freeze()) - }); + res.headers_mut().insert( + DATE, + // SAFETY: serialized date-times are known ASCII strings + unsafe { HeaderValue::from_maybe_shared_unchecked(bytes.freeze()) }, + ); } res diff --git a/actix-http/src/macros.rs b/actix-http/src/macros.rs index b970b14f222..e08d62ba64a 100644 --- a/actix-http/src/macros.rs +++ b/actix-http/src/macros.rs @@ -38,7 +38,7 @@ macro_rules! downcast { /// Downcasts generic body to a specific type. pub fn downcast_ref(&self) -> Option<&T> { if self.__private_get_type_id__().0 == std::any::TypeId::of::() { - // Safety: external crates cannot override the default + // SAFETY: external crates cannot override the default // implementation of `__private_get_type_id__`, since // it requires returning a private type. We can therefore // rely on the returned `TypeId`, which ensures that this @@ -48,10 +48,11 @@ macro_rules! downcast { None } } + /// Downcasts a generic body to a mutable specific type. pub fn downcast_mut(&mut self) -> Option<&mut T> { if self.__private_get_type_id__().0 == std::any::TypeId::of::() { - // Safety: external crates cannot override the default + // SAFETY: external crates cannot override the default // implementation of `__private_get_type_id__`, since // it requires returning a private type. We can therefore // rely on the returned `TypeId`, which ensures that this diff --git a/actix-http/src/ws/mask.rs b/actix-http/src/ws/mask.rs index 367fb021244..726b1a4a12f 100644 --- a/actix-http/src/ws/mask.rs +++ b/actix-http/src/ws/mask.rs @@ -7,6 +7,8 @@ use std::slice; struct ShortSlice<'a>(&'a mut [u8]); impl<'a> ShortSlice<'a> { + /// # Safety + /// Given slice must be shorter than 8 bytes. unsafe fn new(slice: &'a mut [u8]) -> Self { // Sanity check for debug builds debug_assert!(slice.len() < 8); @@ -46,13 +48,13 @@ pub(crate) fn apply_mask(buf: &mut [u8], mask_u32: u32) { } } -#[inline] // TODO: copy_nonoverlapping here compiles to call memcpy. While it is not so // inefficient, it could be done better. The compiler does not understand that // a `ShortSlice` must be smaller than a u64. +#[inline] #[allow(clippy::needless_pass_by_value)] fn xor_short(buf: ShortSlice<'_>, mask: u64) { - // Unsafe: we know that a `ShortSlice` fits in a u64 + // SAFETY: we know that a `ShortSlice` fits in a u64 unsafe { let (ptr, len) = (buf.0.as_mut_ptr(), buf.0.len()); let mut b: u64 = 0; @@ -64,8 +66,9 @@ fn xor_short(buf: ShortSlice<'_>, mask: u64) { } } +/// # Safety +/// Caller must ensure the buffer has the correct size and alignment. #[inline] -// Unsafe: caller must ensure the buffer has the correct size and alignment unsafe fn cast_slice(buf: &mut [u8]) -> &mut [u64] { // Assert correct size and alignment in debug builds debug_assert!(buf.len().trailing_zeros() >= 3); @@ -74,9 +77,9 @@ unsafe fn cast_slice(buf: &mut [u8]) -> &mut [u64] { slice::from_raw_parts_mut(buf.as_mut_ptr() as *mut u64, buf.len() >> 3) } -#[inline] // Splits a slice into three parts: an unaligned short head and tail, plus an aligned // u64 mid section. +#[inline] fn align_buf(buf: &mut [u8]) -> (ShortSlice<'_>, &mut [u64], ShortSlice<'_>) { let start_ptr = buf.as_ptr() as usize; let end_ptr = start_ptr + buf.len(); @@ -91,13 +94,13 @@ fn align_buf(buf: &mut [u8]) -> (ShortSlice<'_>, &mut [u64], ShortSlice<'_>) { let (tmp, tail) = buf.split_at_mut(end_aligned - start_ptr); let (head, mid) = tmp.split_at_mut(start_aligned - start_ptr); - // Unsafe: we know the middle section is correctly aligned, and the outer + // SAFETY: we know the middle section is correctly aligned, and the outer // sections are smaller than 8 bytes unsafe { (ShortSlice::new(head), cast_slice(mid), ShortSlice(tail)) } } else { // We didn't cross even one aligned boundary! - // Unsafe: The outer sections are smaller than 8 bytes + // SAFETY: The outer sections are smaller than 8 bytes unsafe { (ShortSlice::new(buf), &mut [], ShortSlice::new(&mut [])) } } }