diff --git a/actix-http/CHANGES.md b/actix-http/CHANGES.md
index a185a9f8..f7923916 100644
--- a/actix-http/CHANGES.md
+++ b/actix-http/CHANGES.md
@@ -1,6 +1,11 @@
# Changes
## [Unreleased] - xxx
+### Fixed
+* Potential UB in h1 decoder using uninitialized memory. [#1614]
+
+[#1614]: https://github.com/actix/actix-web/pull/1614
+
## [2.0.0-beta.1] - 2020-07-11
diff --git a/actix-http/Cargo.toml b/actix-http/Cargo.toml
index 232b5c3f..bbb2a214 100644
--- a/actix-http/Cargo.toml
+++ b/actix-http/Cargo.toml
@@ -103,3 +103,7 @@ harness = false
[[bench]]
name = "status-line"
harness = false
+
+[[bench]]
+name = "uninit-headers"
+harness = false
diff --git a/actix-http/benches/uninit-headers.rs b/actix-http/benches/uninit-headers.rs
new file mode 100644
index 00000000..83e74171
--- /dev/null
+++ b/actix-http/benches/uninit-headers.rs
@@ -0,0 +1,137 @@
+use criterion::{criterion_group, criterion_main, Criterion};
+
+use bytes::BytesMut;
+
+// A Miri run detects UB, seen on this playground:
+// https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f5d9aa166aa48df8dca05fce2b6c3915
+
+fn bench_header_parsing(c: &mut Criterion) {
+ c.bench_function("Original (Unsound) [short]", |b| {
+ b.iter(|| {
+ let mut buf = BytesMut::from(REQ_SHORT);
+ _original::parse_headers(&mut buf);
+ })
+ });
+
+ c.bench_function("New (safe) [short]", |b| {
+ b.iter(|| {
+ let mut buf = BytesMut::from(REQ_SHORT);
+ _new::parse_headers(&mut buf);
+ })
+ });
+
+ c.bench_function("Original (Unsound) [realistic]", |b| {
+ b.iter(|| {
+ let mut buf = BytesMut::from(REQ);
+ _original::parse_headers(&mut buf);
+ })
+ });
+
+ c.bench_function("New (safe) [realistic]", |b| {
+ b.iter(|| {
+ let mut buf = BytesMut::from(REQ);
+ _new::parse_headers(&mut buf);
+ })
+ });
+}
+
+criterion_group!(benches, bench_header_parsing);
+criterion_main!(benches);
+
+const MAX_HEADERS: usize = 96;
+
+const EMPTY_HEADER_ARRAY: [httparse::Header<'static>; MAX_HEADERS] =
+ [httparse::EMPTY_HEADER; MAX_HEADERS];
+
+#[derive(Clone, Copy)]
+struct HeaderIndex {
+ name: (usize, usize),
+ value: (usize, usize),
+}
+
+const EMPTY_HEADER_INDEX: HeaderIndex = HeaderIndex {
+ name: (0, 0),
+ value: (0, 0),
+};
+
+const EMPTY_HEADER_INDEX_ARRAY: [HeaderIndex; MAX_HEADERS] =
+ [EMPTY_HEADER_INDEX; MAX_HEADERS];
+
+impl HeaderIndex {
+ fn record(
+ bytes: &[u8],
+ headers: &[httparse::Header<'_>],
+ indices: &mut [HeaderIndex],
+ ) {
+ let bytes_ptr = bytes.as_ptr() as usize;
+ for (header, indices) in headers.iter().zip(indices.iter_mut()) {
+ 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);
+ }
+ }
+}
+
+// test cases taken from:
+// https://github.com/seanmonstar/httparse/blob/master/benches/parse.rs
+
+const REQ_SHORT: &'static [u8] = b"\
+GET / HTTP/1.0\r\n\
+Host: example.com\r\n\
+Cookie: session=60; user_id=1\r\n\r\n";
+
+const REQ: &'static [u8] = b"\
+GET /wp-content/uploads/2010/03/hello-kitty-darth-vader-pink.jpg HTTP/1.1\r\n\
+Host: www.kittyhell.com\r\n\
+User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; ja-JP-mac; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 Pathtraq/0.9\r\n\
+Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8\r\n\
+Accept-Language: ja,en-us;q=0.7,en;q=0.3\r\n\
+Accept-Encoding: gzip,deflate\r\n\
+Accept-Charset: Shift_JIS,utf-8;q=0.7,*;q=0.7\r\n\
+Keep-Alive: 115\r\n\
+Connection: keep-alive\r\n\
+Cookie: wp_ozh_wsa_visits=2; wp_ozh_wsa_visit_lasttime=xxxxxxxxxx; __utma=xxxxxxxxx.xxxxxxxxxx.xxxxxxxxxx.xxxxxxxxxx.xxxxxxxxxx.x; __utmz=xxxxxxxxx.xxxxxxxxxx.x.x.utmccn=(referral)|utmcsr=reader.livedoor.com|utmcct=/reader/|utmcmd=referral|padding=under256\r\n\r\n";
+
+mod _new {
+ use super::*;
+
+ pub fn parse_headers(src: &mut BytesMut) -> usize {
+ let mut headers: [HeaderIndex; MAX_HEADERS] = EMPTY_HEADER_INDEX_ARRAY;
+ let mut parsed: [httparse::Header<'_>; MAX_HEADERS] = EMPTY_HEADER_ARRAY;
+
+ let mut req = httparse::Request::new(&mut parsed);
+ match req.parse(src).unwrap() {
+ httparse::Status::Complete(_len) => {
+ HeaderIndex::record(src, req.headers, &mut headers);
+ req.headers.len()
+ }
+ _ => unreachable!(),
+ }
+ }
+}
+
+mod _original {
+ use super::*;
+
+ use std::mem::MaybeUninit;
+
+ pub fn parse_headers(src: &mut BytesMut) -> usize {
+ let mut headers: [HeaderIndex; MAX_HEADERS] =
+ unsafe { MaybeUninit::uninit().assume_init() };
+
+ let mut parsed: [httparse::Header<'_>; MAX_HEADERS] =
+ unsafe { MaybeUninit::uninit().assume_init() };
+
+ let mut req = httparse::Request::new(&mut parsed);
+ match req.parse(src).unwrap() {
+ httparse::Status::Complete(_len) => {
+ HeaderIndex::record(src, req.headers, &mut headers);
+ req.headers.len()
+ }
+ _ => unreachable!(),
+ }
+ }
+}
diff --git a/actix-http/src/h1/decoder.rs b/actix-http/src/h1/decoder.rs
index 89a18aea..a6c52d35 100644
--- a/actix-http/src/h1/decoder.rs
+++ b/actix-http/src/h1/decoder.rs
@@ -1,7 +1,6 @@
use std::convert::TryFrom;
use std::io;
use std::marker::PhantomData;
-use std::mem::MaybeUninit;
use std::task::Poll;
use actix_codec::Decoder;
@@ -77,7 +76,7 @@ pub(crate) trait MessageType: Sized {
let name =
HeaderName::from_bytes(&slice[idx.name.0..idx.name.1]).unwrap();
- // Unsafe: httparse check header value for valid utf-8
+ // SAFETY: httparse checks header value is valid UTF-8
let value = unsafe {
HeaderValue::from_maybe_shared_unchecked(
slice.slice(idx.value.0..idx.value.1),
@@ -184,16 +183,11 @@ impl MessageType for Request {
&mut self.head_mut().headers
}
- #[allow(clippy::uninit_assumed_init)]
fn decode(src: &mut BytesMut) -> Result<Option<(Self, PayloadType)>, ParseError> {
- // Unsafe: we read only this data only after httparse parses headers into.
- // performance bump for pipeline benchmarks.
- let mut headers: [HeaderIndex; MAX_HEADERS] =
- unsafe { MaybeUninit::uninit().assume_init() };
+ let mut headers: [HeaderIndex; MAX_HEADERS] = EMPTY_HEADER_INDEX_ARRAY;
let (len, method, uri, ver, h_len) = {
- let mut parsed: [httparse::Header<'_>; MAX_HEADERS] =
- unsafe { MaybeUninit::uninit().assume_init() };
+ let mut parsed: [httparse::Header<'_>; MAX_HEADERS] = EMPTY_HEADER_ARRAY;
let mut req = httparse::Request::new(&mut parsed);
match req.parse(src)? {
@@ -260,16 +254,11 @@ impl MessageType for ResponseHead {
&mut self.headers
}
- #[allow(clippy::uninit_assumed_init)]
fn decode(src: &mut BytesMut) -> Result<Option<(Self, PayloadType)>, ParseError> {
- // Unsafe: we read only this data only after httparse parses headers into.
- // performance bump for pipeline benchmarks.
- let mut headers: [HeaderIndex; MAX_HEADERS] =
- unsafe { MaybeUninit::uninit().assume_init() };
+ let mut headers: [HeaderIndex; MAX_HEADERS] = EMPTY_HEADER_INDEX_ARRAY;
let (len, ver, status, h_len) = {
- let mut parsed: [httparse::Header<'_>; MAX_HEADERS] =
- unsafe { MaybeUninit::uninit().assume_init() };
+ let mut parsed: [httparse::Header<'_>; MAX_HEADERS] = EMPTY_HEADER_ARRAY;
let mut res = httparse::Response::new(&mut parsed);
match res.parse(src)? {
@@ -324,6 +313,17 @@ pub(crate) struct HeaderIndex {
pub(crate) value: (usize, usize),
}
+pub(crate) const EMPTY_HEADER_INDEX: HeaderIndex = HeaderIndex {
+ name: (0, 0),
+ value: (0, 0),
+};
+
+pub(crate) const EMPTY_HEADER_INDEX_ARRAY: [HeaderIndex; MAX_HEADERS] =
+ [EMPTY_HEADER_INDEX; MAX_HEADERS];
+
+pub(crate) const EMPTY_HEADER_ARRAY: [httparse::Header<'static>; MAX_HEADERS] =
+ [httparse::EMPTY_HEADER; MAX_HEADERS];
+
impl HeaderIndex {
pub(crate) fn record(
bytes: &[u8],
@@ -973,7 +973,7 @@ mod tests {
unreachable!("Error");
}
- // type in chunked
+ // intentional typo in "chunked"
let mut buf = BytesMut::from(
"GET /test HTTP/1.1\r\n\
transfer-encoding: chnked\r\n\r\n",