From 39c33eefa9741ed6460ad4953736b45e6a96615d 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/proto/h1/conn.rs | 18 ++++---------- src/proto/h1/io.rs | 2 -- src/proto/h1/mod.rs | 25 +++++++++++++++++-- src/proto/h1/role.rs | 56 ++++++++++--------------------------------- 9 files changed, 71 insertions(+), 86 deletions(-) diff --git a/src/client/client.rs b/src/client/client.rs index 37a3251a7a..f01c2a2354 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 the connection **must** use HTTP/2. /// /// The destination must either allow HTTP2 Prior Knowledge, or the diff --git a/src/client/conn.rs b/src/client/conn.rs index 2da083db16..f2a44ca875 100644 --- a/src/client/conn.rs +++ b/src/client/conn.rs @@ -123,6 +123,7 @@ where pub struct Builder { pub(super) exec: Exec, h1_title_case_headers: bool, + h1_preserve_header_case: bool, h1_read_buf_exact_size: Option, h1_max_buf_size: Option, #[cfg(feature = "http2")] @@ -495,6 +496,7 @@ impl Builder { exec: Exec::Default, 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(), @@ -519,6 +521,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; @@ -700,6 +707,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 let Some(sz) = opts.h1_read_buf_exact_size { conn.set_read_buf_exact_size(sz); } diff --git a/src/ffi/client.rs b/src/ffi/client.rs index a64842b6a3..956c4aba84 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(), })) } diff --git a/src/ffi/http_types.rs b/src/ffi/http_types.rs index edb43ed31c..7e9fb0ba44 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 39f3276bc8..08acd76806 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/proto/h1/conn.rs b/src/proto/h1/conn.rs index 174a1d8695..ab1b23876e 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, notify_read: false, @@ -78,6 +77,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 = "server")] pub(crate) fn set_allow_half_close(&mut self) { self.state.allow_half_close = true; @@ -144,7 +148,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, } )) { @@ -478,16 +481,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 { @@ -750,7 +743,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, /// Set to true when the Dispatcher should poll read operations diff --git a/src/proto/h1/io.rs b/src/proto/h1/io.rs index 5536b5d164..fa9d42040e 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, }, )? { @@ -638,7 +637,6 @@ mod tests { let parse_ctx = ParseContext { cached_headers: &mut None, req_method: &mut None, - #[cfg(feature = "ffi")] preserve_header_case: false, }; assert!(buffered diff --git a/src/proto/h1/mod.rs b/src/proto/h1/mod.rs index 1498872ea8..59754a4bfd 100644 --- a/src/proto/h1/mod.rs +++ b/src/proto/h1/mod.rs @@ -1,4 +1,5 @@ -use bytes::BytesMut; +use bytes::{Bytes, BytesMut}; +use http::header::{GetAll, HeaderName, IntoHeaderName}; use http::{HeaderMap, Method}; use crate::body::DecodedLength; @@ -70,7 +71,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, } @@ -102,3 +102,24 @@ impl Wants { (self.0 & other.0) == other.0 } } + +#[derive(Debug, Default)] +pub(crate) struct HeaderCaseMap(HeaderMap); + +impl HeaderCaseMap { + pub(crate) fn get_all(&self, name: &HeaderName) -> GetAll<'_, Bytes> { + self.0.get_all(name) + } + + #[cfg(any(test, feature = "ffi"))] + 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: IntoHeaderName, + { + self.0.append(name, orig); + } +} diff --git a/src/proto/h1/role.rs b/src/proto/h1/role.rs index b7310ba99d..7f23350674 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}; @@ -17,7 +17,8 @@ use crate::common::date; use crate::error::Parse; use crate::headers; use crate::proto::h1::{ - Encode, Encoder, Http1Transaction, ParseContext, ParseResult, ParsedMessage, + Encode, Encoder, HeaderCaseMap, Http1Transaction, ParseContext, ParseResult, + ParsedMessage, }; use crate::proto::{BodyLength, MessageHead, RequestHead, RequestLine}; @@ -720,8 +721,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] { @@ -739,7 +739,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)); } @@ -750,7 +749,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); } @@ -818,26 +816,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"); @@ -1150,11 +1134,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 @@ -1220,7 +1203,6 @@ mod tests { ParseContext { cached_headers: &mut None, req_method: &mut method, - #[cfg(feature = "ffi")] preserve_header_case: false, }, ) @@ -1242,7 +1224,6 @@ mod tests { let ctx = ParseContext { cached_headers: &mut None, req_method: &mut Some(crate::Method::GET), - #[cfg(feature = "ffi")] preserve_header_case: false, }; let msg = Client::parse(&mut raw, ctx).unwrap().unwrap(); @@ -1259,7 +1240,6 @@ mod tests { let ctx = ParseContext { cached_headers: &mut None, req_method: &mut None, - #[cfg(feature = "ffi")] preserve_header_case: false, }; Server::parse(&mut raw, ctx).unwrap_err(); @@ -1274,7 +1254,6 @@ mod tests { ParseContext { cached_headers: &mut None, req_method: &mut None, - #[cfg(feature = "ffi")] preserve_header_case: false, }, ) @@ -1289,7 +1268,6 @@ mod tests { ParseContext { cached_headers: &mut None, req_method: &mut None, - #[cfg(feature = "ffi")] preserve_header_case: false, }, ) @@ -1503,7 +1481,6 @@ mod tests { ParseContext { cached_headers: &mut None, req_method: &mut Some(Method::GET), - #[cfg(feature = "ffi")] preserve_header_case: false, } ) @@ -1518,7 +1495,6 @@ mod tests { ParseContext { cached_headers: &mut None, req_method: &mut Some(m), - #[cfg(feature = "ffi")] preserve_header_case: false, }, ) @@ -1533,7 +1509,6 @@ mod tests { ParseContext { cached_headers: &mut None, req_method: &mut Some(Method::GET), - #[cfg(feature = "ffi")] preserve_header_case: false, }, ) @@ -1848,7 +1823,6 @@ mod tests { ParseContext { cached_headers: &mut None, req_method: &mut Some(Method::GET), - #[cfg(feature = "ffi")] preserve_header_case: false, }, ) @@ -1858,13 +1832,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(); @@ -1876,7 +1849,6 @@ mod tests { ); } - #[cfg(feature = "ffi")] #[test] fn test_write_headers_orig_case_multiple_entries() { let mut headers = HeaderMap::new(); @@ -1884,7 +1856,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")); @@ -1929,7 +1901,6 @@ mod tests { ParseContext { cached_headers: &mut headers, req_method: &mut None, - #[cfg(feature = "ffi")] preserve_header_case: false, }, ) @@ -1964,7 +1935,6 @@ mod tests { ParseContext { cached_headers: &mut headers, req_method: &mut None, - #[cfg(feature = "ffi")] preserve_header_case: false, }, )