Skip to content
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

Fix for invalid header panic corrected #695

Merged
merged 6 commits into from
Jun 22, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 15 additions & 11 deletions src/proto/streams/recv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,11 @@ impl Recv {
return Err(Error::library_reset(stream.id, Reason::PROTOCOL_ERROR).into());
}

if pseudo.status.is_some() && counts.peer().is_server() {
proto_err!(stream: "cannot use :status header for requests; stream={:?}", stream.id);
return Err(Error::library_reset(stream.id, Reason::PROTOCOL_ERROR).into());
}

if !pseudo.is_informational() {
let message = counts
.peer()
Expand All @@ -239,27 +244,26 @@ impl Recv {
.pending_recv
.push_back(&mut self.buffer, Event::Headers(message));
stream.notify_recv();
}

// Only servers can receive a headers frame that initiates the stream.
// This is verified in `Streams` before calling this function.
if counts.peer().is_server() {
self.pending_accept.push(stream);
// Only servers can receive a headers frame that initiates the stream.
// This is verified in `Streams` before calling this function.
if counts.peer().is_server() {
self.pending_accept.push(stream);
}
}

Ok(())
}

/// Called by the server to get the request
pub fn take_request(&mut self, stream: &mut store::Ptr) -> Result<Request<()>, proto::Error> {
///
/// TODO: Should this fn return `Result`?
pub fn take_request(&mut self, stream: &mut store::Ptr) -> Request<()> {
use super::peer::PollMessage::*;

match stream.pending_recv.pop_front(&mut self.buffer) {
Some(Event::Headers(Server(request))) => Ok(request),
_ => {
proto_err!(stream: "received invalid request; stream={:?}", stream.id);
Err(Error::library_reset(stream.id, Reason::PROTOCOL_ERROR))
}
Some(Event::Headers(Server(request))) => request,
_ => panic!(),
f0rki marked this conversation as resolved.
Show resolved Hide resolved
f0rki marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/proto/streams/streams.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1178,7 +1178,7 @@ impl<B> StreamRef<B> {
/// # Panics
///
/// This function panics if the request isn't present.
pub fn take_request(&self) -> Result<Request<()>, proto::Error> {
pub fn take_request(&self) -> Request<()> {
let mut me = self.opaque.inner.lock().unwrap();
let me = &mut *me;

Expand Down
17 changes: 5 additions & 12 deletions src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -425,20 +425,13 @@ where

if let Some(inner) = self.connection.next_incoming() {
tracing::trace!("received incoming");
match inner.take_request() {
Ok(req) => {
let (head, _) = req.into_parts();
let body = RecvStream::new(FlowControl::new(inner.clone_to_opaque()));
let (head, _) = inner.take_request().into_parts();
let body = RecvStream::new(FlowControl::new(inner.clone_to_opaque()));

let request = Request::from_parts(head, body);
let respond = SendResponse { inner };
let request = Request::from_parts(head, body);
let respond = SendResponse { inner };

return Poll::Ready(Some(Ok((request, respond))));
}
Err(e) => {
return Poll::Ready(Some(Err(e.into())));
}
}
return Poll::Ready(Some(Ok((request, respond))));
}

Poll::Pending
Expand Down
15 changes: 6 additions & 9 deletions tests/h2-tests/tests/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1380,29 +1380,26 @@ async fn reject_non_authority_target_on_connect_request() {
}

#[tokio::test]
async fn reject_response_headers_in_request() {
async fn reject_informational_status_header_in_request() {
h2_support::trace_init!();

let (io, mut client) = mock::new();

let client = async move {
let _ = client.assert_server_handshake().await;

client.send_frame(frames::headers(1).response(128)).await;
let status_code = 128;
assert!(StatusCode::from_u16(status_code).unwrap().is_informational());

// TODO: is CANCEL the right error code to expect here?
client.recv_frame(frames::reset(1).cancel()).await;
client.send_frame(frames::headers(1).response(status_code)).await;

client.recv_frame(frames::reset(1).protocol_error()).await;
};

let srv = async move {
let builder = server::Builder::new();
let mut srv = builder.handshake::<_, Bytes>(io).await.expect("handshake");

let res = srv.next().await;
tracing::warn!("{:?}", res);
assert!(res.is_some());
assert!(res.unwrap().is_err());

poll_fn(move |cx| srv.poll_closed(cx))
.await
.expect("server");
Expand Down