From e656c4235348e85fb0ca0b605c51638b1e467843 Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Mon, 5 Nov 2018 10:20:09 -0800 Subject: [PATCH] fix inverted split for DATA frame padding (#330) --- src/frame/data.rs | 17 +++++++++ src/frame/util.rs | 7 ++-- tests/h2-support/src/frames.rs | 9 +++++ tests/h2-tests/tests/codec_read.rs | 8 ++-- tests/h2-tests/tests/flow_control.rs | 57 ++++++++++++++++++++++++++++ 5 files changed, 91 insertions(+), 7 deletions(-) diff --git a/src/frame/data.rs b/src/frame/data.rs index 79ae258..89a4e6e 100644 --- a/src/frame/data.rs +++ b/src/frame/data.rs @@ -63,6 +63,18 @@ impl Data { } } + /// Returns whther the `PADDED` flag is set on this frame. + #[cfg(feature = "unstable")] + pub fn is_padded(&self) -> bool { + self.flags.is_padded() + } + + /// Sets the value for the `PADDED` flag on this frame. + #[cfg(feature = "unstable")] + pub fn set_padded(&mut self) { + self.flags.set_padded(); + } + /// Returns a reference to this frame's payload. /// /// This does **not** include any padding that might have been originally @@ -192,6 +204,11 @@ impl DataFlags { fn is_padded(&self) -> bool { self.0 & PADDED == PADDED } + + #[cfg(feature = "unstable")] + fn set_padded(&mut self) { + self.0 |= PADDED + } } impl Default for DataFlags { diff --git a/src/frame/util.rs b/src/frame/util.rs index decc030..ceb44b2 100644 --- a/src/frame/util.rs +++ b/src/frame/util.rs @@ -15,7 +15,8 @@ use bytes::Bytes; /// If the padded payload is invalid (e.g. the length of the padding is equal /// to the total length), returns `None`. pub fn strip_padding(payload: &mut Bytes) -> Result { - if payload.len() == 0 { + let payload_len = payload.len(); + if payload_len == 0 { // If this is the case, the frame is invalid as no padding length can be // extracted, even though the frame should be padded. return Err(Error::TooMuchPadding); @@ -23,14 +24,14 @@ pub fn strip_padding(payload: &mut Bytes) -> Result { let pad_len = payload[0] as usize; - if pad_len >= payload.len() { + if pad_len >= payload_len { // This is invalid: the padding length MUST be less than the // total frame size. return Err(Error::TooMuchPadding); } let _ = payload.split_to(1); - let _ = payload.split_off(pad_len); + let _ = payload.split_off(payload_len - pad_len - 1); Ok(pad_len as u8) } diff --git a/tests/h2-support/src/frames.rs b/tests/h2-support/src/frames.rs index 36e890e..4a757f5 100644 --- a/tests/h2-support/src/frames.rs +++ b/tests/h2-support/src/frames.rs @@ -180,6 +180,11 @@ impl From> for SendFrame { // Data helpers impl Mock { + pub fn padded(mut self) -> Self { + self.0.set_padded(); + self + } + pub fn eos(mut self) -> Self { self.0.set_end_stream(true); self @@ -190,9 +195,13 @@ impl From> for SendFrame { fn from(src: Mock) -> Self { let id = src.0.stream_id(); let eos = src.0.is_end_stream(); + let is_padded = src.0.is_padded(); let payload = src.0.into_payload(); let mut frame = frame::Data::new(id, payload.into_buf()); frame.set_end_stream(eos); + if is_padded { + frame.set_padded(); + } Frame::Data(frame) } } diff --git a/tests/h2-tests/tests/codec_read.rs b/tests/h2-tests/tests/codec_read.rs index c060580..effb17a 100644 --- a/tests/h2-tests/tests/codec_read.rs +++ b/tests/h2-tests/tests/codec_read.rs @@ -72,16 +72,16 @@ fn read_data_end_stream() { fn read_data_padding() { let mut codec = raw_codec! { read => [ - 0, 0, 11, 0, 0x8, 0, 0, 0, 1, + 0, 0, 16, 0, 0x8, 0, 0, 0, 1, 5, // Pad length - "hello", // Data - "world", // Padding + "helloworld", // Data + "\0\0\0\0\0", // Padding ]; }; let data = poll_frame!(Data, codec); assert_eq!(data.stream_id(), 1); - assert_eq!(data.payload(), &b"hello"[..]); + assert_eq!(data.payload(), &b"helloworld"[..]); assert!(!data.is_end_stream()); assert_closed!(codec); diff --git a/tests/h2-tests/tests/flow_control.rs b/tests/h2-tests/tests/flow_control.rs index 9244bd7..63ce56f 100644 --- a/tests/h2-tests/tests/flow_control.rs +++ b/tests/h2-tests/tests/flow_control.rs @@ -1361,3 +1361,60 @@ fn reset_stream_waiting_for_capacity() { client.join(srv).wait().unwrap(); } +#[test] +fn data_padding() { + let _ = ::env_logger::try_init(); + let (io, srv) = mock::new(); + + let mut body = Vec::new(); + body.push(5); + body.extend_from_slice(&[b'z'; 100][..]); + body.extend_from_slice(&[b'0'; 5][..]); + + let srv = srv.assert_client_handshake() + .unwrap() + .recv_settings() + .recv_frame( + frames::headers(1) + .request("GET", "http://example.com/") + .eos() + ) + .send_frame( + frames::headers(1) + .response(200) + .field("content-length", 100) + ) + .send_frame( + frames::data(1, body) + .padded() + .eos() + ) + .close(); + + let h2 = client::handshake(io) + .expect("handshake") + .and_then(|(mut client, conn)| { + let request = Request::builder() + .method(Method::GET) + .uri("http://example.com/") + .body(()) + .unwrap(); + + // first request is allowed + let (response, _) = client.send_request(request, true).unwrap(); + let fut = response + .and_then(|resp| { + assert_eq!(resp.status(), StatusCode::OK); + let body = resp.into_body(); + body.concat2() + }) + .map(|bytes| { + assert_eq!(bytes.len(), 100); + }); + conn + .expect("client") + .join(fut.expect("response")) + }); + + h2.join(srv).wait().expect("wait"); +}