-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add Request::into_http
#11843
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add Request::into_http
#11843
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this; LGTM.
Please run cargo fmt
when you have a chance, which should address the CI failure.
Would it also be possible to update the |
No problem! Seems a fair exchange for the past/future questions I'll have about
I looked into this, but I think I might not quite understand the ask as the Edit: Ah, guessing you meant this code which eventually calls |
Ah yes indeed! That eventually bottoms out here and I think yeah you're right that what you've added here is more suitable for replacing/augmenting the implementation of |
@alexcrichton So I looked into it a bit, and given my limited understanding of Might need a bit of hand holding here 😅 |
Ah ok no worries, I can try to take a look after this merges |
Looks like it leaked some memory during testing as well? 😞 =================================================================
==18584==ERROR: LeakSanitizer: detected memory leaks
Indirect leak of 72 byte(s) in 1 object(s) allocated from:
#0 0x563417655224 (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x14cb224) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#1 0x563417756761 (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x15cc761) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#2 0x5634176fc8f9 (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x15728f9) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#3 0x5634177a923d (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x161f23d) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#4 0x5634176ae4da (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x15244da) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#5 0x56341771965c (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x158f65c) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#6 0x56341775d61d (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x15d361d) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#7 0x5634176ad96d (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x152396d) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#8 0x5634177a894d (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x161e94d) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#9 0x5634177c1c98 (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x1637c98) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#10 0x5634177ffef4 (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x1675ef4) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#11 0x5634177d68e3 (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x164c8e3) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#12 0x5634177da219 (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x1650219) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#13 0x563419c47c6e (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x3abdc6e) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#14 0x563417652b96 (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x14c8b96) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
Indirect leak of 32 byte(s) in 1 object(s) allocated from:
#0 0x563417655224 (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x14cb224) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#1 0x5634176fdb34 (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x1573b34) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#2 0x5634177a95f7 (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x161f5f7) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#3 0x5634176ae4da (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x15244da) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#4 0x56341771965c (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x158f65c) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#5 0x56341775d61d (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x15d361d) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#6 0x5634176ad96d (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x152396d) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#7 0x5634177a894d (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x161e94d) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#8 0x5634177c1c98 (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x1637c98) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#9 0x5634177ffef4 (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x1675ef4) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#10 0x5634177d68e3 (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x164c8e3) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#11 0x5634177da219 (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x1650219) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#12 0x563419c47c6e (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x3abdc6e) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
#13 0x563417652b96 (/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521+0x14c8b96) (BuildId: ebb66c38ee1e4fe09240bde1dc91b9090cdf87d4)
SUMMARY: AddressSanitizer: 104 byte(s) leaked in 2 allocation(s).
error: test failed, to rerun pass `-p wasmtime-wasi-http --lib`
Caused by:
process didn't exit successfully: `/home/runner/work/wasmtime/wasmtime/target/x86_64-unknown-linux-gnu/debug/deps/wasmtime_wasi_http-e2c9f65d23950521` (exit status: 1)
note: test exited abnormally; to see the full output pass --no-capture to the harness.
Error: Process completed with exit code 1. |
As for the leak, it looks like that's caused by the test added here so I don't think it's spurious. The error message isn't great, however, but if you try running locally it might provide a better error message when |
Thanks for the detailed review and guidance @rvolosatovs! I'll incorporate the changes when I get a chance. |
let scheme = match scheme { | ||
None => ctx.default_scheme().ok_or(ErrorCode::HttpProtocolError)?, | ||
Some(scheme) if ctx.is_supported_scheme(&scheme) => scheme, | ||
Some(..) => return Err(ErrorCode::HttpProtocolError.into()), | ||
}; | ||
let mut uri = Uri::builder().scheme(scheme); | ||
if let Some(authority) = authority { | ||
uri = uri.authority(authority) | ||
}; | ||
if let Some(path_with_query) = path_with_query { | ||
uri = uri.path_and_query(path_with_query) | ||
}; | ||
let uri = uri.build().map_err(|err| { | ||
debug!(?err, "failed to build request URI"); | ||
ErrorCode::HttpRequestUriInvalid | ||
})?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these error cases could leak, since the body handles may not be disposed of correctly, in handle
implementation we prevent that by careful ordering of operations - first converting the body (ensuring it will be correctly disposed of on Drop
) and then constructing the request
wasmtime/crates/wasi-http/src/p3/host/handler.rs
Lines 219 to 299 in 6b156f2
let content_length = match get_content_length(&headers) { | |
Ok(content_length) => content_length, | |
Err(err) => { | |
body.drop(&mut store); | |
return Err(ErrorCode::InternalError(Some(format!("{err:#}"))).into()); | |
} | |
}; | |
let mut headers = Arc::unwrap_or_clone(headers); | |
let body = match body { | |
Body::Guest { | |
contents_rx, | |
trailers_rx, | |
result_tx, | |
} => GuestBody::new( | |
&mut store, | |
contents_rx, | |
trailers_rx, | |
result_tx, | |
io_task_result(io_result_rx), | |
content_length, | |
ErrorCode::HttpRequestBodySize, | |
getter, | |
) | |
.with_state(io_task_rx) | |
.boxed(), | |
Body::Host { body, result_tx } => { | |
if let Some(limit) = content_length { | |
let (http_result_tx, http_result_rx) = oneshot::channel(); | |
_ = result_tx.send(Box::new(async move { | |
if let Ok(err) = http_result_rx.await { | |
return Err(err); | |
}; | |
io_task_result(io_result_rx).await | |
})); | |
body.with_content_length( | |
limit, | |
http_result_tx, | |
ErrorCode::HttpRequestBodySize, | |
) | |
.with_state(io_task_rx) | |
.boxed() | |
} else { | |
_ = result_tx.send(Box::new(io_task_result(io_result_rx))); | |
body.with_state(io_task_rx).boxed() | |
} | |
} | |
}; | |
let WasiHttpCtxView { ctx, .. } = store.get(); | |
if ctx.set_host_header() { | |
let host = if let Some(authority) = authority.as_ref() { | |
HeaderValue::try_from(authority.as_str()) | |
.map_err(|err| ErrorCode::InternalError(Some(err.to_string())))? | |
} else { | |
HeaderValue::from_static("") | |
}; | |
headers.insert(HOST, host); | |
} | |
let scheme = match scheme { | |
None => ctx.default_scheme().ok_or(ErrorCode::HttpProtocolError)?, | |
Some(scheme) if ctx.is_supported_scheme(&scheme) => scheme, | |
Some(..) => return Err(ErrorCode::HttpProtocolError.into()), | |
}; | |
let mut uri = Uri::builder().scheme(scheme); | |
if let Some(authority) = authority { | |
uri = uri.authority(authority) | |
}; | |
if let Some(path_with_query) = path_with_query { | |
uri = uri.path_and_query(path_with_query) | |
}; | |
let uri = uri.build().map_err(|err| { | |
debug!(?err, "failed to build request URI"); | |
ErrorCode::HttpRequestUriInvalid | |
})?; | |
let mut req = http::Request::builder(); | |
*req.headers_mut().unwrap() = headers; | |
let req = req | |
.method(method) | |
.uri(uri) | |
.body(body) | |
.map_err(|err| ErrorCode::InternalError(Some(err.to_string())))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refs #11440 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the context (and direct link to the PR discussion!), that makes sense, I'll revert the changes in ordering.
crates/wasi-http/src/p3/request.rs
Outdated
let result = Box::pin(fut).as_mut().poll(&mut cx); | ||
// `fut` returns Ok(()), as the error in `into_http` currently drops the body before anything is sent | ||
// is this desired behavior? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, into_http
fails before it converts the body, which causes this - that makes sense to me, but I think converting the body should be one of the very first things the function should do, which would maybe fix the issue? (see also comments above as to why the order matters)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: Ignore given my misunderstanding of the expectations around the use of Request::new
First, some context:
So (apologies if this explanation is unnecessary) this is due to the one section that did mirror what HostWithStore::handle<T>
currently does, which returns an error if the content length is invalid first (which is used to determine whether to produce a Limited
/UnlimitedGuestBodyConsumer
), which is then matched on and used to explicitly drop the body:
wasmtime/crates/wasi-http/src/p3/host/handler.rs
Lines 219 to 225 in 278656c
let content_length = match get_content_length(&headers) { | |
Ok(content_length) => content_length, | |
Err(err) => { | |
body.drop(&mut store); | |
return Err(ErrorCode::InternalError(Some(format!("{err:#}"))).into()); | |
} | |
}; |
This then drops the sender side of the channel, which results in the else
clause here being evaluated (and fut
returning Ok(())
:
let Ok(fut) = rx.await else { return Ok(()) }; |
Then, the question:
Is this how we want things to be handled? A valid content length header is not strictly necessary (so an error could result in setting content_length = None
, and the rest of the code could happily proceed), but it does also make sense that creating a Request
with an invalid Content-Length
header would result in an error, but if so I think I would expect to learn of that error in fut
...that is, if there weren't also this docstring on Request::new
:
/// Requests constructed this way will not perform any `Content-Length` validation. |
crates/wasi-http/src/p3/request.rs
Outdated
let mut headers = HeaderMap::new(); | ||
headers.insert( | ||
http::header::CONTENT_LENGTH, | ||
HeaderValue::from_static("invalid"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is valid. The Request::new
assumes that the host has performed the validation if the request is of foreign origin. For example, if request was received using hyper
, it would have to be a valid HTTP request (e.g. with compliant content-length
)
This is not a valid content-length
header value though, so Request::new
should not be called with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, changed the error scenario to something which should throw on building the URI instead.
Discussion: #wasi > wasi-http Request and pub(crate) Body @ 💬
Largely copies the implementation from
Response::into_http
as suggested, contains one basic test asserting it works in the happy path.