Merge pull request hyperium#228 from mlalic/chunksize-fix

Fix chunk size parsing: handle invalid chunk sizes

src/client/response.rs

@@ -100,6 +100,8 @@ mod tests {
     use std::io::BufferedReader;
 
     use header::Headers;
+    use header::common::TransferEncoding;
+    use header::common::transfer_encoding::Encoding;
     use http::HttpReader::EofReader;
     use http::RawStatus;
     use mock::MockStream;
@@ -124,4 +126,95 @@ mod tests {
         assert_eq!(b, box MockStream::new());
 
     }
+
+    #[test]
+    fn test_parse_chunked_response() {
+        let stream = MockStream::with_input(b"\
+            HTTP/1.1 200 OK\r\n\
+            Transfer-Encoding: chunked\r\n\
+            \r\n\
+            1\r\n\
+            q\r\n\
+            2\r\n\
+            we\r\n\
+            2\r\n\
+            rt\r\n\
+            0\r\n\
+            \r\n"
+        );
+
+        let mut res = Response::new(box stream).unwrap();
+
+        // The status line is correct?
+        assert_eq!(res.status, status::StatusCode::Ok);
+        assert_eq!(res.version, version::HttpVersion::Http11);
+        // The header is correct?
+        match res.headers.get::<TransferEncoding>() {
+            Some(encodings) => {
+                assert_eq!(1, encodings.len());
+                assert_eq!(Encoding::Chunked, encodings[0]);
+            },
+            None => panic!("Transfer-Encoding: chunked expected!"),
+        };
+        // The body is correct?
+        let body = res.read_to_string().unwrap();
+        assert_eq!("qwert", body);
+    }
+
+    /// Tests that when a chunk size is not a valid radix-16 number, an error
+    /// is returned.
+    #[test]
+    fn test_invalid_chunk_size_not_hex_digit() {
+        let stream = MockStream::with_input(b"\
+            HTTP/1.1 200 OK\r\n\
+            Transfer-Encoding: chunked\r\n\
+            \r\n\
+            X\r\n\
+            1\r\n\
+            0\r\n\
+            \r\n"
+        );
+
+        let mut res = Response::new(box stream).unwrap();
+
+        assert!(res.read_to_string().is_err());
+    }
+
+    /// Tests that when a chunk size contains an invalid extension, an error is
+    /// returned.
+    #[test]
+    fn test_invalid_chunk_size_extension() {
+        let stream = MockStream::with_input(b"\
+            HTTP/1.1 200 OK\r\n\
+            Transfer-Encoding: chunked\r\n\
+            \r\n\
+            1 this is an invalid extension\r\n\
+            1\r\n\
+            0\r\n\
+            \r\n"
+        );
+
+        let mut res = Response::new(box stream).unwrap();
+
+        assert!(res.read_to_string().is_err());
+    }
+
+    /// Tests that when a valid extension that contains a digit is appended to
+    /// the chunk size, the chunk is correctly read.
+    #[test]
+    fn test_chunk_size_with_extension() {
+        let stream = MockStream::with_input(b"\
+            HTTP/1.1 200 OK\r\n\
+            Transfer-Encoding: chunked\r\n\
+            \r\n\
+            1;this is an extension with a digit 1\r\n\
+            1\r\n\
+            0\r\n\
+            \r\n"
+        );
+
+        let mut res = Response::new(box stream).unwrap();
+
+        assert_eq!("1", res.read_to_string().unwrap())
+    }
 }

src/http.rs

