From f673dba7596552d7b8dd78784b17c05cb6478b3e Mon Sep 17 00:00:00 2001 From: Nikolay Kim Date: Sun, 4 Mar 2018 09:48:23 -0800 Subject: [PATCH] Fix handling of requests with an encoded body with a length > 8192 #93 --- CHANGES.md | 4 + Cargo.toml | 2 +- src/server/encoding.rs | 370 ++++++++++++++++++++++------------------- tests/test_client.rs | 26 +++ tests/test_server.rs | 161 +++++++++++++++--- 5 files changed, 369 insertions(+), 194 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index a1f2b6de9..163d136fb 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,9 @@ # Changes +## 0.4.4 (2018-03-xx) + +* Fix handling of requests with an encoded body with a length > 8192 #93 + ## 0.4.3 (2018-03-03) * Fix request body read bug diff --git a/Cargo.toml b/Cargo.toml index 2c035c5d9..eeca59366 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "actix-web" -version = "0.4.3" +version = "0.4.4" authors = ["Nikolay Kim "] description = "Actix web is a small, pragmatic, extremely fast, web framework for Rust." readme = "README.md" diff --git a/src/server/encoding.rs b/src/server/encoding.rs index 5e0ac7b0f..23f4aef7f 100644 --- a/src/server/encoding.rs +++ b/src/server/encoding.rs @@ -11,7 +11,7 @@ use flate2::Compression; use flate2::read::GzDecoder; use flate2::write::{GzEncoder, DeflateDecoder, DeflateEncoder}; use brotli2::write::{BrotliDecoder, BrotliEncoder}; -use bytes::{Bytes, BytesMut, BufMut, Writer}; +use bytes::{Bytes, BytesMut, BufMut}; use headers::ContentEncoding; use body::{Body, Binary}; @@ -128,177 +128,6 @@ impl PayloadWriter for PayloadType { } } -pub(crate) enum Decoder { - Deflate(Box>>), - Gzip(Option>>), - Br(Box>>), - Identity, -} - -// should go after write::GzDecoder get implemented -#[derive(Debug)] -pub(crate) struct Wrapper { - pub buf: BytesMut, - pub eof: bool, -} - -impl io::Read for Wrapper { - fn read(&mut self, buf: &mut [u8]) -> io::Result { - let len = cmp::min(buf.len(), self.buf.len()); - buf[..len].copy_from_slice(&self.buf[..len]); - self.buf.split_to(len); - if len == 0 { - if self.eof { - Ok(0) - } else { - Err(io::Error::new(io::ErrorKind::WouldBlock, "")) - } - } else { - Ok(len) - } - } -} - -impl io::Write for Wrapper { - fn write(&mut self, buf: &[u8]) -> io::Result { - self.buf.extend_from_slice(buf); - Ok(buf.len()) - } - fn flush(&mut self) -> io::Result<()> { - Ok(()) - } -} - -/// Payload stream with decompression support -pub(crate) struct PayloadStream { - decoder: Decoder, - dst: BytesMut, -} - -impl PayloadStream { - pub fn new(enc: ContentEncoding) -> PayloadStream { - let dec = match enc { - ContentEncoding::Br => Decoder::Br( - Box::new(BrotliDecoder::new(BytesMut::with_capacity(8192).writer()))), - ContentEncoding::Deflate => Decoder::Deflate( - Box::new(DeflateDecoder::new(BytesMut::with_capacity(8192).writer()))), - ContentEncoding::Gzip => Decoder::Gzip(None), - _ => Decoder::Identity, - }; - PayloadStream{ decoder: dec, dst: BytesMut::new() } - } -} - -impl PayloadStream { - - pub fn feed_eof(&mut self) -> io::Result> { - match self.decoder { - Decoder::Br(ref mut decoder) => { - match decoder.finish() { - Ok(mut writer) => { - let b = writer.get_mut().take().freeze(); - if !b.is_empty() { - Ok(Some(b)) - } else { - Ok(None) - } - }, - Err(err) => Err(err), - } - }, - Decoder::Gzip(ref mut decoder) => { - if let Some(ref mut decoder) = *decoder { - decoder.as_mut().get_mut().eof = true; - - loop { - self.dst.reserve(8192); - match decoder.read(unsafe{self.dst.bytes_mut()}) { - Ok(n) => { - if n == 0 { - return Ok(Some(self.dst.take().freeze())) - } else { - unsafe{self.dst.set_len(n)}; - } - } - Err(err) => return Err(err), - } - } - } else { - Ok(None) - } - }, - Decoder::Deflate(ref mut decoder) => { - match decoder.try_finish() { - Ok(_) => { - let b = decoder.get_mut().get_mut().take().freeze(); - if !b.is_empty() { - Ok(Some(b)) - } else { - Ok(None) - } - }, - Err(err) => Err(err), - } - }, - Decoder::Identity => Ok(None), - } - } - - pub fn feed_data(&mut self, data: Bytes) -> io::Result> { - match self.decoder { - Decoder::Br(ref mut decoder) => { - match decoder.write(&data).and_then(|_| decoder.flush()) { - Ok(_) => { - let b = decoder.get_mut().get_mut().take().freeze(); - if !b.is_empty() { - Ok(Some(b)) - } else { - Ok(None) - } - }, - Err(err) => Err(err) - } - }, - Decoder::Gzip(ref mut decoder) => { - if decoder.is_none() { - *decoder = Some( - Box::new(GzDecoder::new( - Wrapper{buf: BytesMut::from(data), eof: false}))); - } else { - let _ = decoder.as_mut().unwrap().write(&data); - } - - loop { - self.dst.reserve(8192); - match decoder.as_mut().as_mut().unwrap().read(unsafe{self.dst.bytes_mut()}) { - Ok(n) => { - if n == 0 { - return Ok(Some(self.dst.split_to(n).freeze())); - } else { - unsafe{self.dst.set_len(n)}; - } - } - Err(e) => return Err(e), - } - } - }, - Decoder::Deflate(ref mut decoder) => { - match decoder.write(&data).and_then(|_| decoder.flush()) { - Ok(_) => { - let b = decoder.get_mut().get_mut().take().freeze(); - if !b.is_empty() { - Ok(Some(b)) - } else { - Ok(None) - } - }, - Err(e) => Err(e), - } - }, - Decoder::Identity => Ok(Some(data)), - } - } -} /// Payload wrapper with content decompression support pub(crate) struct EncodedPayload { @@ -357,6 +186,203 @@ impl PayloadWriter for EncodedPayload { } } +pub(crate) enum Decoder { + Deflate(Box>), + Gzip(Option>>), + Br(Box>), + Identity, +} + +// should go after write::GzDecoder get implemented +#[derive(Debug)] +pub(crate) struct Wrapper { + pub buf: BytesMut, + pub eof: bool, +} + +impl io::Read for Wrapper { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + let len = cmp::min(buf.len(), self.buf.len()); + buf[..len].copy_from_slice(&self.buf[..len]); + self.buf.split_to(len); + if len == 0 { + if self.eof { + Ok(0) + } else { + Err(io::Error::new(io::ErrorKind::WouldBlock, "")) + } + } else { + Ok(len) + } + } +} + +impl io::Write for Wrapper { + fn write(&mut self, buf: &[u8]) -> io::Result { + self.buf.extend_from_slice(buf); + Ok(buf.len()) + } + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } +} + +pub(crate) struct Writer { + buf: BytesMut, +} + +impl Writer { + fn new() -> Writer { + Writer{buf: BytesMut::with_capacity(8192)} + } + fn take(&mut self) -> Bytes { + self.buf.take().freeze() + } +} + +impl io::Write for Writer { + fn write(&mut self, buf: &[u8]) -> io::Result { + self.buf.extend_from_slice(buf); + Ok(buf.len()) + } + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } +} + +/// Payload stream with decompression support +pub(crate) struct PayloadStream { + decoder: Decoder, + dst: BytesMut, +} + +impl PayloadStream { + pub fn new(enc: ContentEncoding) -> PayloadStream { + let dec = match enc { + ContentEncoding::Br => Decoder::Br( + Box::new(BrotliDecoder::new(Writer::new()))), + ContentEncoding::Deflate => Decoder::Deflate( + Box::new(DeflateDecoder::new(Writer::new()))), + ContentEncoding::Gzip => Decoder::Gzip(None), + _ => Decoder::Identity, + }; + PayloadStream{ decoder: dec, dst: BytesMut::new() } + } +} + +impl PayloadStream { + + pub fn feed_eof(&mut self) -> io::Result> { + match self.decoder { + Decoder::Br(ref mut decoder) => { + match decoder.finish() { + Ok(mut writer) => { + let b = writer.take(); + if !b.is_empty() { + Ok(Some(b)) + } else { + Ok(None) + } + }, + Err(e) => Err(e), + } + }, + Decoder::Gzip(ref mut decoder) => { + if let Some(ref mut decoder) = *decoder { + decoder.as_mut().get_mut().eof = true; + + loop { + self.dst.reserve(8192); + match decoder.read(unsafe{self.dst.bytes_mut()}) { + Ok(n) => { + if n == 0 { + return Ok(Some(self.dst.take().freeze())) + } else { + unsafe{self.dst.advance_mut(n)}; + } + } + Err(e) => return Err(e), + } + } + } else { + Ok(None) + } + }, + Decoder::Deflate(ref mut decoder) => { + match decoder.try_finish() { + Ok(_) => { + let b = decoder.get_mut().take(); + if !b.is_empty() { + Ok(Some(b)) + } else { + Ok(None) + } + }, + Err(e) => Err(e), + } + }, + Decoder::Identity => Ok(None), + } + } + + pub fn feed_data(&mut self, data: Bytes) -> io::Result> { + match self.decoder { + Decoder::Br(ref mut decoder) => { + match decoder.write(&data).and_then(|_| decoder.flush()) { + Ok(_) => { + let b = decoder.get_mut().take(); + if !b.is_empty() { + Ok(Some(b)) + } else { + Ok(None) + } + }, + Err(e) => Err(e) + } + }, + Decoder::Gzip(ref mut decoder) => { + if decoder.is_none() { + *decoder = Some( + Box::new(GzDecoder::new( + Wrapper{buf: BytesMut::from(data), eof: false}))); + } else { + let _ = decoder.as_mut().unwrap().write(&data); + } + + loop { + self.dst.reserve(8192); + match decoder.as_mut().as_mut().unwrap().read(unsafe{self.dst.bytes_mut()}) { + Ok(n) => { + if n == 0 { + return Ok(Some(self.dst.take().freeze())); + } else { + unsafe{self.dst.advance_mut(n)}; + } + } + Err(e) => { + return Err(e) + } + } + } + }, + Decoder::Deflate(ref mut decoder) => { + match decoder.write(&data).and_then(|_| decoder.flush()) { + Ok(_) => { + let b = decoder.get_mut().take(); + if !b.is_empty() { + Ok(Some(b)) + } else { + Ok(None) + } + }, + Err(e) => Err(e), + } + }, + Decoder::Identity => Ok(Some(data)), + } + } +} + pub(crate) enum ContentEncoder { Deflate(DeflateEncoder), Gzip(GzEncoder), diff --git a/tests/test_client.rs b/tests/test_client.rs index cac1ab78e..b29bc522b 100644 --- a/tests/test_client.rs +++ b/tests/test_client.rs @@ -116,6 +116,32 @@ fn test_client_gzip_encoding() { assert_eq!(bytes, Bytes::from_static(STR.as_ref())); } +#[test] +fn test_client_gzip_encoding_large() { + let data = STR.to_owned() + STR + STR + STR + STR + STR + STR + STR + STR + STR; + + let mut srv = test::TestServer::new(|app| app.handler(|req: HttpRequest| { + req.body() + .and_then(|bytes: Bytes| { + Ok(httpcodes::HTTPOk + .build() + .content_encoding(headers::ContentEncoding::Deflate) + .body(bytes)) + }).responder()} + )); + + // client request + let request = srv.post() + .content_encoding(headers::ContentEncoding::Gzip) + .body(data.clone()).unwrap(); + let response = srv.execute(request.send()).unwrap(); + assert!(response.status().is_success()); + + // read response + let bytes = srv.execute(response.body()).unwrap(); + assert_eq!(bytes, Bytes::from(data)); +} + #[test] fn test_client_brotli_encoding() { let mut srv = test::TestServer::new(|app| app.handler(|req: HttpRequest| { diff --git a/tests/test_server.rs b/tests/test_server.rs index 44d0316d1..53d7ea49f 100644 --- a/tests/test_server.rs +++ b/tests/test_server.rs @@ -136,27 +136,32 @@ fn test_simple() { #[test] fn test_headers() { + let data = STR.to_owned() + STR + STR + STR + STR + STR + STR + STR + STR + STR; + let srv_data = Arc::new(data.clone()); let mut srv = test::TestServer::new( - |app| app.handler(|_| { - let mut builder = httpcodes::HTTPOk.build(); - for idx in 0..90 { - builder.header( - format!("X-TEST-{}", idx).as_str(), - "TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ - TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ - TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ - TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ - TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ - TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ - TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ - TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ - TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ - TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ - TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ - TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ - TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST "); - } - builder.body(STR)})); + move |app| { + let data = srv_data.clone(); + app.handler(move |_| { + let mut builder = httpcodes::HTTPOk.build(); + for idx in 0..90 { + builder.header( + format!("X-TEST-{}", idx).as_str(), + "TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ + TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ + TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ + TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ + TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ + TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ + TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ + TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ + TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ + TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ + TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ + TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST \ + TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST "); + } + builder.body(data.as_ref())}) + }); let request = srv.get().finish().unwrap(); let response = srv.execute(request.send()).unwrap(); @@ -164,7 +169,7 @@ fn test_headers() { // read response let bytes = srv.execute(response.body()).unwrap(); - assert_eq!(bytes, Bytes::from_static(STR.as_ref())); + assert_eq!(bytes, Bytes::from(data)); } #[test] @@ -203,6 +208,33 @@ fn test_body_gzip() { assert_eq!(Bytes::from(dec), Bytes::from_static(STR.as_ref())); } +#[test] +fn test_body_gzip_large() { + let data = STR.to_owned() + STR + STR + STR + STR + STR + STR + STR + STR + STR; + let srv_data = Arc::new(data.clone()); + + let mut srv = test::TestServer::new( + move |app| { + let data = srv_data.clone(); + app.handler( + move |_| httpcodes::HTTPOk.build() + .content_encoding(headers::ContentEncoding::Gzip) + .body(data.as_ref()))}); + + let request = srv.get().disable_decompress().finish().unwrap(); + let response = srv.execute(request.send()).unwrap(); + assert!(response.status().is_success()); + + // read response + let bytes = srv.execute(response.body()).unwrap(); + + // decode + let mut e = GzDecoder::new(&bytes[..]); + let mut dec = Vec::new(); + e.read_to_end(&mut dec).unwrap(); + assert_eq!(Bytes::from(dec), Bytes::from(data)); +} + #[test] fn test_body_chunked_implicit() { let mut srv = test::TestServer::new( @@ -430,6 +462,35 @@ fn test_gzip_encoding() { assert_eq!(bytes, Bytes::from_static(STR.as_ref())); } +#[test] +fn test_gzip_encoding_large() { + let data = STR.to_owned() + STR + STR + STR + STR + STR + STR + STR + STR + STR; + let mut srv = test::TestServer::new(|app| app.handler(|req: HttpRequest| { + req.body() + .and_then(|bytes: Bytes| { + Ok(httpcodes::HTTPOk + .build() + .content_encoding(headers::ContentEncoding::Identity) + .body(bytes)) + }).responder()} + )); + + // client request + let mut e = GzEncoder::new(Vec::new(), Compression::default()); + e.write_all(data.as_ref()).unwrap(); + let enc = e.finish().unwrap(); + + let request = srv.post() + .header(header::CONTENT_ENCODING, "gzip") + .body(enc.clone()).unwrap(); + let response = srv.execute(request.send()).unwrap(); + assert!(response.status().is_success()); + + // read response + let bytes = srv.execute(response.body()).unwrap(); + assert_eq!(bytes, Bytes::from(data)); +} + #[test] fn test_deflate_encoding() { let mut srv = test::TestServer::new(|app| app.handler(|req: HttpRequest| { @@ -458,6 +519,35 @@ fn test_deflate_encoding() { assert_eq!(bytes, Bytes::from_static(STR.as_ref())); } +#[test] +fn test_deflate_encoding_large() { + let data = STR.to_owned() + STR + STR + STR + STR + STR + STR + STR + STR + STR + STR; + let mut srv = test::TestServer::new(|app| app.handler(|req: HttpRequest| { + req.body() + .and_then(|bytes: Bytes| { + Ok(httpcodes::HTTPOk + .build() + .content_encoding(headers::ContentEncoding::Identity) + .body(bytes)) + }).responder()} + )); + + let mut e = DeflateEncoder::new(Vec::new(), Compression::default()); + e.write_all(data.as_ref()).unwrap(); + let enc = e.finish().unwrap(); + + // client request + let request = srv.post() + .header(header::CONTENT_ENCODING, "deflate") + .body(enc).unwrap(); + let response = srv.execute(request.send()).unwrap(); + assert!(response.status().is_success()); + + // read response + let bytes = srv.execute(response.body()).unwrap(); + assert_eq!(bytes, Bytes::from(data)); +} + #[test] fn test_brotli_encoding() { let mut srv = test::TestServer::new(|app| app.handler(|req: HttpRequest| { @@ -486,6 +576,35 @@ fn test_brotli_encoding() { assert_eq!(bytes, Bytes::from_static(STR.as_ref())); } +#[test] +fn test_brotli_encoding_large() { + let data = STR.to_owned() + STR + STR + STR + STR + STR + STR + STR + STR + STR + STR; + let mut srv = test::TestServer::new(|app| app.handler(|req: HttpRequest| { + req.body() + .and_then(|bytes: Bytes| { + Ok(httpcodes::HTTPOk + .build() + .content_encoding(headers::ContentEncoding::Identity) + .body(bytes)) + }).responder()} + )); + + let mut e = BrotliEncoder::new(Vec::new(), 5); + e.write_all(data.as_ref()).unwrap(); + let enc = e.finish().unwrap(); + + // client request + let request = srv.post() + .header(header::CONTENT_ENCODING, "br") + .body(enc).unwrap(); + let response = srv.execute(request.send()).unwrap(); + assert!(response.status().is_success()); + + // read response + let bytes = srv.execute(response.body()).unwrap(); + assert_eq!(bytes, Bytes::from(data)); +} + #[test] fn test_h2() { let srv = test::TestServer::new(|app| app.handler(|_|{