From f4fec5ce017cdcd6e1ca1706d17f3b0720dee67d Mon Sep 17 00:00:00 2001 From: Liam Warfield Date: Tue, 29 Mar 2022 18:29:38 -0600 Subject: [PATCH 1/3] feature(ffi): Added connection option to preserve header order. Libcurl expects that headers are iterated in the same order that they are recieved. Previously this caused curl tests 580 and 581 to fail. This necessitated exposing a way to preserve the original ordering of http headers. SUMMARY OF CHANGES: Add a new data structure called OriginalHeaderOrder that represents the order in which headers originally appear in a HTTP message. This datastructure is `Vec<(Headername, multimap-index)>`. This vector is ordered by the order which headers were recieved. Add the following ffi functions: - ffi::client::hyper_clientconn_options_set_preserve_header_order : An ffi interface to configure a connection to preserve header order. - ffi::client::hyper_clientconn_options_set_preserve_header_case : An ffi interface to configure a connection to preserve header case. - ffi::http_types::hyper_headers_foreach_ordered : Iterates the headers in the order the were recieved, passing each name and value pair to the callback. Add a new option to ParseContext, and Conn::State called `preserve_header_order`. This option, and all the code paths it creates are behind the `ffi` feature flag. This should not change performance of response parsing for non-ffi users. CLOSES ISSUE: #2780 --- capi/include/hyper.h | 28 +++++++++++ src/client/conn.rs | 21 ++++++++ src/ext.rs | 50 ++++++++++++++++++- src/ffi/client.rs | 21 ++++++++ src/ffi/http_types.rs | 108 ++++++++++++++++++++++++++++++++++++++++-- src/proto/h1/conn.rs | 11 +++++ src/proto/h1/io.rs | 32 ++++++++----- src/proto/h1/mod.rs | 5 +- src/proto/h1/role.rs | 71 ++++++++++++++++++++++++++- 9 files changed, 327 insertions(+), 20 deletions(-) diff --git a/capi/include/hyper.h b/capi/include/hyper.h index efe5f06106..2a7d03d598 100644 --- a/capi/include/hyper.h +++ b/capi/include/hyper.h @@ -355,6 +355,22 @@ void hyper_clientconn_free(struct hyper_clientconn *conn); */ struct hyper_clientconn_options *hyper_clientconn_options_new(void); +/* + Set the whether or not header case is preserved. + + Pass `0` to allow lowercase normalization (default), `1` to retain original case. + */ +void hyper_clientconn_options_set_preserve_header_case(struct hyper_clientconn_options *opts, + int enabled); + +/* + Set the whether or not header order is preserved. + + Pass `0` to allow reordering (default), `1` to retain original ordering. + */ +void hyper_clientconn_options_set_preserve_header_order(struct hyper_clientconn_options *opts, + int enabled); + /* Free a `hyper_clientconn_options *`. */ @@ -595,6 +611,18 @@ void hyper_headers_foreach(const struct hyper_headers *headers, hyper_headers_foreach_callback func, void *userdata); +/* + Iterates the headers in the order the were recieved, passing each name and value pair to the callback. + + The `userdata` pointer is also passed to the callback. + + The callback should return `HYPER_ITER_CONTINUE` to keep iterating, or + `HYPER_ITER_BREAK` to stop. + */ +void hyper_headers_foreach_ordered(const struct hyper_headers *headers, + hyper_headers_foreach_callback func, + void *userdata); + /* Sets the header with the provided name to the provided value. diff --git a/src/client/conn.rs b/src/client/conn.rs index 85bc366be9..91a8551705 100644 --- a/src/client/conn.rs +++ b/src/client/conn.rs @@ -156,6 +156,8 @@ pub struct Builder { h1_writev: Option, h1_title_case_headers: bool, h1_preserve_header_case: bool, + #[cfg(feature = "ffi")] + h1_preserve_header_order: bool, h1_read_buf_exact_size: Option, h1_max_buf_size: Option, #[cfg(feature = "ffi")] @@ -558,6 +560,7 @@ impl Builder { h1_parser_config: Default::default(), h1_title_case_headers: false, h1_preserve_header_case: false, + h1_preserve_header_order: false, h1_max_buf_size: None, #[cfg(feature = "ffi")] h1_headers_raw: false, @@ -704,6 +707,21 @@ impl Builder { self } + /// Set whether to support preserving original header order. + /// + /// Currently, this will record the order in which headers are received, and store this + /// ordering in a private extension on the `Response`. It will also look for and use + /// such an extension in any provided `Request`. + /// + /// Note that this setting does not affect HTTP/2. + /// + /// Default is false. + #[cfg(feature = "ffi")] + pub fn http1_preserve_header_order(&mut self, enabled: bool) -> &mut Builder { + self.h1_preserve_header_order = enabled; + self + } + /// Sets the exact size of the read buffer to *always* use. /// /// Note that setting this option unsets the `http1_max_buf_size` option. @@ -951,6 +969,9 @@ impl Builder { if opts.h1_preserve_header_case { conn.set_preserve_header_case(); } + if opts.h1_preserve_header_order { + conn.set_preserve_header_order(); + } if opts.h09_responses { conn.set_h09_responses(); } diff --git a/src/ext.rs b/src/ext.rs index e9d4587784..28e082ee83 100644 --- a/src/ext.rs +++ b/src/ext.rs @@ -1,9 +1,11 @@ //! HTTP extensions. use bytes::Bytes; +use http::header::HeaderName; #[cfg(feature = "http1")] -use http::header::{HeaderName, IntoHeaderName, ValueIter}; +use http::header::{IntoHeaderName, ValueIter}; use http::HeaderMap; +use std::collections::HashMap; #[cfg(feature = "http2")] use std::fmt; @@ -120,3 +122,49 @@ impl HeaderCaseMap { self.0.append(name, orig); } } + +#[derive(Clone, Debug)] +/// Hashmap +pub(crate) struct OriginalHeaderOrder(HashMap, Vec<(HeaderName, usize)>); + +#[cfg(all(feature = "http1", feature = "ffi"))] +impl OriginalHeaderOrder { + pub(crate) fn default() -> Self { + Self(Default::default(), Default::default()) + } + + #[cfg(any(test, feature = "ffi"))] + pub(crate) fn insert(&mut self, name: HeaderName) { + if !self.0.contains_key(&name) { + let idx = 0; + self.0.insert(name.clone(), 1); + self.1.push((name, idx)); + } + // replacing an already existing element does not + // change ordering, so we only care if its the first + // header name encountered + } + + pub(crate) fn append(&mut self, name: N) + where + N: IntoHeaderName + Into + Clone, + { + let name: HeaderName = name.into(); + let idx; + if self.0.contains_key(&name) { + idx = self.0[&name]; + *self.0.get_mut(&name).unwrap() += 1; + } else { + idx = 0; + self.0.insert(name.clone(), 1); + } + self.1.push((name, idx)); + } + + /// This returns an iterator that provides header names and indexes + /// in the original order recieved. + #[cfg(any(test, feature = "ffi"))] + pub(crate) fn get_in_order(&self) -> impl Iterator { + self.1.iter() + } +} diff --git a/src/ffi/client.rs b/src/ffi/client.rs index 1e5f29d548..76da0a6e05 100644 --- a/src/ffi/client.rs +++ b/src/ffi/client.rs @@ -95,6 +95,7 @@ ffi_fn! { fn hyper_clientconn_options_new() -> *mut hyper_clientconn_options { let mut builder = conn::Builder::new(); builder.http1_preserve_header_case(true); + builder.http1_preserve_header_order(true); Box::into_raw(Box::new(hyper_clientconn_options { builder, @@ -103,6 +104,26 @@ ffi_fn! { } ?= std::ptr::null_mut() } +ffi_fn! { + /// Set the whether or not header case is preserved. + /// + /// Pass `0` to allow lowercase normalization (default), `1` to retain original case. + fn hyper_clientconn_options_set_preserve_header_case(opts: *mut hyper_clientconn_options, enabled: c_int) { + let opts = non_null! { &mut *opts ?= () }; + opts.builder.http1_preserve_header_case(enabled != 0); + } +} + +ffi_fn! { + /// Set the whether or not header order is preserved. + /// + /// Pass `0` to allow reordering (default), `1` to retain original ordering. + fn hyper_clientconn_options_set_preserve_header_order(opts: *mut hyper_clientconn_options, enabled: c_int) { + let opts = non_null! { &mut *opts ?= () }; + opts.builder.http1_preserve_header_order(enabled != 0); + } +} + ffi_fn! { /// Free a `hyper_clientconn_options *`. fn hyper_clientconn_options_free(opts: *mut hyper_clientconn_options) { diff --git a/src/ffi/http_types.rs b/src/ffi/http_types.rs index f6d32947bf..d6d347961c 100644 --- a/src/ffi/http_types.rs +++ b/src/ffi/http_types.rs @@ -6,7 +6,7 @@ use super::body::{hyper_body, hyper_buf}; use super::error::hyper_code; use super::task::{hyper_task_return_type, AsTaskType}; use super::{UserDataPointer, HYPER_ITER_CONTINUE}; -use crate::ext::HeaderCaseMap; +use crate::ext::{HeaderCaseMap, OriginalHeaderOrder}; use crate::header::{HeaderName, HeaderValue}; use crate::{Body, HeaderMap, Method, Request, Response, Uri}; @@ -22,6 +22,7 @@ pub struct hyper_response(pub(super) Response); pub struct hyper_headers { pub(super) headers: HeaderMap, orig_casing: HeaderCaseMap, + orig_order: OriginalHeaderOrder, } #[derive(Debug)] @@ -233,6 +234,7 @@ impl hyper_request { if let Some(headers) = self.0.extensions_mut().remove::() { *self.0.headers_mut() = headers.headers; self.0.extensions_mut().insert(headers.orig_casing); + self.0.extensions_mut().insert(headers.orig_order); } } } @@ -348,9 +350,14 @@ impl hyper_response { .extensions_mut() .remove::() .unwrap_or_else(HeaderCaseMap::default); + let orig_order = resp + .extensions_mut() + .remove::() + .unwrap_or_else(OriginalHeaderOrder::default); resp.extensions_mut().insert(hyper_headers { headers, orig_casing, + orig_order, }); hyper_response(resp) @@ -428,6 +435,37 @@ ffi_fn! { } } +ffi_fn! { + /// Iterates the headers in the order the were recieved, passing each name and value pair to the callback. + /// + /// The `userdata` pointer is also passed to the callback. + /// + /// The callback should return `HYPER_ITER_CONTINUE` to keep iterating, or + /// `HYPER_ITER_BREAK` to stop. + fn hyper_headers_foreach_ordered(headers: *const hyper_headers, func: hyper_headers_foreach_callback, userdata: *mut c_void) { + let headers = non_null!(&*headers ?= ()); + // For each header name/value pair, there may be a value in the casemap + // that corresponds to the HeaderValue. So, we iterator all the keys, + // and for each one, try to pair the originally cased name with the value. + // + // TODO: consider adding http::HeaderMap::entries() iterator + for (name, idx) in headers.orig_order.get_in_order() { + let orig_name = headers.orig_casing.get_all(&name).nth(*idx).unwrap(); + let value = headers.headers.get_all(name).iter().nth(*idx).unwrap(); + + let name_ptr = orig_name.as_ref().as_ptr(); + let name_len = orig_name.as_ref().len(); + + let val_ptr = value.as_bytes().as_ptr(); + let val_len = value.as_bytes().len(); + + if HYPER_ITER_CONTINUE != func(userdata, name_ptr, name_len, val_ptr, val_len) { + return; + } + } + } +} + ffi_fn! { /// Sets the header with the provided name to the provided value. /// @@ -437,7 +475,8 @@ ffi_fn! { match unsafe { raw_name_value(name, name_len, value, value_len) } { Ok((name, value, orig_name)) => { headers.headers.insert(&name, value); - headers.orig_casing.insert(name, orig_name); + headers.orig_casing.insert(name.clone(), orig_name.clone()); + headers.orig_order.insert(name); hyper_code::HYPERE_OK } Err(code) => code, @@ -456,7 +495,8 @@ ffi_fn! { match unsafe { raw_name_value(name, name_len, value, value_len) } { Ok((name, value, orig_name)) => { headers.headers.append(&name, value); - headers.orig_casing.append(name, orig_name); + headers.orig_casing.append(&name, orig_name.clone()); + headers.orig_order.append(name); hyper_code::HYPERE_OK } Err(code) => code, @@ -469,6 +509,7 @@ impl Default for hyper_headers { Self { headers: Default::default(), orig_casing: HeaderCaseMap::default(), + orig_order: OriginalHeaderOrder::default(), } } } @@ -555,4 +596,65 @@ mod tests { HYPER_ITER_CONTINUE } } + + #[cfg(all(feature = "http1", feature = "ffi"))] + #[test] + fn test_headers_foreach_order_preserved() { + let mut headers = hyper_headers::default(); + + let name1 = b"Set-CookiE"; + let value1 = b"a=b"; + hyper_headers_add( + &mut headers, + name1.as_ptr(), + name1.len(), + value1.as_ptr(), + value1.len(), + ); + + let name2 = b"Content-Encoding"; + let value2 = b"gzip"; + hyper_headers_add( + &mut headers, + name2.as_ptr(), + name2.len(), + value2.as_ptr(), + value2.len(), + ); + + let name3 = b"SET-COOKIE"; + let value3 = b"c=d"; + hyper_headers_add( + &mut headers, + name3.as_ptr(), + name3.len(), + value3.as_ptr(), + value3.len(), + ); + + let mut vec = Vec::::new(); + hyper_headers_foreach_ordered(&headers, concat, &mut vec as *mut _ as *mut c_void); + + println!("{}", std::str::from_utf8(&vec).unwrap()); + assert_eq!(vec, b"Set-CookiE: a=b\r\nContent-Encoding: gzip\r\nSET-COOKIE: c=d\r\n"); + + extern "C" fn concat( + vec: *mut c_void, + name: *const u8, + name_len: usize, + value: *const u8, + value_len: usize, + ) -> c_int { + unsafe { + let vec = &mut *(vec as *mut Vec); + let name = std::slice::from_raw_parts(name, name_len); + let value = std::slice::from_raw_parts(value, value_len); + vec.extend(name); + vec.extend(b": "); + vec.extend(value); + vec.extend(b"\r\n"); + } + HYPER_ITER_CONTINUE + } + } } diff --git a/src/proto/h1/conn.rs b/src/proto/h1/conn.rs index 66b2cdacc3..74571baa15 100644 --- a/src/proto/h1/conn.rs +++ b/src/proto/h1/conn.rs @@ -58,6 +58,8 @@ where #[cfg(all(feature = "server", feature = "runtime"))] h1_header_read_timeout_running: false, preserve_header_case: false, + #[cfg(feature = "ffi")] + preserve_header_order: false, title_case_headers: false, h09_responses: false, #[cfg(feature = "ffi")] @@ -111,6 +113,11 @@ where self.state.preserve_header_case = true; } + #[cfg(feature = "ffi")] + pub(crate) fn set_preserve_header_order(&mut self) { + self.state.preserve_header_order = true; + } + #[cfg(feature = "client")] pub(crate) fn set_h09_responses(&mut self) { self.state.h09_responses = true; @@ -200,6 +207,8 @@ where #[cfg(all(feature = "server", feature = "runtime"))] h1_header_read_timeout_running: &mut self.state.h1_header_read_timeout_running, preserve_header_case: self.state.preserve_header_case, + #[cfg(feature = "ffi")] + preserve_header_order: self.state.preserve_header_order, h09_responses: self.state.h09_responses, #[cfg(feature = "ffi")] on_informational: &mut self.state.on_informational, @@ -824,6 +833,8 @@ struct State { #[cfg(all(feature = "server", feature = "runtime"))] h1_header_read_timeout_running: bool, preserve_header_case: bool, + #[cfg(feature = "ffi")] + preserve_header_order: bool, title_case_headers: bool, h09_responses: bool, /// If set, called with each 1xx informational response received for diff --git a/src/proto/h1/io.rs b/src/proto/h1/io.rs index 08a3993684..7d0bc56eba 100644 --- a/src/proto/h1/io.rs +++ b/src/proto/h1/io.rs @@ -1,17 +1,17 @@ use std::cmp; use std::fmt; +#[cfg(all(feature = "server", feature = "runtime"))] +use std::future::Future; use std::io::{self, IoSlice}; use std::marker::Unpin; use std::mem::MaybeUninit; #[cfg(all(feature = "server", feature = "runtime"))] -use std::future::Future; -#[cfg(all(feature = "server", feature = "runtime"))] use std::time::Duration; -#[cfg(all(feature = "server", feature = "runtime"))] -use tokio::time::Instant; use bytes::{Buf, BufMut, Bytes, BytesMut}; use tokio::io::{AsyncRead, AsyncWrite, ReadBuf}; +#[cfg(all(feature = "server", feature = "runtime"))] +use tokio::time::Instant; use tracing::{debug, trace}; use super::{Http1Transaction, ParseContext, ParsedMessage}; @@ -194,6 +194,8 @@ where #[cfg(all(feature = "server", feature = "runtime"))] h1_header_read_timeout_running: parse_ctx.h1_header_read_timeout_running, preserve_header_case: parse_ctx.preserve_header_case, + #[cfg(feature = "ffi")] + preserve_header_order: parse_ctx.preserve_header_order, h09_responses: parse_ctx.h09_responses, #[cfg(feature = "ffi")] on_informational: parse_ctx.on_informational, @@ -208,9 +210,13 @@ where { *parse_ctx.h1_header_read_timeout_running = false; - if let Some(h1_header_read_timeout_fut) = parse_ctx.h1_header_read_timeout_fut { + if let Some(h1_header_read_timeout_fut) = + parse_ctx.h1_header_read_timeout_fut + { // Reset the timer in order to avoid woken up when the timeout finishes - h1_header_read_timeout_fut.as_mut().reset(Instant::now() + Duration::from_secs(30 * 24 * 60 * 60)); + h1_header_read_timeout_fut + .as_mut() + .reset(Instant::now() + Duration::from_secs(30 * 24 * 60 * 60)); } } return Poll::Ready(Ok(msg)); @@ -224,12 +230,14 @@ where #[cfg(all(feature = "server", feature = "runtime"))] if *parse_ctx.h1_header_read_timeout_running { - if let Some(h1_header_read_timeout_fut) = parse_ctx.h1_header_read_timeout_fut { - if Pin::new( h1_header_read_timeout_fut).poll(cx).is_ready() { + if let Some(h1_header_read_timeout_fut) = + parse_ctx.h1_header_read_timeout_fut + { + if Pin::new(h1_header_read_timeout_fut).poll(cx).is_ready() { *parse_ctx.h1_header_read_timeout_running = false; tracing::warn!("read header from client timeout"); - return Poll::Ready(Err(crate::Error::new_header_timeout())) + return Poll::Ready(Err(crate::Error::new_header_timeout())); } } } @@ -731,6 +739,8 @@ mod tests { h1_header_read_timeout_fut: &mut None, h1_header_read_timeout_running: &mut false, preserve_header_case: false, + #[cfg(feature = "ffi")] + preserve_header_order: false, h09_responses: false, #[cfg(feature = "ffi")] on_informational: &mut None, @@ -894,9 +904,7 @@ mod tests { async fn write_buf_flatten() { let _ = pretty_env_logger::try_init(); - let mock = Mock::new() - .write(b"hello world, it's hyper!") - .build(); + let mock = Mock::new().write(b"hello world, it's hyper!").build(); let mut buffered = Buffered::<_, Cursor>>::new(mock); buffered.write_buf.set_strategy(WriteStrategy::Flatten); diff --git a/src/proto/h1/mod.rs b/src/proto/h1/mod.rs index 06d03bf5f1..5a2587a843 100644 --- a/src/proto/h1/mod.rs +++ b/src/proto/h1/mod.rs @@ -14,7 +14,7 @@ pub(crate) use self::conn::Conn; pub(crate) use self::decode::Decoder; pub(crate) use self::dispatch::Dispatcher; pub(crate) use self::encode::{EncodedBuf, Encoder}; - //TODO: move out of h1::io +//TODO: move out of h1::io pub(crate) use self::io::MINIMUM_MAX_BUFFER_SIZE; mod conn; @@ -24,7 +24,6 @@ mod encode; mod io; mod role; - cfg_client! { pub(crate) type ClientTransaction = role::Client; } @@ -84,6 +83,8 @@ pub(crate) struct ParseContext<'a> { #[cfg(all(feature = "server", feature = "runtime"))] h1_header_read_timeout_running: &'a mut bool, preserve_header_case: bool, + #[cfg(feature = "ffi")] + preserve_header_order: bool, h09_responses: bool, #[cfg(feature = "ffi")] on_informational: &'a mut Option, diff --git a/src/proto/h1/role.rs b/src/proto/h1/role.rs index 968b63cb8e..4c933f42ed 100644 --- a/src/proto/h1/role.rs +++ b/src/proto/h1/role.rs @@ -16,7 +16,7 @@ use crate::body::DecodedLength; #[cfg(feature = "server")] use crate::common::date; use crate::error::Parse; -use crate::ext::HeaderCaseMap; +use crate::ext::{HeaderCaseMap, OriginalHeaderOrder}; use crate::headers; use crate::proto::h1::{ Encode, Encoder, Http1Transaction, ParseContext, ParseResult, ParsedMessage, @@ -214,6 +214,13 @@ impl Http1Transaction for Server { None }; + #[cfg(feature = "ffi")] + let mut header_order = if ctx.preserve_header_order { + Some(OriginalHeaderOrder::default()) + } else { + None + }; + let mut headers = ctx.cached_headers.take().unwrap_or_else(HeaderMap::new); headers.reserve(headers_len); @@ -290,6 +297,11 @@ impl Http1Transaction for Server { header_case_map.append(&name, slice.slice(header.name.0..header.name.1)); } + #[cfg(feature = "ffi")] + if let Some(ref mut header_order) = header_order { + header_order.append(&name); + } + headers.append(name, value); } @@ -304,6 +316,10 @@ impl Http1Transaction for Server { extensions.insert(header_case_map); } + if let Some(header_order) = header_order { + extensions.insert(header_order); + } + *ctx.req_method = Some(subject.0.clone()); Ok(Some(ParsedMessage { @@ -957,7 +973,10 @@ impl Http1Transaction for Client { let mut slice = buf.split_to(len); - if ctx.h1_parser_config.obsolete_multiline_headers_in_responses_are_allowed() { + if ctx + .h1_parser_config + .obsolete_multiline_headers_in_responses_are_allowed() + { for header in &headers_indices[..headers_len] { // SAFETY: array is valid up to `headers_len` let header = unsafe { &*header.as_ptr() }; @@ -981,6 +1000,13 @@ impl Http1Transaction for Client { None }; + #[cfg(feature = "ffi")] + let mut header_order = if ctx.preserve_header_order { + Some(OriginalHeaderOrder::default()) + } else { + None + }; + headers.reserve(headers_len); for header in &headers_indices[..headers_len] { // SAFETY: array is valid up to `headers_len` @@ -1003,6 +1029,11 @@ impl Http1Transaction for Client { header_case_map.append(&name, slice.slice(header.name.0..header.name.1)); } + #[cfg(feature = "ffi")] + if let Some(ref mut header_order) = header_order { + header_order.append(&name); + } + headers.append(name, value); } @@ -1012,6 +1043,10 @@ impl Http1Transaction for Client { extensions.insert(header_case_map); } + if let Some(header_order) = header_order { + extensions.insert(header_order); + } + #[cfg(feature = "ffi")] if let Some(reason) = reason { extensions.insert(crate::ffi::ReasonPhrase(reason)); @@ -1478,6 +1513,8 @@ mod tests { h1_header_read_timeout_fut: &mut None, h1_header_read_timeout_running: &mut false, preserve_header_case: false, + #[cfg(feature = "ffi")] + preserve_header_order: false, h09_responses: false, #[cfg(feature = "ffi")] on_informational: &mut None, @@ -1508,6 +1545,8 @@ mod tests { h1_header_read_timeout_fut: &mut None, h1_header_read_timeout_running: &mut false, preserve_header_case: false, + #[cfg(feature = "ffi")] + preserve_header_order: false, h09_responses: false, #[cfg(feature = "ffi")] on_informational: &mut None, @@ -1533,6 +1572,8 @@ mod tests { h1_header_read_timeout_fut: &mut None, h1_header_read_timeout_running: &mut false, preserve_header_case: false, + #[cfg(feature = "ffi")] + preserve_header_order: false, h09_responses: false, #[cfg(feature = "ffi")] on_informational: &mut None, @@ -1556,6 +1597,8 @@ mod tests { h1_header_read_timeout_fut: &mut None, h1_header_read_timeout_running: &mut false, preserve_header_case: false, + #[cfg(feature = "ffi")] + preserve_header_order: false, h09_responses: true, #[cfg(feature = "ffi")] on_informational: &mut None, @@ -1581,6 +1624,8 @@ mod tests { h1_header_read_timeout_fut: &mut None, h1_header_read_timeout_running: &mut false, preserve_header_case: false, + #[cfg(feature = "ffi")] + preserve_header_order: false, h09_responses: false, #[cfg(feature = "ffi")] on_informational: &mut None, @@ -1610,6 +1655,8 @@ mod tests { h1_header_read_timeout_fut: &mut None, h1_header_read_timeout_running: &mut false, preserve_header_case: false, + #[cfg(feature = "ffi")] + preserve_header_order: false, h09_responses: false, #[cfg(feature = "ffi")] on_informational: &mut None, @@ -1636,6 +1683,8 @@ mod tests { h1_header_read_timeout_fut: &mut None, h1_header_read_timeout_running: &mut false, preserve_header_case: false, + #[cfg(feature = "ffi")] + preserve_header_order: false, h09_responses: false, #[cfg(feature = "ffi")] on_informational: &mut None, @@ -1657,6 +1706,8 @@ mod tests { h1_header_read_timeout_fut: &mut None, h1_header_read_timeout_running: &mut false, preserve_header_case: true, + #[cfg(feature = "ffi")] + preserve_header_order: false, h09_responses: false, #[cfg(feature = "ffi")] on_informational: &mut None, @@ -1699,6 +1750,8 @@ mod tests { h1_header_read_timeout_fut: &mut None, h1_header_read_timeout_running: &mut false, preserve_header_case: false, + #[cfg(feature = "ffi")] + preserve_header_order: false, h09_responses: false, #[cfg(feature = "ffi")] on_informational: &mut None, @@ -1722,6 +1775,8 @@ mod tests { h1_header_read_timeout_fut: &mut None, h1_header_read_timeout_running: &mut false, preserve_header_case: false, + #[cfg(feature = "ffi")] + preserve_header_order: false, h09_responses: false, #[cfg(feature = "ffi")] on_informational: &mut None, @@ -1954,6 +2009,8 @@ mod tests { h1_header_read_timeout_fut: &mut None, h1_header_read_timeout_running: &mut false, preserve_header_case: false, + #[cfg(feature = "ffi")] + preserve_header_order: false, h09_responses: false, #[cfg(feature = "ffi")] on_informational: &mut None, @@ -1977,6 +2034,8 @@ mod tests { h1_header_read_timeout_fut: &mut None, h1_header_read_timeout_running: &mut false, preserve_header_case: false, + #[cfg(feature = "ffi")] + preserve_header_order: false, h09_responses: false, #[cfg(feature = "ffi")] on_informational: &mut None, @@ -2000,6 +2059,8 @@ mod tests { h1_header_read_timeout_fut: &mut None, h1_header_read_timeout_running: &mut false, preserve_header_case: false, + #[cfg(feature = "ffi")] + preserve_header_order: false, h09_responses: false, #[cfg(feature = "ffi")] on_informational: &mut None, @@ -2500,6 +2561,8 @@ mod tests { h1_header_read_timeout_fut: &mut None, h1_header_read_timeout_running: &mut false, preserve_header_case: false, + #[cfg(feature = "ffi")] + preserve_header_order: false, h09_responses: false, #[cfg(feature = "ffi")] on_informational: &mut None, @@ -2587,6 +2650,8 @@ mod tests { h1_header_read_timeout_fut: &mut None, h1_header_read_timeout_running: &mut false, preserve_header_case: false, + #[cfg(feature = "ffi")] + preserve_header_order: false, h09_responses: false, #[cfg(feature = "ffi")] on_informational: &mut None, @@ -2630,6 +2695,8 @@ mod tests { h1_header_read_timeout_fut: &mut None, h1_header_read_timeout_running: &mut false, preserve_header_case: false, + #[cfg(feature = "ffi")] + preserve_header_order: false, h09_responses: false, #[cfg(feature = "ffi")] on_informational: &mut None, From 720743ef00723dcd67cd4c49632a970900a7e938 Mon Sep 17 00:00:00 2001 From: Liam Warfield Date: Wed, 13 Apr 2022 22:00:13 -0600 Subject: [PATCH 2/3] refactor(ffi): add docs to OriginalHeaderOrder and refactor hyper_headers_foreach_ordered hyper_headers_foreach_ordered was merged into hyper_headers_foreach. OriginalHeaderOrder docs were given examples OriginalHeaderOrder has named members for better clarity. BREAKING CHANGE: ffi::client::hyper_clientconn_options_new no longer sets the http1_preserve_header_case connection option by default. Users should now call ffi::client::hyper_clientconn_options_set_preserve_header_order if they desire that functionality --- capi/include/hyper.h | 12 ------- src/ext.rs | 77 ++++++++++++++++++++++++++++++++-------- src/ffi/client.rs | 2 -- src/ffi/http_types.rs | 82 +++++++++++++++++++++---------------------- 4 files changed, 104 insertions(+), 69 deletions(-) diff --git a/capi/include/hyper.h b/capi/include/hyper.h index 2a7d03d598..1f938b8714 100644 --- a/capi/include/hyper.h +++ b/capi/include/hyper.h @@ -611,18 +611,6 @@ void hyper_headers_foreach(const struct hyper_headers *headers, hyper_headers_foreach_callback func, void *userdata); -/* - Iterates the headers in the order the were recieved, passing each name and value pair to the callback. - - The `userdata` pointer is also passed to the callback. - - The callback should return `HYPER_ITER_CONTINUE` to keep iterating, or - `HYPER_ITER_BREAK` to stop. - */ -void hyper_headers_foreach_ordered(const struct hyper_headers *headers, - hyper_headers_foreach_callback func, - void *userdata); - /* Sets the header with the provided name to the provided value. diff --git a/src/ext.rs b/src/ext.rs index 28e082ee83..59a01ca964 100644 --- a/src/ext.rs +++ b/src/ext.rs @@ -125,22 +125,34 @@ impl HeaderCaseMap { #[derive(Clone, Debug)] /// Hashmap -pub(crate) struct OriginalHeaderOrder(HashMap, Vec<(HeaderName, usize)>); +pub(crate) struct OriginalHeaderOrder { + /// Stores how many entries a Headername maps to. This is used + /// for accounting. + num_entries: HashMap, + /// Stores the ordering of the headers. ex: `vec[i] = (headerName, idx)`, + /// The vector is ordered such that the ith element + /// represents the ith header that came in off the line. + /// The `HeaderName` and `idx` are then used elsewhere to index into + /// the multi map that stores the header values. + entry_order: Vec<(HeaderName, usize)>, +} #[cfg(all(feature = "http1", feature = "ffi"))] impl OriginalHeaderOrder { pub(crate) fn default() -> Self { - Self(Default::default(), Default::default()) + OriginalHeaderOrder { + num_entries: HashMap::new(), + entry_order: Vec::new(), + } } - #[cfg(any(test, feature = "ffi"))] pub(crate) fn insert(&mut self, name: HeaderName) { - if !self.0.contains_key(&name) { + if !self.num_entries.contains_key(&name) { let idx = 0; - self.0.insert(name.clone(), 1); - self.1.push((name, idx)); + self.num_entries.insert(name.clone(), 1); + self.entry_order.push((name, idx)); } - // replacing an already existing element does not + // Replacing an already existing element does not // change ordering, so we only care if its the first // header name encountered } @@ -151,20 +163,57 @@ impl OriginalHeaderOrder { { let name: HeaderName = name.into(); let idx; - if self.0.contains_key(&name) { - idx = self.0[&name]; - *self.0.get_mut(&name).unwrap() += 1; + if self.num_entries.contains_key(&name) { + idx = self.num_entries[&name]; + *self.num_entries.get_mut(&name).unwrap() += 1; } else { idx = 0; - self.0.insert(name.clone(), 1); + self.num_entries.insert(name.clone(), 1); } - self.1.push((name, idx)); + self.entry_order.push((name, idx)); } + // No doc test is run here because `RUSTFLAGS='--cfg hyper_unstable_ffi'` + // is needed to compile. Once ffi is stablized `no_run` should be removed + // here. /// This returns an iterator that provides header names and indexes /// in the original order recieved. - #[cfg(any(test, feature = "ffi"))] + /// + /// # Examples + /// ```no_run + /// use hyper::ext::OriginalHeaderOrder; + /// use hyper::header::{HeaderName, HeaderValue, HeaderMap}; + /// + /// let mut h_order = OriginalHeaderOrder::default(); + /// let mut h_map = Headermap::new(); + /// + /// let name1 = b"Set-CookiE"; + /// let value1 = b"a=b"; + /// h_map.append(name1); + /// h_order.append(name1); + /// + /// let name2 = b"Content-Encoding"; + /// let value2 = b"gzip"; + /// h_map.append(name2, value2); + /// h_order.append(name2); + /// + /// let name3 = b"SET-COOKIE"; + /// let value3 = b"c=d"; + /// h_map.append(name3, value3); + /// h_order.append(name3) + /// + /// let mut iter = h_order.get_in_order() + /// + /// let (name, idx) = iter.next(); + /// assert_eq!(b"a=b", h_map.get_all(name).nth(idx).unwrap()); + /// + /// let (name, idx) = iter.next(); + /// assert_eq!(b"gzip", h_map.get_all(name).nth(idx).unwrap()); + /// + /// let (name, idx) = iter.next(); + /// assert_eq!(b"c=d", h_map.get_all(name).nth(idx).unwrap()); + /// ``` pub(crate) fn get_in_order(&self) -> impl Iterator { - self.1.iter() + self.entry_order.iter() } } diff --git a/src/ffi/client.rs b/src/ffi/client.rs index 76da0a6e05..012a0da836 100644 --- a/src/ffi/client.rs +++ b/src/ffi/client.rs @@ -94,8 +94,6 @@ 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.http1_preserve_header_case(true); - builder.http1_preserve_header_order(true); Box::into_raw(Box::new(hyper_clientconn_options { builder, diff --git a/src/ffi/http_types.rs b/src/ffi/http_types.rs index d6d347961c..868d58e72f 100644 --- a/src/ffi/http_types.rs +++ b/src/ffi/http_types.rs @@ -411,56 +411,53 @@ ffi_fn! { // and for each one, try to pair the originally cased name with the value. // // TODO: consider adding http::HeaderMap::entries() iterator - for name in headers.headers.keys() { - let mut names = headers.orig_casing.get_all(name); - - for value in headers.headers.get_all(name) { - let (name_ptr, name_len) = if let Some(orig_name) = names.next() { + let mut ordered_iter = headers.orig_order.get_in_order().peekable(); + if ordered_iter.peek().is_some() { + for (name, idx) in ordered_iter { + let (name_ptr, name_len) = if let Some(orig_name) = headers.orig_casing.get_all(name).nth(*idx) { (orig_name.as_ref().as_ptr(), orig_name.as_ref().len()) } else { ( - name.as_str().as_bytes().as_ptr(), - name.as_str().as_bytes().len(), + name.as_str().as_bytes().as_ptr(), + name.as_str().as_bytes().len(), ) }; - let val_ptr = value.as_bytes().as_ptr(); - let val_len = value.as_bytes().len(); + let val_ptr; + let val_len; + if let Some(value) = headers.headers.get_all(name).iter().nth(*idx) { + val_ptr = value.as_bytes().as_ptr(); + val_len = value.as_bytes().len(); + } else { + // Stop iterating, something has gone wrong. + return; + } if HYPER_ITER_CONTINUE != func(userdata, name_ptr, name_len, val_ptr, val_len) { return; } } - } - } -} - -ffi_fn! { - /// Iterates the headers in the order the were recieved, passing each name and value pair to the callback. - /// - /// The `userdata` pointer is also passed to the callback. - /// - /// The callback should return `HYPER_ITER_CONTINUE` to keep iterating, or - /// `HYPER_ITER_BREAK` to stop. - fn hyper_headers_foreach_ordered(headers: *const hyper_headers, func: hyper_headers_foreach_callback, userdata: *mut c_void) { - let headers = non_null!(&*headers ?= ()); - // For each header name/value pair, there may be a value in the casemap - // that corresponds to the HeaderValue. So, we iterator all the keys, - // and for each one, try to pair the originally cased name with the value. - // - // TODO: consider adding http::HeaderMap::entries() iterator - for (name, idx) in headers.orig_order.get_in_order() { - let orig_name = headers.orig_casing.get_all(&name).nth(*idx).unwrap(); - let value = headers.headers.get_all(name).iter().nth(*idx).unwrap(); - - let name_ptr = orig_name.as_ref().as_ptr(); - let name_len = orig_name.as_ref().len(); - - let val_ptr = value.as_bytes().as_ptr(); - let val_len = value.as_bytes().len(); - - if HYPER_ITER_CONTINUE != func(userdata, name_ptr, name_len, val_ptr, val_len) { - return; + } else { + for name in headers.headers.keys() { + let mut names = headers.orig_casing.get_all(name); + + for value in headers.headers.get_all(name) { + let (name_ptr, name_len) = if let Some(orig_name) = names.next() { + (orig_name.as_ref().as_ptr(), orig_name.as_ref().len()) + } else { + ( + name.as_str().as_bytes().as_ptr(), + name.as_str().as_bytes().len(), + ) + }; + + let val_ptr = value.as_bytes().as_ptr(); + let val_len = value.as_bytes().len(); + + if HYPER_ITER_CONTINUE != func(userdata, name_ptr, name_len, val_ptr, val_len) { + return; + } + } } } } @@ -633,10 +630,13 @@ mod tests { ); let mut vec = Vec::::new(); - hyper_headers_foreach_ordered(&headers, concat, &mut vec as *mut _ as *mut c_void); + hyper_headers_foreach(&headers, concat, &mut vec as *mut _ as *mut c_void); println!("{}", std::str::from_utf8(&vec).unwrap()); - assert_eq!(vec, b"Set-CookiE: a=b\r\nContent-Encoding: gzip\r\nSET-COOKIE: c=d\r\n"); + assert_eq!( + vec, + b"Set-CookiE: a=b\r\nContent-Encoding: gzip\r\nSET-COOKIE: c=d\r\n" + ); extern "C" fn concat( vec: *mut c_void, From fcc98518f62fdc73987367bdd57fb8b75ead2254 Mon Sep 17 00:00:00 2001 From: Liam Warfield Date: Fri, 15 Apr 2022 18:44:25 -0600 Subject: [PATCH 3/3] missed a few #[cfg(feature = "ffi")] This cased the CI to fail --- src/client/conn.rs | 3 +++ src/ext.rs | 2 ++ src/ffi/client.rs | 2 +- src/proto/h1/role.rs | 6 +++++- 4 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/client/conn.rs b/src/client/conn.rs index 91a8551705..6746f441e7 100644 --- a/src/client/conn.rs +++ b/src/client/conn.rs @@ -560,6 +560,7 @@ impl Builder { h1_parser_config: Default::default(), h1_title_case_headers: false, h1_preserve_header_case: false, + #[cfg(feature = "ffi")] h1_preserve_header_order: false, h1_max_buf_size: None, #[cfg(feature = "ffi")] @@ -966,9 +967,11 @@ impl Builder { if opts.h1_title_case_headers { conn.set_title_case_headers(); } + #[cfg(feature = "ffi")] if opts.h1_preserve_header_case { conn.set_preserve_header_case(); } + #[cfg(feature = "ffi")] if opts.h1_preserve_header_order { conn.set_preserve_header_order(); } diff --git a/src/ext.rs b/src/ext.rs index 59a01ca964..863e83f682 100644 --- a/src/ext.rs +++ b/src/ext.rs @@ -5,6 +5,7 @@ use http::header::HeaderName; #[cfg(feature = "http1")] use http::header::{IntoHeaderName, ValueIter}; use http::HeaderMap; +#[cfg(feature = "ffi")] use std::collections::HashMap; #[cfg(feature = "http2")] use std::fmt; @@ -123,6 +124,7 @@ impl HeaderCaseMap { } } +#[cfg(feature = "ffi")] #[derive(Clone, Debug)] /// Hashmap pub(crate) struct OriginalHeaderOrder { diff --git a/src/ffi/client.rs b/src/ffi/client.rs index 012a0da836..4cdb257e30 100644 --- a/src/ffi/client.rs +++ b/src/ffi/client.rs @@ -93,7 +93,7 @@ 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(); + let builder = conn::Builder::new(); Box::into_raw(Box::new(hyper_clientconn_options { builder, diff --git a/src/proto/h1/role.rs b/src/proto/h1/role.rs index 4c933f42ed..b2e885a217 100644 --- a/src/proto/h1/role.rs +++ b/src/proto/h1/role.rs @@ -16,7 +16,9 @@ use crate::body::DecodedLength; #[cfg(feature = "server")] use crate::common::date; use crate::error::Parse; -use crate::ext::{HeaderCaseMap, OriginalHeaderOrder}; +use crate::ext::HeaderCaseMap; +#[cfg(feature = "ffi")] +use crate::ext::OriginalHeaderOrder; use crate::headers; use crate::proto::h1::{ Encode, Encoder, Http1Transaction, ParseContext, ParseResult, ParsedMessage, @@ -316,6 +318,7 @@ impl Http1Transaction for Server { extensions.insert(header_case_map); } + #[cfg(feature = "ffi")] if let Some(header_order) = header_order { extensions.insert(header_order); } @@ -1043,6 +1046,7 @@ impl Http1Transaction for Client { extensions.insert(header_case_map); } + #[cfg(feature = "ffi")] if let Some(header_order) = header_order { extensions.insert(header_order); }