From 952c946bfbfdc372843eb15df3629233fe5d8bf2 Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Thu, 9 Nov 2023 17:21:14 -0800 Subject: [PATCH 1/3] Make child fields immutable --- .../http_outbound_request_invalid_header.rs | 20 +++++++- crates/wasi-http/src/types.rs | 15 ++++++ crates/wasi-http/src/types_impl.rs | 46 +++++++++++++------ crates/wasi-http/wit/deps/http/types.wit | 1 + crates/wasi/wit/deps/http/types.wit | 1 + 5 files changed, 67 insertions(+), 16 deletions(-) diff --git a/crates/test-programs/src/bin/http_outbound_request_invalid_header.rs b/crates/test-programs/src/bin/http_outbound_request_invalid_header.rs index 09f1732983ba..65931dd6115e 100644 --- a/crates/test-programs/src/bin/http_outbound_request_invalid_header.rs +++ b/crates/test-programs/src/bin/http_outbound_request_invalid_header.rs @@ -1,4 +1,4 @@ -use test_programs::wasi::http::types::{HeaderError, Headers}; +use test_programs::wasi::http::types::{HeaderError, Headers, OutgoingRequest}; fn main() { let hdrs = Headers::new(); @@ -57,4 +57,22 @@ fn main() { Headers::from_list(&[("ok-header-name".to_owned(), b"bad\nvalue".to_vec())]), Err(HeaderError::InvalidSyntax) )); + + let req = OutgoingRequest::new(hdrs); + let hdrs = req.headers(); + + assert!(matches!( + hdrs.set(&"Content-Length".to_owned(), &[b"10".to_vec()]), + Err(HeaderError::Immutable), + )); + + assert!(matches!( + hdrs.append(&"Content-Length".to_owned(), &b"10".to_vec()), + Err(HeaderError::Immutable), + )); + + assert!(matches!( + hdrs.delete(&"Content-Length".to_owned()), + Err(HeaderError::Immutable), + )); } diff --git a/crates/wasi-http/src/types.rs b/crates/wasi-http/src/types.rs index c7041fcad5c0..0fb413afe1c5 100644 --- a/crates/wasi-http/src/types.rs +++ b/crates/wasi-http/src/types.rs @@ -323,6 +323,18 @@ impl TryFrom for hyper::Response { } } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum FieldMapMutability { + Mutable, + Immutable, +} + +impl FieldMapMutability { + pub fn is_immutable(&self) -> bool { + *self == Self::Immutable + } +} + pub type FieldMap = hyper::HeaderMap; pub enum HostFields { @@ -333,6 +345,9 @@ pub enum HostFields { // always be registered as a child of the entry with the `parent` id. This ensures that the // entry will always exist while this `HostFields::Ref` entry exists in the table, thus we // don't need to account for failure when fetching the fields ref from the parent. + // + // NOTE: references are always considered immutable -- the only way to modify fields is to + // create an owned version first. get_fields: for<'a> fn(elem: &'a mut (dyn Any + 'static)) -> &'a mut FieldMap, }, Owned { diff --git a/crates/wasi-http/src/types_impl.rs b/crates/wasi-http/src/types_impl.rs index 89455e3aa04d..6e04c8191306 100644 --- a/crates/wasi-http/src/types_impl.rs +++ b/crates/wasi-http/src/types_impl.rs @@ -2,7 +2,7 @@ use crate::{ bindings::http::types::{self, Error, Headers, Method, Scheme, StatusCode, Trailers}, body::{FinishMessage, HostFutureTrailers, HostIncomingBody, HostOutgoingBody}, types::{ - FieldMap, HostFields, HostFutureIncomingResponse, HostIncomingRequest, + FieldMap, FieldMapMutability, HostFields, HostFutureIncomingResponse, HostIncomingRequest, HostIncomingResponse, HostOutgoingRequest, HostOutgoingResponse, HostResponseOutparam, }, WasiHttpView, @@ -20,30 +20,33 @@ use wasmtime_wasi::preview2::{ impl crate::bindings::http::types::Host for T {} /// Take ownership of the underlying [`FieldMap`] associated with this fields resource. If the -/// fields resource references another fields, the returned [`FieldMap`] will be cloned. +/// fields resource references another fields, the returned [`FieldMap`] will be cloned. We throw +/// away the `immutable` status of the original fields, as the new context will determine how the +/// [`FieldMap`] is used. fn move_fields(table: &mut Table, id: Resource) -> wasmtime::Result { match table.delete(id)? { HostFields::Ref { parent, get_fields } => { let entry = table.get_any_mut(parent)?; - Ok(get_fields(entry).clone()) + let fields = get_fields(entry); + Ok(fields.clone()) } - HostFields::Owned { fields } => Ok(fields), + HostFields::Owned { fields, .. } => Ok(fields), } } fn get_fields_mut<'a>( table: &'a mut Table, id: &Resource, -) -> wasmtime::Result<&'a mut FieldMap> { +) -> wasmtime::Result<(FieldMapMutability, &'a mut FieldMap)> { let fields = table.get(&id)?; if let HostFields::Ref { parent, get_fields } = *fields { let entry = table.get_any_mut(parent)?; - return Ok(get_fields(entry)); + return Ok((FieldMapMutability::Immutable, get_fields(entry))); } match table.get_mut(&id)? { - HostFields::Owned { fields } => Ok(fields), + HostFields::Owned { fields } => Ok((FieldMapMutability::Mutable, fields)), // NB: ideally the `if let` above would go here instead. That makes // the borrow-checker unhappy. Unclear why. If you, dear reader, can // refactor this to remove the `unreachable!` please do. @@ -83,7 +86,7 @@ impl crate::bindings::http::types::HostFields for T { &mut self, entries: Vec<(String, Vec)>, ) -> wasmtime::Result, types::HeaderError>> { - let mut map = hyper::HeaderMap::new(); + let mut fields = hyper::HeaderMap::new(); for (header, value) in entries { let header = match hyper::header::HeaderName::from_bytes(header.as_bytes()) { @@ -100,12 +103,12 @@ impl crate::bindings::http::types::HostFields for T { Err(_) => return Ok(Err(types::HeaderError::InvalidSyntax)), }; - map.append(header, value); + fields.append(header, value); } let id = self .table() - .push(HostFields::Owned { fields: map }) + .push(HostFields::Owned { fields }) .context("[new_fields] pushing fields")?; Ok(Ok(id)) @@ -130,6 +133,7 @@ impl crate::bindings::http::types::HostFields for T { let res = get_fields_mut(self.table(), &fields) .context("[fields_get] getting fields")? + .1 .get_all(header) .into_iter() .map(|val| val.as_bytes().to_owned()) @@ -160,8 +164,12 @@ impl crate::bindings::http::types::HostFields for T { } } - let m = + let (mutability, m) = get_fields_mut(self.table(), &fields).context("[fields_set] getting mutable fields")?; + if mutability.is_immutable() { + return Ok(Err(types::HeaderError::Immutable)); + } + m.remove(&header); for value in values { m.append(&header, value); @@ -184,7 +192,11 @@ impl crate::bindings::http::types::HostFields for T { return Ok(Err(types::HeaderError::Forbidden)); } - let m = get_fields_mut(self.table(), &fields)?; + let (mutability, m) = get_fields_mut(self.table(), &fields)?; + if mutability.is_immutable() { + return Ok(Err(types::HeaderError::Immutable)); + } + m.remove(header); Ok(Ok(())) } @@ -209,8 +221,11 @@ impl crate::bindings::http::types::HostFields for T { Err(_) => return Ok(Err(types::HeaderError::InvalidSyntax)), }; - let m = get_fields_mut(self.table(), &fields) + let (mutability, m) = get_fields_mut(self.table(), &fields) .context("[fields_append] getting mutable fields")?; + if mutability.is_immutable() { + return Ok(Err(types::HeaderError::Immutable)); + } m.append(header, value); Ok(Ok(())) @@ -220,7 +235,7 @@ impl crate::bindings::http::types::HostFields for T { &mut self, fields: Resource, ) -> wasmtime::Result)>> { - let fields = get_fields_mut(self.table(), &fields)?; + let (_, fields) = get_fields_mut(self.table(), &fields)?; let result = fields .iter() .map(|(name, value)| (name.as_str().to_owned(), value.as_bytes().to_owned())) @@ -231,6 +246,7 @@ impl crate::bindings::http::types::HostFields for T { fn clone(&mut self, fields: Resource) -> wasmtime::Result> { let fields = get_fields_mut(self.table(), &fields) .context("[fields_clone] getting fields")? + .1 .clone(); let id = self @@ -837,7 +853,7 @@ impl crate::bindings::http::types::HostOutgoingBody for T { .expect("outgoing-body trailer_sender consumed by a non-owning function"); let message = if let Some(ts) = ts { - FinishMessage::Trailers(get_fields_mut(self.table(), &ts)?.clone().into()) + FinishMessage::Trailers(move_fields(self.table(), ts)?) } else { FinishMessage::Finished }; diff --git a/crates/wasi-http/wit/deps/http/types.wit b/crates/wasi-http/wit/deps/http/types.wit index fac47f61e933..4662f3fd6ba8 100644 --- a/crates/wasi-http/wit/deps/http/types.wit +++ b/crates/wasi-http/wit/deps/http/types.wit @@ -42,6 +42,7 @@ interface types { variant header-error { invalid-syntax, forbidden, + immutable } /// Field keys are always strings. diff --git a/crates/wasi/wit/deps/http/types.wit b/crates/wasi/wit/deps/http/types.wit index fac47f61e933..4662f3fd6ba8 100644 --- a/crates/wasi/wit/deps/http/types.wit +++ b/crates/wasi/wit/deps/http/types.wit @@ -42,6 +42,7 @@ interface types { variant header-error { invalid-syntax, forbidden, + immutable } /// Field keys are always strings. From 60abe0ea195d35aaf5d24db8e4f960a2d28c8bf2 Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Fri, 10 Nov 2023 10:36:29 -0800 Subject: [PATCH 2/3] Add `get_fields` and remove `FieldMapMutability` Clean up the interface to immutable fields by adding a different accessor. --- crates/wasi-http/src/types.rs | 12 ----- crates/wasi-http/src/types_impl.rs | 78 +++++++++++++++--------------- 2 files changed, 38 insertions(+), 52 deletions(-) diff --git a/crates/wasi-http/src/types.rs b/crates/wasi-http/src/types.rs index 0fb413afe1c5..200227fd6990 100644 --- a/crates/wasi-http/src/types.rs +++ b/crates/wasi-http/src/types.rs @@ -323,18 +323,6 @@ impl TryFrom for hyper::Response { } } -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub enum FieldMapMutability { - Mutable, - Immutable, -} - -impl FieldMapMutability { - pub fn is_immutable(&self) -> bool { - *self == Self::Immutable - } -} - pub type FieldMap = hyper::HeaderMap; pub enum HostFields { diff --git a/crates/wasi-http/src/types_impl.rs b/crates/wasi-http/src/types_impl.rs index 6e04c8191306..93f3e94f05a1 100644 --- a/crates/wasi-http/src/types_impl.rs +++ b/crates/wasi-http/src/types_impl.rs @@ -2,7 +2,7 @@ use crate::{ bindings::http::types::{self, Error, Headers, Method, Scheme, StatusCode, Trailers}, body::{FinishMessage, HostFutureTrailers, HostIncomingBody, HostOutgoingBody}, types::{ - FieldMap, FieldMapMutability, HostFields, HostFutureIncomingResponse, HostIncomingRequest, + FieldMap, HostFields, HostFutureIncomingResponse, HostIncomingRequest, HostIncomingResponse, HostOutgoingRequest, HostOutgoingResponse, HostResponseOutparam, }, WasiHttpView, @@ -35,18 +35,18 @@ fn move_fields(table: &mut Table, id: Resource) -> wasmtime::Result< } } -fn get_fields_mut<'a>( +fn get_fields<'a>( table: &'a mut Table, id: &Resource, -) -> wasmtime::Result<(FieldMapMutability, &'a mut FieldMap)> { +) -> wasmtime::Result<&'a FieldMap> { let fields = table.get(&id)?; if let HostFields::Ref { parent, get_fields } = *fields { let entry = table.get_any_mut(parent)?; - return Ok((FieldMapMutability::Immutable, get_fields(entry))); + return Ok(get_fields(entry)); } match table.get_mut(&id)? { - HostFields::Owned { fields } => Ok((FieldMapMutability::Mutable, fields)), + HostFields::Owned { fields } => Ok(fields), // NB: ideally the `if let` above would go here instead. That makes // the borrow-checker unhappy. Unclear why. If you, dear reader, can // refactor this to remove the `unreachable!` please do. @@ -54,6 +54,16 @@ fn get_fields_mut<'a>( } } +fn get_fields_mut<'a>( + table: &'a mut Table, + id: &Resource, +) -> wasmtime::Result> { + match table.get_mut(&id)? { + HostFields::Owned { fields } => Ok(Ok(fields)), + HostFields::Ref { .. } => Ok(Err(types::HeaderError::Immutable)), + } +} + fn is_forbidden_header(view: &mut T, name: &HeaderName) -> bool { static FORBIDDEN_HEADERS: [HeaderName; 9] = [ hyper::header::CONNECTION, @@ -131,9 +141,8 @@ impl crate::bindings::http::types::HostFields for T { Err(_) => return Ok(vec![]), }; - let res = get_fields_mut(self.table(), &fields) + let res = get_fields(self.table(), &fields) .context("[fields_get] getting fields")? - .1 .get_all(header) .into_iter() .map(|val| val.as_bytes().to_owned()) @@ -164,18 +173,15 @@ impl crate::bindings::http::types::HostFields for T { } } - let (mutability, m) = - get_fields_mut(self.table(), &fields).context("[fields_set] getting mutable fields")?; - if mutability.is_immutable() { - return Ok(Err(types::HeaderError::Immutable)); - } - - m.remove(&header); - for value in values { - m.append(&header, value); - } - - Ok(Ok(())) + Ok(get_fields_mut(self.table(), &fields) + .context("[fields_set] getting mutable fields")? + .map(|fields| { + fields.remove(&header); + for value in values { + fields.append(&header, value); + } + () + })) } fn delete( @@ -192,13 +198,10 @@ impl crate::bindings::http::types::HostFields for T { return Ok(Err(types::HeaderError::Forbidden)); } - let (mutability, m) = get_fields_mut(self.table(), &fields)?; - if mutability.is_immutable() { - return Ok(Err(types::HeaderError::Immutable)); - } - - m.remove(header); - Ok(Ok(())) + Ok(get_fields_mut(self.table(), &fields)?.map(|fields| { + fields.remove(header); + () + })) } fn append( @@ -221,32 +224,27 @@ impl crate::bindings::http::types::HostFields for T { Err(_) => return Ok(Err(types::HeaderError::InvalidSyntax)), }; - let (mutability, m) = get_fields_mut(self.table(), &fields) - .context("[fields_append] getting mutable fields")?; - if mutability.is_immutable() { - return Ok(Err(types::HeaderError::Immutable)); - } - - m.append(header, value); - Ok(Ok(())) + Ok(get_fields_mut(self.table(), &fields) + .context("[fields_append] getting mutable fields")? + .map(|fields| { + fields.append(header, value); + () + })) } fn entries( &mut self, fields: Resource, ) -> wasmtime::Result)>> { - let (_, fields) = get_fields_mut(self.table(), &fields)?; - let result = fields + Ok(get_fields(self.table(), &fields)? .iter() .map(|(name, value)| (name.as_str().to_owned(), value.as_bytes().to_owned())) - .collect(); - Ok(result) + .collect()) } fn clone(&mut self, fields: Resource) -> wasmtime::Result> { - let fields = get_fields_mut(self.table(), &fields) + let fields = get_fields(self.table(), &fields) .context("[fields_clone] getting fields")? - .1 .clone(); let id = self From ede308fc6b40a9948182188c5aabfdfde680cc0b Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Fri, 10 Nov 2023 10:42:18 -0800 Subject: [PATCH 3/3] Clean up the diff --- crates/wasi-http/src/types.rs | 3 --- crates/wasi-http/src/types_impl.rs | 12 +++--------- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/crates/wasi-http/src/types.rs b/crates/wasi-http/src/types.rs index 200227fd6990..c7041fcad5c0 100644 --- a/crates/wasi-http/src/types.rs +++ b/crates/wasi-http/src/types.rs @@ -333,9 +333,6 @@ pub enum HostFields { // always be registered as a child of the entry with the `parent` id. This ensures that the // entry will always exist while this `HostFields::Ref` entry exists in the table, thus we // don't need to account for failure when fetching the fields ref from the parent. - // - // NOTE: references are always considered immutable -- the only way to modify fields is to - // create an owned version first. get_fields: for<'a> fn(elem: &'a mut (dyn Any + 'static)) -> &'a mut FieldMap, }, Owned { diff --git a/crates/wasi-http/src/types_impl.rs b/crates/wasi-http/src/types_impl.rs index 93f3e94f05a1..00400a07c860 100644 --- a/crates/wasi-http/src/types_impl.rs +++ b/crates/wasi-http/src/types_impl.rs @@ -20,18 +20,15 @@ use wasmtime_wasi::preview2::{ impl crate::bindings::http::types::Host for T {} /// Take ownership of the underlying [`FieldMap`] associated with this fields resource. If the -/// fields resource references another fields, the returned [`FieldMap`] will be cloned. We throw -/// away the `immutable` status of the original fields, as the new context will determine how the -/// [`FieldMap`] is used. +/// fields resource references another fields, the returned [`FieldMap`] will be cloned. fn move_fields(table: &mut Table, id: Resource) -> wasmtime::Result { match table.delete(id)? { HostFields::Ref { parent, get_fields } => { let entry = table.get_any_mut(parent)?; - let fields = get_fields(entry); - Ok(fields.clone()) + Ok(get_fields(entry).clone()) } - HostFields::Owned { fields, .. } => Ok(fields), + HostFields::Owned { fields } => Ok(fields), } } @@ -180,7 +177,6 @@ impl crate::bindings::http::types::HostFields for T { for value in values { fields.append(&header, value); } - () })) } @@ -200,7 +196,6 @@ impl crate::bindings::http::types::HostFields for T { Ok(get_fields_mut(self.table(), &fields)?.map(|fields| { fields.remove(header); - () })) } @@ -228,7 +223,6 @@ impl crate::bindings::http::types::HostFields for T { .context("[fields_append] getting mutable fields")? .map(|fields| { fields.append(header, value); - () })) }