From 7f0e3129eff1ae43666674fe223ab95ef5b4ffbd Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Fri, 26 Mar 2021 12:25:10 +0100 Subject: [PATCH] feat(http1): decouple preserving header case from FFI (fixes #2313) --- src/client/client.rs | 15 +++++++++++- src/client/conn.rs | 10 ++++++++ src/ffi/client.rs | 5 +++- src/ffi/http_types.rs | 24 +------------------ src/ffi/mod.rs | 2 +- src/lib.rs | 1 + src/proto/h1/conn.rs | 18 ++++---------- src/proto/h1/io.rs | 2 -- src/proto/h1/mod.rs | 1 - src/proto/h1/role.rs | 56 ++++++++++--------------------------------- 10 files changed, 48 insertions(+), 86 deletions(-) diff --git a/src/client/client.rs b/src/client/client.rs index 418f3fb4e9..89c698e963 100644 --- a/src/client/client.rs +++ b/src/client/client.rs @@ -964,7 +964,8 @@ impl Builder { /// Set whether HTTP/1 connections will write header names as title case at /// the socket level. /// - /// Note that this setting does not affect HTTP/2. + /// Note that this setting does not affect HTTP/2 and supersedes the + /// `http1_preserve_header_case` setting. /// /// Default is false. pub fn http1_title_case_headers(&mut self, val: bool) -> &mut Self { @@ -972,6 +973,18 @@ impl Builder { self } + /// Set whether HTTP/1 connections will write header names as provided + /// at the socket level. + /// + /// Note that this setting does not affect HTTP/2 and is superseded + /// by the `http1_title_case_headers` setting. + /// + /// Default is false. + pub fn http1_preserve_header_case(&mut self, val: bool) -> &mut Self { + self.conn_builder.h1_preserve_header_case(val); + self + } + /// Set whether HTTP/0.9 responses should be tolerated. /// /// Default is false. diff --git a/src/client/conn.rs b/src/client/conn.rs index b87600d85a..029e958731 100644 --- a/src/client/conn.rs +++ b/src/client/conn.rs @@ -124,6 +124,7 @@ pub struct Builder { pub(super) exec: Exec, h09_responses: bool, h1_title_case_headers: bool, + h1_preserve_header_case: bool, h1_read_buf_exact_size: Option, h1_max_buf_size: Option, #[cfg(feature = "http2")] @@ -497,6 +498,7 @@ impl Builder { h09_responses: false, h1_read_buf_exact_size: None, h1_title_case_headers: false, + h1_preserve_header_case: false, h1_max_buf_size: None, #[cfg(feature = "http2")] h2_builder: Default::default(), @@ -526,6 +528,11 @@ impl Builder { self } + pub(crate) fn h1_preserve_header_case(&mut self, enabled: bool) -> &mut Builder { + self.h1_preserve_header_case = enabled; + self + } + pub(super) fn h1_read_buf_exact_size(&mut self, sz: Option) -> &mut Builder { self.h1_read_buf_exact_size = sz; self.h1_max_buf_size = None; @@ -707,6 +714,9 @@ impl Builder { if opts.h1_title_case_headers { conn.set_title_case_headers(); } + if opts.h1_preserve_header_case { + conn.set_preserve_header_case(); + } if opts.h09_responses { conn.set_h09_responses(); } diff --git a/src/ffi/client.rs b/src/ffi/client.rs index 0351214e09..9be4f5a04d 100644 --- a/src/ffi/client.rs +++ b/src/ffi/client.rs @@ -106,8 +106,11 @@ unsafe impl AsTaskType for hyper_clientconn { ffi_fn! { /// Creates a new set of HTTP clientconn options to be used in a handshake. fn hyper_clientconn_options_new() -> *mut hyper_clientconn_options { + let mut builder = conn::Builder::new(); + builder.h1_preserve_header_case(true); + Box::into_raw(Box::new(hyper_clientconn_options { - builder: conn::Builder::new(), + builder, exec: WeakExec::new(), })) } ?= std::ptr::null_mut() diff --git a/src/ffi/http_types.rs b/src/ffi/http_types.rs index 1fce28902a..07289c00b7 100644 --- a/src/ffi/http_types.rs +++ b/src/ffi/http_types.rs @@ -7,6 +7,7 @@ use super::error::hyper_code; use super::task::{hyper_task_return_type, AsTaskType}; use super::HYPER_ITER_CONTINUE; use crate::header::{HeaderName, HeaderValue}; +use crate::proto::h1::HeaderCaseMap; use crate::{Body, HeaderMap, Method, Request, Response, Uri}; /// An HTTP request. @@ -24,10 +25,6 @@ pub struct hyper_headers { orig_casing: HeaderCaseMap, } -// Will probably be moved to `hyper::ext::http1` -#[derive(Debug, Default)] -pub(crate) struct HeaderCaseMap(HeaderMap); - #[derive(Debug)] pub(crate) struct ReasonPhrase(pub(crate) Bytes); @@ -370,25 +367,6 @@ unsafe fn raw_name_value( Ok((name, value, orig_name)) } -// ===== impl HeaderCaseMap ===== - -impl HeaderCaseMap { - pub(crate) fn get_all(&self, name: &HeaderName) -> http::header::GetAll<'_, Bytes> { - self.0.get_all(name) - } - - pub(crate) fn insert(&mut self, name: HeaderName, orig: Bytes) { - self.0.insert(name, orig); - } - - pub(crate) fn append(&mut self, name: N, orig: Bytes) - where - N: http::header::IntoHeaderName, - { - self.0.append(name, orig); - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/src/ffi/mod.rs b/src/ffi/mod.rs index 3d2c7a8df4..2a3a133a82 100644 --- a/src/ffi/mod.rs +++ b/src/ffi/mod.rs @@ -62,7 +62,7 @@ pub use self::io::*; pub use self::task::*; pub(crate) use self::body::UserBody; -pub(crate) use self::http_types::{HeaderCaseMap, ReasonPhrase}; +pub(crate) use self::http_types::ReasonPhrase; /// Return in iter functions to continue iterating. pub const HYPER_ITER_CONTINUE: libc::c_int = 0; diff --git a/src/lib.rs b/src/lib.rs index 059f8821c6..132a054eff 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -80,6 +80,7 @@ mod cfg; mod common; pub mod body; mod error; +pub mod ext; #[cfg(test)] mod mock; #[cfg(any(feature = "http1", feature = "http2",))] diff --git a/src/proto/h1/conn.rs b/src/proto/h1/conn.rs index ce0848ddea..aea43e6f7a 100644 --- a/src/proto/h1/conn.rs +++ b/src/proto/h1/conn.rs @@ -44,7 +44,6 @@ where error: None, keep_alive: KA::Busy, method: None, - #[cfg(feature = "ffi")] preserve_header_case: false, title_case_headers: false, h09_responses: false, @@ -79,6 +78,11 @@ where self.state.title_case_headers = true; } + #[cfg(feature = "client")] + pub(crate) fn set_preserve_header_case(&mut self) { + self.state.preserve_header_case = true; + } + #[cfg(feature = "client")] pub(crate) fn set_h09_responses(&mut self) { self.state.h09_responses = true; @@ -150,7 +154,6 @@ where ParseContext { cached_headers: &mut self.state.cached_headers, req_method: &mut self.state.method, - #[cfg(feature = "ffi")] preserve_header_case: self.state.preserve_header_case, h09_responses: self.state.h09_responses, } @@ -488,16 +491,6 @@ where self.enforce_version(&mut head); - // Maybe check if we should preserve header casing on received - // message headers... - #[cfg(feature = "ffi")] - { - if T::is_client() && !self.state.preserve_header_case { - self.state.preserve_header_case = - head.extensions.get::().is_some(); - } - } - let buf = self.io.headers_buf(); match super::role::encode_headers::( Encode { @@ -760,7 +753,6 @@ struct State { /// This is used to know things such as if the message can include /// a body or not. method: Option, - #[cfg(feature = "ffi")] preserve_header_case: bool, title_case_headers: bool, h09_responses: bool, diff --git a/src/proto/h1/io.rs b/src/proto/h1/io.rs index c7ce48664b..e35d3390b9 100644 --- a/src/proto/h1/io.rs +++ b/src/proto/h1/io.rs @@ -159,7 +159,6 @@ where ParseContext { cached_headers: parse_ctx.cached_headers, req_method: parse_ctx.req_method, - #[cfg(feature = "ffi")] preserve_header_case: parse_ctx.preserve_header_case, h09_responses: parse_ctx.h09_responses, }, @@ -639,7 +638,6 @@ mod tests { let parse_ctx = ParseContext { cached_headers: &mut None, req_method: &mut None, - #[cfg(feature = "ffi")] preserve_header_case: false, h09_responses: false, }; diff --git a/src/proto/h1/mod.rs b/src/proto/h1/mod.rs index 01a9253fa3..ec9691a216 100644 --- a/src/proto/h1/mod.rs +++ b/src/proto/h1/mod.rs @@ -70,7 +70,6 @@ pub(crate) struct ParsedMessage { pub(crate) struct ParseContext<'a> { cached_headers: &'a mut Option, req_method: &'a mut Option, - #[cfg(feature = "ffi")] preserve_header_case: bool, h09_responses: bool, } diff --git a/src/proto/h1/role.rs b/src/proto/h1/role.rs index ea9dc96be1..ccc81b09aa 100644 --- a/src/proto/h1/role.rs +++ b/src/proto/h1/role.rs @@ -5,7 +5,7 @@ use std::fmt::{self, Write}; use std::mem; -#[cfg(feature = "ffi")] +#[cfg(any(test, feature = "ffi"))] use bytes::Bytes; use bytes::BytesMut; use http::header::{self, Entry, HeaderName, HeaderValue}; @@ -15,6 +15,7 @@ use crate::body::DecodedLength; #[cfg(feature = "server")] use crate::common::date; use crate::error::Parse; +use crate::ext::HeaderCaseMap; use crate::headers; use crate::proto::h1::{ Encode, Encoder, Http1Transaction, ParseContext, ParseResult, ParsedMessage, @@ -731,8 +732,7 @@ impl Http1Transaction for Client { let mut keep_alive = version == Version::HTTP_11; - #[cfg(feature = "ffi")] - let mut header_case_map = crate::ffi::HeaderCaseMap::default(); + let mut header_case_map = HeaderCaseMap::default(); headers.reserve(headers_len); for header in &headers_indices[..headers_len] { @@ -750,7 +750,6 @@ impl Http1Transaction for Client { } } - #[cfg(feature = "ffi")] if ctx.preserve_header_case { header_case_map.append(&name, slice.slice(header.name.0..header.name.1)); } @@ -761,7 +760,6 @@ impl Http1Transaction for Client { #[allow(unused_mut)] let mut extensions = http::Extensions::default(); - #[cfg(feature = "ffi")] if ctx.preserve_header_case { extensions.insert(header_case_map); } @@ -829,26 +827,12 @@ impl Http1Transaction for Client { } extend(dst, b"\r\n"); - #[cfg(feature = "ffi")] - { - if msg.title_case_headers { - write_headers_title_case(&msg.head.headers, dst); - } else if let Some(orig_headers) = - msg.head.extensions.get::() - { - write_headers_original_case(&msg.head.headers, orig_headers, dst); - } else { - write_headers(&msg.head.headers, dst); - } - } - - #[cfg(not(feature = "ffi"))] - { - if msg.title_case_headers { - write_headers_title_case(&msg.head.headers, dst); - } else { - write_headers(&msg.head.headers, dst); - } + if msg.title_case_headers { + write_headers_title_case(&msg.head.headers, dst); + } else if let Some(orig_headers) = msg.head.extensions.get::() { + write_headers_original_case(&msg.head.headers, orig_headers, dst); + } else { + write_headers(&msg.head.headers, dst); } extend(dst, b"\r\n"); @@ -1161,11 +1145,10 @@ fn write_headers(headers: &HeaderMap, dst: &mut Vec) { } } -#[cfg(feature = "ffi")] #[cold] fn write_headers_original_case( headers: &HeaderMap, - orig_case: &crate::ffi::HeaderCaseMap, + orig_case: &HeaderCaseMap, dst: &mut Vec, ) { // For each header name/value pair, there may be a value in the casemap @@ -1231,7 +1214,6 @@ mod tests { ParseContext { cached_headers: &mut None, req_method: &mut method, - #[cfg(feature = "ffi")] preserve_header_case: false, h09_responses: false, }, @@ -1254,7 +1236,6 @@ mod tests { let ctx = ParseContext { cached_headers: &mut None, req_method: &mut Some(crate::Method::GET), - #[cfg(feature = "ffi")] preserve_header_case: false, h09_responses: false, }; @@ -1272,7 +1253,6 @@ mod tests { let ctx = ParseContext { cached_headers: &mut None, req_method: &mut None, - #[cfg(feature = "ffi")] preserve_header_case: false, h09_responses: false, }; @@ -1288,7 +1268,6 @@ mod tests { let ctx = ParseContext { cached_headers: &mut None, req_method: &mut Some(crate::Method::GET), - #[cfg(feature = "ffi")] preserve_header_case: false, h09_responses: true, }; @@ -1306,7 +1285,6 @@ mod tests { let ctx = ParseContext { cached_headers: &mut None, req_method: &mut Some(crate::Method::GET), - #[cfg(feature = "ffi")] preserve_header_case: false, h09_responses: false, }; @@ -1323,7 +1301,6 @@ mod tests { ParseContext { cached_headers: &mut None, req_method: &mut None, - #[cfg(feature = "ffi")] preserve_header_case: false, h09_responses: false, }, @@ -1339,7 +1316,6 @@ mod tests { ParseContext { cached_headers: &mut None, req_method: &mut None, - #[cfg(feature = "ffi")] preserve_header_case: false, h09_responses: false, }, @@ -1554,7 +1530,6 @@ mod tests { ParseContext { cached_headers: &mut None, req_method: &mut Some(Method::GET), - #[cfg(feature = "ffi")] preserve_header_case: false, h09_responses: false, } @@ -1570,7 +1545,6 @@ mod tests { ParseContext { cached_headers: &mut None, req_method: &mut Some(m), - #[cfg(feature = "ffi")] preserve_header_case: false, h09_responses: false, }, @@ -1586,7 +1560,6 @@ mod tests { ParseContext { cached_headers: &mut None, req_method: &mut Some(Method::GET), - #[cfg(feature = "ffi")] preserve_header_case: false, h09_responses: false, }, @@ -1902,7 +1875,6 @@ mod tests { ParseContext { cached_headers: &mut None, req_method: &mut Some(Method::GET), - #[cfg(feature = "ffi")] preserve_header_case: false, h09_responses: false, }, @@ -1913,13 +1885,12 @@ mod tests { assert_eq!(parsed.head.headers["server"], "hello\tworld"); } - #[cfg(feature = "ffi")] #[test] fn test_write_headers_orig_case_empty_value() { let mut headers = HeaderMap::new(); let name = http::header::HeaderName::from_static("x-empty"); headers.insert(&name, "".parse().expect("parse empty")); - let mut orig_cases = crate::ffi::HeaderCaseMap::default(); + let mut orig_cases = HeaderCaseMap::default(); orig_cases.insert(name, Bytes::from_static(b"X-EmptY")); let mut dst = Vec::new(); @@ -1931,7 +1902,6 @@ mod tests { ); } - #[cfg(feature = "ffi")] #[test] fn test_write_headers_orig_case_multiple_entries() { let mut headers = HeaderMap::new(); @@ -1939,7 +1909,7 @@ mod tests { headers.insert(&name, "a".parse().unwrap()); headers.append(&name, "b".parse().unwrap()); - let mut orig_cases = crate::ffi::HeaderCaseMap::default(); + let mut orig_cases = HeaderCaseMap::default(); orig_cases.insert(name.clone(), Bytes::from_static(b"X-Empty")); orig_cases.append(name, Bytes::from_static(b"X-EMPTY")); @@ -1984,7 +1954,6 @@ mod tests { ParseContext { cached_headers: &mut headers, req_method: &mut None, - #[cfg(feature = "ffi")] preserve_header_case: false, h09_responses: false, }, @@ -2020,7 +1989,6 @@ mod tests { ParseContext { cached_headers: &mut headers, req_method: &mut None, - #[cfg(feature = "ffi")] preserve_header_case: false, h09_responses: false, },