From 877dc0a52adedd1daaabcfcb56d90d1da1da862f Mon Sep 17 00:00:00 2001 From: Soveu Date: Tue, 4 May 2021 20:59:00 +0200 Subject: [PATCH 1/5] first step of removing mem::uninitialized --- src/proto/h1/role.rs | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/src/proto/h1/role.rs b/src/proto/h1/role.rs index 84dc091147..0751c0f493 100644 --- a/src/proto/h1/role.rs +++ b/src/proto/h1/role.rs @@ -1,16 +1,14 @@ -// `mem::uninitialized` replaced with `mem::MaybeUninit`, -// can't upgrade yet #![allow(deprecated)] use std::fmt::{self, Write}; -use std::mem; +use std::mem::{self, MaybeUninit}; #[cfg(any(test, feature = "server", feature = "ffi"))] use bytes::Bytes; use bytes::BytesMut; -use http::header::{self, Entry, HeaderName, HeaderValue}; #[cfg(feature = "server")] use http::header::ValueIter; +use http::header::{self, Entry, HeaderName, HeaderValue}; use http::{HeaderMap, Method, StatusCode, Version}; use crate::body::DecodedLength; @@ -126,7 +124,10 @@ impl Http1Transaction for Server { // but we *never* read any of it until after httparse has assigned // values into it. By not zeroing out the stack memory, this saves // a good ~5% on pipeline benchmarks. - let mut headers_indices: [HeaderIndices; MAX_HEADERS] = unsafe { mem::uninitialized() }; + let mut headers_indices: [MaybeUninit; MAX_HEADERS] = unsafe { + // SAFETY: We can go safely from MaybeUninit array to array of MaybeUninit + MaybeUninit::uninit().assume_init() + }; { let mut headers: [httparse::Header<'_>; MAX_HEADERS] = unsafe { mem::uninitialized() }; trace!( @@ -205,6 +206,8 @@ impl Http1Transaction for Server { headers.reserve(headers_len); for header in &headers_indices[..headers_len] { + // SAFETY: array is valid up to `headers_len` + let header = unsafe { &*header.as_ptr() }; let name = header_name!(&slice[header.name.0..header.name.1]); let value = header_value!(slice.slice(header.value.0..header.value.1)); @@ -880,7 +883,10 @@ impl Http1Transaction for Client { // Loop to skip information status code headers (100 Continue, etc). loop { // Unsafe: see comment in Server Http1Transaction, above. - let mut headers_indices: [HeaderIndices; MAX_HEADERS] = unsafe { mem::uninitialized() }; + let mut headers_indices: [MaybeUninit; MAX_HEADERS] = unsafe { + // SAFETY: We can go safely from MaybeUninit array to array of MaybeUninit + MaybeUninit::uninit().assume_init() + }; let (len, status, reason, version, headers_len) = { let mut headers: [httparse::Header<'_>; MAX_HEADERS] = unsafe { mem::uninitialized() }; @@ -891,8 +897,7 @@ impl Http1Transaction for Client { ); let mut res = httparse::Response::new(&mut headers); let bytes = buf.as_ref(); - match ctx.h1_parser_config.parse_response(&mut res, bytes) - { + match ctx.h1_parser_config.parse_response(&mut res, bytes) { Ok(httparse::Status::Complete(len)) => { trace!("Response.parse Complete({})", len); let status = StatusCode::from_u16(res.code.unwrap())?; @@ -948,6 +953,8 @@ impl Http1Transaction for Client { headers.reserve(headers_len); for header in &headers_indices[..headers_len] { + // SAFETY: array is valid up to `headers_len` + let header = unsafe { &*header.as_ptr() }; let name = header_name!(&slice[header.name.0..header.name.1]); let value = header_value!(slice.slice(header.value.0..header.value.1)); @@ -1290,7 +1297,7 @@ struct HeaderIndices { fn record_header_indices( bytes: &[u8], headers: &[httparse::Header<'_>], - indices: &mut [HeaderIndices], + indices: &mut [MaybeUninit], ) -> Result<(), crate::error::Parse> { let bytes_ptr = bytes.as_ptr() as usize; @@ -1301,10 +1308,19 @@ fn record_header_indices( } let name_start = header.name.as_ptr() as usize - bytes_ptr; let name_end = name_start + header.name.len(); - indices.name = (name_start, name_end); let value_start = header.value.as_ptr() as usize - bytes_ptr; let value_end = value_start + header.value.len(); - indices.value = (value_start, value_end); + + // FIXME(maybe_uninit_extra) + // FIXME(addr_of) + // Currently we don't have `ptr::addr_of_mut` in stable rust or + // MaybeUninit::write, so this is some way of assigning into a MaybeUninit + // safely + let new_header_indices = HeaderIndices { + name: (name_start, name_end), + value: (value_start, value_end), + }; + *indices = MaybeUninit::new(new_header_indices); } Ok(()) From 7500d7ddfd1c39a5fbfefd54b9014842d9862af4 Mon Sep 17 00:00:00 2001 From: Soveu Date: Tue, 11 May 2021 12:53:59 +0200 Subject: [PATCH 2/5] Server::parse - use MaybeUninit --- src/proto/h1/role.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/proto/h1/role.rs b/src/proto/h1/role.rs index 0751c0f493..59ff94f6e5 100644 --- a/src/proto/h1/role.rs +++ b/src/proto/h1/role.rs @@ -129,15 +129,18 @@ impl Http1Transaction for Server { MaybeUninit::uninit().assume_init() }; { - let mut headers: [httparse::Header<'_>; MAX_HEADERS] = unsafe { mem::uninitialized() }; + /* SAFETY: it is safe to go from MaybeUninit array to array of MaybeUninit */ + let mut headers: [MaybeUninit>; MAX_HEADERS] = unsafe { + MaybeUninit::uninit().assume_init() + }; trace!( "Request.parse([Header; {}], [u8; {}])", headers.len(), buf.len() ); - let mut req = httparse::Request::new(&mut headers); let bytes = buf.as_ref(); - match req.parse(bytes) { + let (req, status) = httparse::Request::with_uninit_headers(&mut headers, bytes); + match status { Ok(httparse::Status::Complete(parsed_len)) => { trace!("Request.parse Complete({})", parsed_len); len = parsed_len; From e57b06f701462547dc5eceb032b0651501ee9743 Mon Sep 17 00:00:00 2001 From: Soveu Date: Tue, 11 May 2021 13:33:54 +0200 Subject: [PATCH 3/5] Client::parse - use MaybeUninit --- src/proto/h1/role.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/proto/h1/role.rs b/src/proto/h1/role.rs index 59ff94f6e5..36f94a20d5 100644 --- a/src/proto/h1/role.rs +++ b/src/proto/h1/role.rs @@ -891,16 +891,17 @@ impl Http1Transaction for Client { MaybeUninit::uninit().assume_init() }; let (len, status, reason, version, headers_len) = { - let mut headers: [httparse::Header<'_>; MAX_HEADERS] = - unsafe { mem::uninitialized() }; + // SAFETY: We can go safely from MaybeUninit array to array of MaybeUninit + let mut headers: [MaybeUninit>; MAX_HEADERS] = + unsafe { MaybeUninit::uninit().assume_init() }; trace!( "Response.parse([Header; {}], [u8; {}])", headers.len(), buf.len() ); - let mut res = httparse::Response::new(&mut headers); let bytes = buf.as_ref(); - match ctx.h1_parser_config.parse_response(&mut res, bytes) { + let (res, status) = ctx.h1_parser_config.parse_response_with_uninit_headers(&mut headers, bytes); + match status { Ok(httparse::Status::Complete(len)) => { trace!("Response.parse Complete({})", len); let status = StatusCode::from_u16(res.code.unwrap())?; From 5cf0b14921d2d21b0317307441cd6e7c36fa0da8 Mon Sep 17 00:00:00 2001 From: Soveu Date: Wed, 12 May 2021 13:28:07 +0200 Subject: [PATCH 4/5] uninit headers - apply changes from second approach --- src/proto/h1/role.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/proto/h1/role.rs b/src/proto/h1/role.rs index 36f94a20d5..0ad3d25e19 100644 --- a/src/proto/h1/role.rs +++ b/src/proto/h1/role.rs @@ -138,9 +138,9 @@ impl Http1Transaction for Server { headers.len(), buf.len() ); + let mut req = httparse::Request::new(&mut []); let bytes = buf.as_ref(); - let (req, status) = httparse::Request::with_uninit_headers(&mut headers, bytes); - match status { + match req.parse_with_uninit_headers(bytes, &mut headers) { Ok(httparse::Status::Complete(parsed_len)) => { trace!("Request.parse Complete({})", parsed_len); len = parsed_len; @@ -899,9 +899,11 @@ impl Http1Transaction for Client { headers.len(), buf.len() ); + let mut res = httparse::Response::new(&mut []); let bytes = buf.as_ref(); - let (res, status) = ctx.h1_parser_config.parse_response_with_uninit_headers(&mut headers, bytes); - match status { + match ctx.h1_parser_config + .parse_response_with_uninit_headers(&mut res, bytes, &mut headers) + { Ok(httparse::Status::Complete(len)) => { trace!("Response.parse Complete({})", len); let status = StatusCode::from_u16(res.code.unwrap())?; From 0063c828c8c56c6f8ab2036412ec8b8de4965979 Mon Sep 17 00:00:00 2001 From: Soveu Date: Mon, 16 Aug 2021 19:41:44 +0200 Subject: [PATCH 5/5] remove allow(deprecated) from h1/role.rs --- src/proto/h1/role.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/proto/h1/role.rs b/src/proto/h1/role.rs index 8ce967366b..318f0c441a 100644 --- a/src/proto/h1/role.rs +++ b/src/proto/h1/role.rs @@ -1,5 +1,3 @@ -#![allow(deprecated)] - use std::fmt::{self, Write}; use std::mem::{self, MaybeUninit};