@@ -137,17 +137,18 @@ fn read_chunk_size<R: Reader>(rdr: &mut R) -> IoResult<uint> {
     let mut size = 0u;
     let radix = 16;
     let mut in_ext = false;
+    let mut in_chunk_size = true;
     loop {
         match try!(rdr.read_byte()) {
-            b@b'0'...b'9' if !in_ext => {
+            b@b'0'...b'9' if in_chunk_size => {
                 size *= radix;
                 size += (b - b'0') as uint;
             },
-            b@b'a'...b'f' if !in_ext => {
+            b@b'a'...b'f' if in_chunk_size => {
                 size *= radix;
                 size += (b + 10 - b'a') as uint;
             },
-            b@b'A'...b'F' if !in_ext => {
+            b@b'A'...b'F' if in_chunk_size => {
                 size *= radix;
                 size += (b + 10 - b'A') as uint;
             },
@@ -157,9 +158,28 @@ fn read_chunk_size<R: Reader>(rdr: &mut R) -> IoResult<uint> {
                     _ => return Err(io::standard_error(io::InvalidInput))
                 }
             },
-            ext => {
+            // If we weren't in the extension yet, the ";" signals its start
+            b';' if !in_ext => {
                 in_ext = true;
+                in_chunk_size = false;
+            },
+            // "Linear white space" is ignored between the chunk size and the
+            // extension separator token (";") due to the "implied *LWS rule".
+            b'\t' | b' ' if !in_ext & !in_chunk_size => {},
+            // LWS can follow the chunk size, but no more digits can come
+            b'\t' | b' ' if in_chunk_size => in_chunk_size = false,
+            // We allow any arbitrary octet once we are in the extension, since
+            // they all get ignored anyway. According to the HTTP spec, valid
+            // extensions would have a more strict syntax:
+            //     (token ["=" (token | quoted-string)])
+            // but we gain nothing by rejecting an otherwise valid chunk size.
+            ext if in_ext => {
                 todo!("chunk extension byte={}", ext);
+            },
+            // Finally, if we aren't in the extension and we're reading any
+            // other octet, the chunk size line is invalid!
+            _ => {
+                return Err(io::standard_error(io::InvalidInput));
             }
         }
     }
@@ -689,7 +709,7 @@ fn expect(r: IoResult<u8>, expected: u8) -> HttpResult<()> {
 
 #[cfg(test)]
 mod tests {
-    use std::io::{self, MemReader, MemWriter};
+    use std::io::{self, MemReader, MemWriter, IoResult};
     use std::borrow::Cow::{Borrowed, Owned};
     use test::Bencher;
     use uri::RequestUri;
@@ -702,7 +722,7 @@ mod tests {
     use url::Url;
 
     use super::{read_method, read_uri, read_http_version, read_header,
-                RawHeaderLine, read_status, RawStatus};
+                RawHeaderLine, read_status, RawStatus, read_chunk_size};
 
     fn mem(s: &str) -> MemReader {
         MemReader::new(s.as_bytes().to_vec())
@@ -811,6 +831,43 @@ mod tests {
         assert_eq!(s, "foo barb");
     }
 
+    #[test]
+    fn test_read_chunk_size() {
+        fn read(s: &str, result: IoResult<uint>) {
+            assert_eq!(read_chunk_size(&mut mem(s)), result);
+        }
+
+        read("1\r\n", Ok(1));
+        read("01\r\n", Ok(1));
+        read("0\r\n", Ok(0));
+        read("00\r\n", Ok(0));
+        read("A\r\n", Ok(10));
+        read("a\r\n", Ok(10));
+        read("Ff\r\n", Ok(255));
+        read("Ff   \r\n", Ok(255));
+        // Missing LF or CRLF
+        read("F\rF", Err(io::standard_error(io::InvalidInput)));
+        read("F", Err(io::standard_error(io::EndOfFile)));
+        // Invalid hex digit
+        read("X\r\n", Err(io::standard_error(io::InvalidInput)));
+        read("1X\r\n", Err(io::standard_error(io::InvalidInput)));
+        read("-\r\n", Err(io::standard_error(io::InvalidInput)));
+        read("-1\r\n", Err(io::standard_error(io::InvalidInput)));
+        // Acceptable (if not fully valid) extensions do not influence the size
+        read("1;extension\r\n", Ok(1));
+        read("a;ext name=value\r\n", Ok(10));
+        read("1;extension;extension2\r\n", Ok(1));
+        read("1;;;  ;\r\n", Ok(1));
+        read("2; extension...\r\n", Ok(2));
+        read("3   ; extension=123\r\n", Ok(3));
+        read("3   ;\r\n", Ok(3));
+        read("3   ;   \r\n", Ok(3));
+        // Invalid extensions cause an error
+        read("1 invalid extension\r\n", Err(io::standard_error(io::InvalidInput)));
+        read("1 A\r\n", Err(io::standard_error(io::InvalidInput)));
+        read("1;no CRLF", Err(io::standard_error(io::EndOfFile)));
+    }
+
     #[bench]
     fn bench_read_method(b: &mut Bencher) {
         b.bytes = b"CONNECT ".len() as u64;

src/server/request.rs

@@ -75,6 +75,8 @@ impl<'a> Reader for Request<'a> {
 
 #[cfg(test)]
 mod tests {
+    use header::common::{Host, TransferEncoding};
+    use header::common::transfer_encoding::Encoding;
     use mock::MockStream;
     use super::Request;
 
@@ -122,4 +124,103 @@ mod tests {
         let mut req = Request::new(&mut stream, sock("127.0.0.1:80")).unwrap();
         assert_eq!(req.read_to_string(), Ok("".to_string()));
     }
+
+    #[test]
+    fn test_parse_chunked_request() {
+        let mut stream = MockStream::with_input(b"\
+            POST / HTTP/1.1\r\n\
+            Host: example.domain\r\n\
+            Transfer-Encoding: chunked\r\n\
+            \r\n\
+            1\r\n\
+            q\r\n\
+            2\r\n\
+            we\r\n\
+            2\r\n\
+            rt\r\n\
+            0\r\n\
+            \r\n"
+        );
+
+        let mut req = Request::new(&mut stream, sock("127.0.0.1:80")).unwrap();
+
+        // The headers are correct?
+        match req.headers.get::<Host>() {
+            Some(host) => {
+                assert_eq!("example.domain", host.hostname);
+            },
+            None => panic!("Host header expected!"),
+        };
+        match req.headers.get::<TransferEncoding>() {
+            Some(encodings) => {
+                assert_eq!(1, encodings.len());
+                assert_eq!(Encoding::Chunked, encodings[0]);
+            }
+            None => panic!("Transfer-Encoding: chunked expected!"),
+        };
+        // The content is correctly read?
+        let body = req.read_to_string().unwrap();
+        assert_eq!("qwert", body);
+    }
+
+    /// Tests that when a chunk size is not a valid radix-16 number, an error
+    /// is returned.
+    #[test]
+    fn test_invalid_chunk_size_not_hex_digit() {
+        let mut stream = MockStream::with_input(b"\
+            POST / HTTP/1.1\r\n\
+            Host: example.domain\r\n\
+            Transfer-Encoding: chunked\r\n\
+            \r\n\
+            X\r\n\
+            1\r\n\
+            0\r\n\
+            \r\n"
+        );
+
+        let mut req = Request::new(&mut stream, sock("127.0.0.1:80")).unwrap();
+
+        assert!(req.read_to_string().is_err());
+    }
+
+    /// Tests that when a chunk size contains an invalid extension, an error is
+    /// returned.
+    #[test]
+    fn test_invalid_chunk_size_extension() {
+        let mut stream = MockStream::with_input(b"\
+            POST / HTTP/1.1\r\n\
+            Host: example.domain\r\n\
+            Transfer-Encoding: chunked\r\n\
+            \r\n\
+            1 this is an invalid extension\r\n\
+            1\r\n\
+            0\r\n\
+            \r\n"
+        );
+
+        let mut req = Request::new(&mut stream, sock("127.0.0.1:80")).unwrap();
+
+        assert!(req.read_to_string().is_err());
+    }
+
+    /// Tests that when a valid extension that contains a digit is appended to
+    /// the chunk size, the chunk is correctly read.
+    #[test]
+    fn test_chunk_size_with_extension() {
+        let mut stream = MockStream::with_input(b"\
+            POST / HTTP/1.1\r\n\
+            Host: example.domain\r\n\
+            Transfer-Encoding: chunked\r\n\
+            \r\n\
+            1;this is an extension with a digit 1\r\n\
+            1\r\n\
+            0\r\n\
+            \r\n"
+        );
+
+        let mut req = Request::new(&mut stream, sock("127.0.0.1:80")).unwrap();
+
+        assert_eq!("1", req.read_to_string().unwrap())
+    }
+
 }