diff --git a/src/dumbo/src/tcp/endpoint.rs b/src/dumbo/src/tcp/endpoint.rs index d2d7c855736..c56defb4785 100644 --- a/src/dumbo/src/tcp/endpoint.rs +++ b/src/dumbo/src/tcp/endpoint.rs @@ -341,6 +341,16 @@ fn parse_request_bytes(byte_stream: &[u8], callback: fn(Request) -> Response) -> StatusCode::BadRequest, Body::new("Invalid headers.".to_string()), ), + RequestError::Overflow => build_response( + Version::default(), + StatusCode::BadRequest, + Body::new(e.to_string()), + ), + RequestError::Underflow => build_response( + Version::default(), + StatusCode::BadRequest, + Body::new(e.to_string()), + ), // `micro-http` supports a predefined list of HTTP headers. // It shouldn't reach this point, because it ignores the // HTTP unsupported headers. diff --git a/src/micro_http/src/common/mod.rs b/src/micro_http/src/common/mod.rs index 21fcf4c6bc5..20e25a1bf9c 100644 --- a/src/micro_http/src/common/mod.rs +++ b/src/micro_http/src/common/mod.rs @@ -28,6 +28,10 @@ pub enum RequestError { InvalidHeader, /// The Request is invalid and cannot be served. InvalidRequest, + /// Overflow occurred when parsing a request. + Overflow, + /// Underflow occurred when parsing a request. + Underflow, } impl Display for RequestError { @@ -39,6 +43,8 @@ impl Display for RequestError { Self::UnsupportedHeader => write!(f, "Unsupported header."), Self::InvalidHeader => write!(f, "Invalid header."), Self::InvalidRequest => write!(f, "Invalid request."), + Self::Overflow => write!(f, "Overflow occurred when parsing a request."), + Self::Underflow => write!(f, "Underflow occurred when parsing a request."), } } } @@ -76,6 +82,10 @@ pub enum ServerError { ConnectionError(ConnectionError), /// Server maximum capacity has been reached. ServerFull, + /// Overflow occured while processing messages. + Overflow, + /// Underflow occured while processing mesagges. + Underflow, } impl Display for ServerError { @@ -84,6 +94,8 @@ impl Display for ServerError { Self::IOError(inner) => write!(f, "IO error: {}", inner), Self::ConnectionError(inner) => write!(f, "Connection error: {}", inner), Self::ServerFull => write!(f, "Server is full."), + Self::Overflow => write!(f, "Overflow occured while processing messages."), + Self::Underflow => write!(f, "Underflow occured while processing messages."), } } } @@ -226,9 +238,11 @@ mod tests { fn eq(&self, other: &Self) -> bool { use self::ConnectionError::*; match (self, other) { - (ParseError(_), ParseError(_)) => true, + (ParseError(ref e), ParseError(ref other_e)) => e.eq(other_e), (ConnectionClosed, ConnectionClosed) => true, - (StreamError(_), StreamError(_)) => true, + (StreamError(ref e), StreamError(ref other_e)) => { + format!("{}", e).eq(&format!("{}", other_e)) + } (InvalidWrite, InvalidWrite) => true, _ => false, } @@ -284,30 +298,38 @@ mod tests { #[test] fn test_display_request_error() { + assert_eq!( + format!("{}", RequestError::InvalidHeader), + "Invalid header." + ); assert_eq!( format!("{}", RequestError::InvalidHttpMethod("test")), "Invalid HTTP Method: test" ); + assert_eq!( + format!("{}", RequestError::InvalidHttpVersion("test")), + "Invalid HTTP Version: test" + ); + assert_eq!( + format!("{}", RequestError::InvalidRequest), + "Invalid request." + ); assert_eq!( format!("{}", RequestError::InvalidUri("test")), "Invalid URI: test" ); assert_eq!( - format!("{}", RequestError::InvalidHttpVersion("test")), - "Invalid HTTP Version: test" + format!("{}", RequestError::Overflow), + "Overflow occurred when parsing a request." ); assert_eq!( - format!("{}", RequestError::InvalidHeader), - "Invalid header." + format!("{}", RequestError::Underflow), + "Underflow occurred when parsing a request." ); assert_eq!( format!("{}", RequestError::UnsupportedHeader), "Unsupported header." ); - assert_eq!( - format!("{}", RequestError::InvalidRequest), - "Invalid request." - ); } #[test] @@ -353,5 +375,13 @@ mod tests { ), "IO error: Resource temporarily unavailable (os error 11)" ); + assert_eq!( + format!("{}", ServerError::Overflow), + "Overflow occured while processing messages." + ); + assert_eq!( + format!("{}", ServerError::Underflow), + "Underflow occured while processing messages." + ); } } diff --git a/src/micro_http/src/request.rs b/src/micro_http/src/request.rs index 4ae45e4dc54..466a8c3396f 100644 --- a/src/micro_http/src/request.rs +++ b/src/micro_http/src/request.rs @@ -8,6 +8,9 @@ pub use common::RequestError; use common::{Body, Method, Version}; use headers::Headers; +// This type represents the RequestLine raw parts: method, uri and version. +type RequestLineParts<'a> = (&'a [u8], &'a [u8], &'a [u8]); + /// Finds the first occurence of `sequence` in the `bytes` slice. /// /// Returns the starting position of the `sequence` in `bytes` or `None` if the @@ -57,6 +60,7 @@ impl Uri { const HTTP_SCHEME_PREFIX: &str = "http://"; if self.string.starts_with(HTTP_SCHEME_PREFIX) { + // Slice access is safe because we checked above that `self.string` size <= `HTTP_SCHEME_PREFIX.len()`. let without_scheme = &self.string[HTTP_SCHEME_PREFIX.len()..]; if without_scheme.is_empty() { return ""; @@ -64,6 +68,7 @@ impl Uri { // The host in this case includes the port and contains the bytes after http:// up to // the next '/'. match without_scheme.bytes().position(|byte| byte == b'/') { + // Slice access is safe because `position` validates that `len` is a valid index. Some(len) => &without_scheme[len..], None => "", } @@ -86,24 +91,36 @@ pub struct RequestLine { } impl RequestLine { - fn parse_request_line(request_line: &[u8]) -> (&[u8], &[u8], &[u8]) { + fn parse_request_line( + request_line: &[u8], + ) -> std::result::Result { if let Some(method_end) = find(request_line, &[SP]) { + // The slice access is safe because `find` validates that `method_end` < `request_line` size. let method = &request_line[..method_end]; - let uri_and_version = &request_line[(method_end + 1)..]; + // `uri_start` <= `request_line` size. + let uri_start = method_end.checked_add(1).ok_or(RequestError::Overflow)?; + + // Slice access is safe because `uri_start` <= `request_line` size. + // If `uri_start` == `request_line` size, then `uri_and_version` will be an empty slice. + let uri_and_version = &request_line[uri_start..]; if let Some(uri_end) = find(uri_and_version, &[SP]) { + // Slice access is safe because `find` validates that `uri_end` < `uri_and_version` size. let uri = &uri_and_version[..uri_end]; - let version = &uri_and_version[(uri_end + 1)..]; + // `version_start` <= `uri_and_version` size. + let version_start = uri_end.checked_add(1).ok_or(RequestError::Overflow)?; - return (method, uri, version); - } + // Slice access is safe because `version_start` <= `uri_and_version` size. + let version = &uri_and_version[version_start..]; - return (method, uri_and_version, b""); + return Ok((method, uri, version)); + } } - (b"", b"", b"") + // Request Line can be valid only if it contains the method, uri and version separated with SP. + Err(RequestError::InvalidRequest) } /// Tries to parse a byte stream in a request line. Fails if the request line is malformed. @@ -113,7 +130,7 @@ impl RequestLine { /// `InvalidHttpVersion` is returned if the specified HTTP version is unsupported. /// `InvalidUri` is returned if the specified Uri is not valid. pub fn try_from(request_line: &[u8]) -> Result { - let (method, uri, version) = Self::parse_request_line(request_line); + let (method, uri, version) = Self::parse_request_line(request_line)?; Ok(Self { method: Method::try_from(method)?, @@ -123,9 +140,10 @@ impl RequestLine { } // Returns the minimum length of a valid request. The request must contain - // the method (GET), the URI (minmum 1 character), the HTTP version(HTTP/DIGIT.DIGIT) and + // the method (GET), the URI (minimum 1 character), the HTTP version(HTTP/DIGIT.DIGIT) and // 2 separators (SP). fn min_len() -> usize { + // Addition is safe because these are small constants. Method::Get.raw().len() + 1 + Version::Http10.raw().len() + 2 } } @@ -148,8 +166,9 @@ impl Request { /// The byte slice is expected to have the following format:
/// * Request Line: "GET SP Request-uri SP HTTP/1.0 CRLF" - Mandatory
/// * Request Headers " CRLF"- Optional
+ /// * Empty Line "CRLF"
/// * Entity Body - Optional
- /// The request headers and the entity body is not parsed and None is returned because + /// The request headers and the entity body are not parsed and None is returned because /// these are not used by the MMDS server. /// The only supported method is GET and the HTTP protocol is expected to be HTTP/1.0 /// or HTTP/1.1. @@ -163,7 +182,7 @@ impl Request { /// extern crate micro_http; /// use micro_http::Request; /// - /// let http_request = Request::try_from(b"GET http://localhost/home HTTP/1.0\r\n"); + /// let http_request = Request::try_from(b"GET http://localhost/home HTTP/1.0\r\n\r\n").unwrap(); /// ``` pub fn try_from(byte_stream: &[u8]) -> Result { // The first line of the request is the Request Line. The line ending is CR LF. @@ -173,6 +192,7 @@ impl Request { None => return Err(RequestError::InvalidRequest), }; + // Slice access is safe because `find` validates that `request_line_end` < `byte_stream` size. let request_line_bytes = &byte_stream[..request_line_end]; if request_line_bytes.len() < RequestLine::min_len() { return Err(RequestError::InvalidRequest); @@ -193,8 +213,20 @@ impl Request { Some(headers_end) => { // Parse the request headers. // Start by removing the leading CR LF from them. - let headers_and_body = &byte_stream[(request_line_end + CRLF_LEN)..]; + let headers_start = request_line_end + .checked_add(CRLF_LEN) + .ok_or(RequestError::Overflow)?; + // Slice access is safe because starting from `request_line_end` there are at least two CRLF + // (enforced by `find` at the start of this method). + let headers_and_body = &byte_stream[headers_start..]; + // Because we advanced the start with CRLF_LEN, we now have to subtract CRLF_LEN + // from the end in order to keep the same window. + // Underflow is not possible here because `byte_stream[request_line_end..]` starts with CR LF, + // so `headers_end` can be either zero (this case is treated separately in the first match arm) + // or >= 3 (current case). let headers_end = headers_end - CRLF_LEN; + // Slice access is safe because `headers_end` is checked above + // (`find` gives a valid position, and subtracting 2 can't underflow). let headers = Headers::try_from(&headers_and_body[..headers_end])?; // Parse the body of the request. @@ -205,16 +237,22 @@ impl Request { None } content_length => { + // Multiplication is safe because `CRLF_LEN` is a small constant. + let crlf_end = headers_end + .checked_add(2 * CRLF_LEN) + .ok_or(RequestError::Overflow)?; + // This can't underflow because `headers_and_body.len()` >= `crlf_end`. + let body_len = headers_and_body.len() - crlf_end; // Headers suggest we have a body, but the buffer is shorter than the specified // content length. - if headers_and_body.len() - (headers_end + 2 * CRLF_LEN) - < content_length as usize - { + if body_len < content_length as usize { return Err(RequestError::InvalidRequest); } - let body_as_bytes = &headers_and_body[(headers_end + 2 * CRLF_LEN)..]; + // Slice access is safe because `crlf_end` is the index after two CRLF + // (it is <= `headers_and_body` size). + let body_as_bytes = &headers_and_body[crlf_end..]; // If the actual length of the body is different than the `Content-Length` value - // in the headers then this request is invalid. + // in the headers, then this request is invalid. if body_as_bytes.len() == content_length as usize { Some(Body::new(body_as_bytes)) } else { @@ -343,6 +381,13 @@ mod tests { expected_request_line ); + // Test for invalid request missing the separator. + let request_line = b"GET"; + assert_eq!( + RequestLine::try_from(request_line).unwrap_err(), + RequestError::InvalidRequest + ); + // Test for invalid method. let request_line = b"POST http://localhost/home HTTP/1.0"; assert_eq!( @@ -368,14 +413,14 @@ mod tests { let request_line = b"nothing"; assert_eq!( RequestLine::try_from(request_line).unwrap_err(), - RequestError::InvalidHttpMethod("Unsupported HTTP method.") + RequestError::InvalidRequest ); // Test for invalid format with no version. let request_line = b"GET /"; assert_eq!( RequestLine::try_from(request_line).unwrap_err(), - RequestError::InvalidHttpVersion("Unsupported HTTP version.") + RequestError::InvalidRequest ); } @@ -398,6 +443,13 @@ mod tests { assert_eq!(request.http_version(), Version::Http10); assert!(request.body.is_none()); + // Test for invalid Request (missing CR LF). + let request_bytes = b"GET / HTTP/1.1"; + assert_eq!( + Request::try_from(request_bytes).unwrap_err(), + RequestError::InvalidRequest + ); + // Test for invalid Request (length is less than minimum). let request_bytes = b"GET"; assert_eq!( @@ -442,7 +494,6 @@ mod tests { // Test for an invalid content length. let request = Request::try_from( b"PATCH http://localhost/home HTTP/1.1\r\n\ - Expect: 100-continue\r\n\ Content-Length: 5000\r\n\r\nthis is a short body", ) .unwrap_err(); diff --git a/src/micro_http/src/response.rs b/src/micro_http/src/response.rs index aeb8d93dcb7..04c9e5391f8 100644 --- a/src/micro_http/src/response.rs +++ b/src/micro_http/src/response.rs @@ -107,6 +107,7 @@ impl ResponseHeaders { let delimitator = b", "; for (idx, method) in self.allow.iter().enumerate() { buf.write_all(method.raw())?; + // We check above that `self.allow` is not empty. if idx < self.allow.len() - 1 { buf.write_all(delimitator)?; } diff --git a/src/micro_http/src/server.rs b/src/micro_http/src/server.rs index 0ae67f82bf4..71229a912f5 100644 --- a/src/micro_http/src/server.rs +++ b/src/micro_http/src/server.rs @@ -146,7 +146,10 @@ impl ClientConnection { } } } - self.in_flight_response_count += parsed_requests.len() as u32; + self.in_flight_response_count = self + .in_flight_response_count + .checked_add(parsed_requests.len() as u32) + .ok_or(ServerError::Overflow)?; // If the state of the connection has changed, we need to update // the event set in the `epoll` structure. if self.connection.pending_write() { @@ -178,11 +181,15 @@ impl ClientConnection { Ok(()) } - fn enqueue_response(&mut self, response: Response) { + fn enqueue_response(&mut self, response: Response) -> Result<()> { if self.state != ClientConnectionState::Closed { self.connection.enqueue_response(response); } - self.in_flight_response_count -= 1; + self.in_flight_response_count = self + .in_flight_response_count + .checked_sub(1) + .ok_or(ServerError::Underflow)?; + Ok(()) } // Returns `true` if the connection is closed and safe to drop. @@ -450,6 +457,7 @@ impl HttpServer { /// /// # Errors /// `IOError` is returned when an `epoll::ctl` operation fails. + /// `Underflow` is returned when `enqueue_response` fails. pub fn respond(&mut self, response: ServerResponse) -> Result<()> { if let Some(client_connection) = self.connections.get_mut(&(response.id as i32)) { // If the connection was incoming before we enqueue the response, we change its @@ -458,7 +466,7 @@ impl HttpServer { client_connection.state = ClientConnectionState::AwaitingOutgoing; Self::epoll_mod(&self.epoll, response.id as RawFd, epoll::EventSet::OUT)?; } - client_connection.enqueue_response(response.response); + client_connection.enqueue_response(response.response)?; } Ok(()) } diff --git a/tests/integration_tests/build/test_coverage.py b/tests/integration_tests/build/test_coverage.py index eac64e5ec57..72f6ca989e7 100644 --- a/tests/integration_tests/build/test_coverage.py +++ b/tests/integration_tests/build/test_coverage.py @@ -23,7 +23,7 @@ # this contains the frequency while on AMD it does not. # Checkout the cpuid crate. In the future other # differences may appear. -COVERAGE_DICT = {"Intel": 84.52, "AMD": 84.51} +COVERAGE_DICT = {"Intel": 84.44, "AMD": 84.45} PROC_MODEL = proc.proc_type() COVERAGE_MAX_DELTA = 0.05