From 5f432851882c6f5e2dbf4efea464978bc4d88700 Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Fri, 13 Feb 2015 18:35:40 -0800 Subject: [PATCH] fix(server): Drain requests on drop. If a client sent an illegal request (like a GET request with a message body), or if there was a legal request with a body but the Handler didn't read all of it, the remaining bytes would be left in the stream. The next request to come from the same client would error, as the server would confuse the remaining bytes, and think the request was malformed. Fixes #197 Fixes #309 --- src/http.rs | 34 ++++++++++++++-------- src/lib.rs | 2 +- src/server/request.rs | 67 ++++++++++++++++++++++++++++++++----------- 3 files changed, 74 insertions(+), 29 deletions(-) diff --git a/src/http.rs b/src/http.rs index 2d081bcb43..76bc5ba405 100644 --- a/src/http.rs +++ b/src/http.rs @@ -3,7 +3,9 @@ use std::borrow::Cow::{Borrowed, Owned}; use std::borrow::IntoCow; use std::cmp::min; use std::old_io::{self, Reader, IoResult, BufWriter}; +use std::mem; use std::num::from_u16; +use std::ptr; use std::str; use std::string::CowString; @@ -20,7 +22,7 @@ use HttpError::{HttpHeaderError, HttpIoError, HttpMethodError, HttpStatusError, HttpUriError, HttpVersionError}; use HttpResult; -use self::HttpReader::{SizedReader, ChunkedReader, EofReader, EmptyReader}; +use self::HttpReader::{SizedReader, ChunkedReader, EofReader}; use self::HttpWriter::{ThroughWriter, ChunkedWriter, SizedWriter, EmptyWriter}; /// Readers to handle different Transfer-Encodings. @@ -47,22 +49,31 @@ pub enum HttpReader { /// > reliably; the server MUST respond with the 400 (Bad Request) /// > status code and then close the connection. EofReader(R), - /// A Reader used for messages that should never have a body. - /// - /// See https://tools.ietf.org/html/rfc7230#section-3.3.3 - EmptyReader(R), } impl HttpReader { /// Unwraps this HttpReader and returns the underlying Reader. + #[inline] pub fn unwrap(self) -> R { - match self { - SizedReader(r, _) => r, - ChunkedReader(r, _) => r, - EofReader(r) => r, - EmptyReader(r) => r, - } + let r = unsafe { + ptr::read(match self { + SizedReader(ref r, _) => r, + ChunkedReader(ref r, _) => r, + EofReader(ref r) => r, + }) + }; + unsafe { mem::forget(self); } + r + } +} + +#[unsafe_destructor] +impl Drop for HttpReader { + #[inline] + fn drop(&mut self) { + let mut buf = [0u8; 512]; + while self.read(&mut buf).is_ok() {} } } @@ -116,7 +127,6 @@ impl Reader for HttpReader { EofReader(ref mut body) => { body.read(buf) }, - EmptyReader(_) => Err(old_io::standard_error(old_io::EndOfFile)) } } } diff --git a/src/lib.rs b/src/lib.rs index 592c8d6ef0..c1ef1dc2a5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,5 +1,5 @@ #![feature(core, collections, hash, io, os, path, std_misc, - slicing_syntax, box_syntax)] + slicing_syntax, box_syntax, unsafe_destructor)] #![deny(missing_docs)] #![cfg_attr(test, deny(warnings))] #![cfg_attr(test, feature(alloc, test))] diff --git a/src/server/request.rs b/src/server/request.rs index fde76ab840..fde7e9b5ed 100644 --- a/src/server/request.rs +++ b/src/server/request.rs @@ -2,7 +2,7 @@ //! //! These are requests that a `hyper::Server` receives, and include its method, //! target URI, headers, and message body. -use std::old_io::IoResult; +use std::old_io::{self, IoResult}; use std::old_io::net::ip::SocketAddr; use {HttpResult}; @@ -11,7 +11,7 @@ use method::Method::{self, Get, Head}; use header::{Headers, ContentLength, TransferEncoding}; use http::{read_request_line}; use http::HttpReader; -use http::HttpReader::{SizedReader, ChunkedReader, EmptyReader}; +use http::HttpReader::{SizedReader, ChunkedReader}; use uri::RequestUri; /// A request bundles several parts of an incoming `NetworkStream`, given to a `Handler`. @@ -26,7 +26,7 @@ pub struct Request<'a> { pub uri: RequestUri, /// The version of HTTP for this request. pub version: HttpVersion, - body: HttpReader<&'a mut (Reader + 'a)> + body: Body> } @@ -39,18 +39,19 @@ impl<'a> Request<'a> { let headers = try!(Headers::from_raw(&mut stream)); debug!("{:?}", headers); - let body = if method == Get || method == Head { - EmptyReader(stream) - } else if headers.has::() { - match headers.get::() { - Some(&ContentLength(len)) => SizedReader(stream, len), - None => unreachable!() - } + let body = if let Some(len) = headers.get::() { + SizedReader(stream, **len) } else if headers.has::() { todo!("check for Transfer-Encoding: chunked"); ChunkedReader(stream, None) } else { - EmptyReader(stream) + SizedReader(stream, 0) + }; + + let body = if method == Get || method == Head { + Body::Empty(body) + } else { + Body::NonEmpty(body) }; Ok(Request { @@ -68,13 +69,31 @@ impl<'a> Request<'a> { RequestUri, HttpVersion, HttpReader<&'a mut (Reader + 'a)>,) { (self.remote_addr, self.method, self.headers, - self.uri, self.version, self.body) + self.uri, self.version, self.body.into_inner()) } } impl<'a> Reader for Request<'a> { + #[inline] fn read(&mut self, buf: &mut [u8]) -> IoResult { - self.body.read(buf) + match self.body { + Body::Empty(..) => Err(old_io::standard_error(old_io::EndOfFile)), + Body::NonEmpty(ref mut r) => r.read(buf) + } + } +} + +enum Body { + Empty(R), + NonEmpty(R), +} + +impl Body { + fn into_inner(self) -> R { + match self { + Body::Empty(r) => r, + Body::NonEmpty(r) => r + } } } @@ -95,8 +114,9 @@ mod tests { let mut stream = MockStream::with_input(b"\ GET / HTTP/1.1\r\n\ Host: example.domain\r\n\ + Content-Length: 18\r\n\ \r\n\ - I'm a bad request.\r\n\ + I'm a bad request.\ "); let mut req = Request::new(&mut stream, sock("127.0.0.1:80")).unwrap(); @@ -108,8 +128,9 @@ mod tests { let mut stream = MockStream::with_input(b"\ HEAD / HTTP/1.1\r\n\ Host: example.domain\r\n\ + Content-Length: 18\r\n\ \r\n\ - I'm a bad request.\r\n\ + I'm a bad request.\ "); let mut req = Request::new(&mut stream, sock("127.0.0.1:80")).unwrap(); @@ -117,7 +138,7 @@ mod tests { } #[test] - fn test_post_empty_body() { + fn test_post_body_with_no_content_length() { let mut stream = MockStream::with_input(b"\ POST / HTTP/1.1\r\n\ Host: example.domain\r\n\ @@ -129,6 +150,20 @@ mod tests { assert_eq!(req.read_to_string(), Ok("".to_string())); } + #[test] + fn test_unexpected_body_drains_upon_drop() { + let mut stream = MockStream::with_input(b"\ + GET / HTTP/1.1\r\n\ + Host: example.domain\r\n\ + Content-Length: 18\r\n\ + \r\n\ + I'm a bad request.\ + "); + + Request::new(&mut stream, sock("127.0.0.1:80")).unwrap().read_to_string().unwrap(); + assert!(stream.read.eof()); + } + #[test] fn test_parse_chunked_request() { let mut stream = MockStream::with_input(b"\