Refactor deframer to hopefully improve clarity

There were some unwraps here that we can get rid of if we unduplicate
the work Message::read already does.  That goes in Message::read_with_detailed_error.

Delete a fuzz corpus file that was actually wrong, but allowed by Message::read's
previous lax semantics.

rustls/src/msgs/deframer.rs

@@ -3,15 +3,7 @@ use std::collections::VecDeque;
 use std::io;
 
 use crate::msgs::codec;
-use crate::msgs::codec::Codec;
-use crate::msgs::message::Message;
-
-const HEADER_SIZE: usize = 1 + 2 + 2;
-
-/// This is the maximum on-the-wire size of a TLSCiphertext.
-/// That's 2^14 payload bytes, a header, and a 2KB allowance
-/// for ciphertext overheads.
-const MAX_MESSAGE: usize = 16384 + 2048 + HEADER_SIZE;
+use crate::msgs::message::{Message, MessageError};
 
 /// This deframer works to reconstruct TLS messages
 /// from arbitrary-sized reads, buffering as necessary.
@@ -27,7 +19,7 @@ pub struct MessageDeframer {
 
     /// A fixed-size buffer containing the currently-accumulating
     /// TLS message.
-    buf: Box<[u8; MAX_MESSAGE]>,
+    buf: Box<[u8; Message::MAX_WIRE_SIZE]>,
 
     /// What size prefix of `buf` is used.
     used: usize,
@@ -54,7 +46,7 @@ impl MessageDeframer {
         MessageDeframer {
             frames: VecDeque::new(),
             desynced: false,
-            buf: Box::new([0u8; MAX_MESSAGE]),
+            buf: Box::new([0u8; Message::MAX_WIRE_SIZE]),
             used: 0,
         }
     }
@@ -67,20 +59,18 @@ impl MessageDeframer {
         // we get a message with a length field out of range here,
         // we do a zero length read.  That looks like an EOF to
         // the next layer up, which is fine.
-        debug_assert!(self.used <= MAX_MESSAGE);
+        debug_assert!(self.used <= Message::MAX_WIRE_SIZE);
         let new_bytes = rd.read(&mut self.buf[self.used..])?;
 
         self.used += new_bytes;
 
         loop {
-            match self.buf_contains_message() {
+            match self.try_deframe_one() {
                 BufferContents::Invalid => {
                     self.desynced = true;
                     break;
                 }
-                BufferContents::Valid => {
-                    self.deframe_one();
-                }
+                BufferContents::Valid => continue,
                 BufferContents::Partial => break,
             }
         }
@@ -97,39 +87,23 @@ impl MessageDeframer {
 
     /// Does our `buf` contain a full message?  It does if it is big enough to
     /// contain a header, and that header has a length which falls within `buf`.
-    fn buf_contains_message(&self) -> BufferContents {
-        if self.used < HEADER_SIZE {
-            return BufferContents::Partial;
+    /// If so, deframe it and place the message onto the frames output queue.
+    fn try_deframe_one(&mut self) -> BufferContents {
+        // Try to decode a message off the front of buf.
+        let mut rd = codec::Reader::init(&self.buf[..self.used]);
+
+        match Message::read_with_detailed_error(&mut rd) {
+            Ok(m) => {
+                let used = rd.used();
+                self.frames.push_back(m);
+                self.buf_consume(used);
+                BufferContents::Valid
+            },
+            Err(MessageError::TooShortForHeader) | Err(MessageError::TooShortForLength) => {
+                BufferContents::Partial
+            },
+            Err(_) => BufferContents::Invalid
         }
-
-        let len_maybe = Message::check_header(&self.buf[..self.used]);
-
-        // Header damaged.
-        if len_maybe == None {
-            return BufferContents::Invalid;
-        }
-
-        let len = len_maybe.unwrap();
-
-        // This is just too large.
-        if len >= MAX_MESSAGE - HEADER_SIZE {
-            return BufferContents::Invalid;
-        }
-
-        let full_message = self.used >= len + HEADER_SIZE;
-        if full_message { BufferContents::Valid } else { BufferContents::Partial }
-    }
-
-    /// Take a TLS message off the front of `buf`, and put it onto the back
-    /// of our `frames` deque.
-    fn deframe_one(&mut self) {
-        let used = {
-            let mut rd = codec::Reader::init(&self.buf[..self.used]);
-            let m = Message::read(&mut rd).unwrap();
-            self.frames.push_back(m);
-            rd.used()
-        };
-        self.buf_consume(used);
     }
 
     fn buf_consume(&mut self, taken: usize) {

rustls/src/msgs/message.rs

@@ -78,21 +78,24 @@ pub struct Message {
     pub payload: MessagePayload,
 }
 
+impl Message {
+    /// This is the maximum on-the-wire size of a TLSCiphertext.
+    /// That's 2^14 payload bytes, a header, and a 2KB allowance
+    /// for ciphertext overheads.
+    const MAX_PAYLOAD: u16 = 16384 + 2048;
+
+    /// Content type, version and size.
+    const HEADER_SIZE: u16 = 1 + 2 + 2;
+
+    /// Maximum on-wire message size.
+    pub const MAX_WIRE_SIZE: usize = (Message::MAX_PAYLOAD + Message::HEADER_SIZE) as usize;
+}
+
 impl Codec for Message {
     fn read(r: &mut Reader) -> Option<Message> {
-        let typ = ContentType::read(r)?;
-        let version = ProtocolVersion::read(r)?;
-        let len = u16::read(r)?;
-
-        let mut sub = r.sub(len as usize)?;
-        let payload = Payload::read(&mut sub)?;
-
-        Some(Message {
-            typ,
-            version,
-            payload: MessagePayload::Opaque(payload),
-        })
-    }
+        Message::read_with_detailed_error(r)
+            .ok()
+   }
 
     fn encode(&self, bytes: &mut Vec<u8>) {
         self.typ.encode(bytes);
@@ -102,31 +105,55 @@ impl Codec for Message {
     }
 }
 
-impl Message {
-    /// Do some *very* lax checks on the header, and return
-    /// None if it looks really broken.  Otherwise, return
-    /// the length field.
-    pub fn check_header(bytes: &[u8]) -> Option<usize> {
-        let mut rd = Reader::init(bytes);
+#[derive(Debug)]
+pub enum MessageError {
+    TooShortForHeader,
+    TooShortForLength,
+    IllegalLength,
+    IllegalContentType,
+    IllegalProtocolVersion,
+}
 
-        let typ = ContentType::read(&mut rd)?;
-        let version = ProtocolVersion::read(&mut rd)?;
-        let len = u16::read(&mut rd)?;
+impl Message {
+    /// Like Message::read(), but allows the important distinction between:
+    /// this message might be valid if we read more data; and this message will
+    /// never be valid.
+    pub fn read_with_detailed_error(r: &mut Reader) -> Result<Message, MessageError> {
+        let typ = ContentType::read(r)
+            .ok_or(MessageError::TooShortForHeader)?;
+        let version = ProtocolVersion::read(r)
+            .ok_or(MessageError::TooShortForHeader)?;
+        let len = u16::read(r)
+            .ok_or(MessageError::TooShortForHeader)?;
+
+        // Reject oversize messages
+        if len >= Message::MAX_PAYLOAD {
+            return Err(MessageError::IllegalLength);
+        }
 
         // Don't accept any new content-types.
         if let ContentType::Unknown(_) = typ {
-            return None;
+            return Err(MessageError::IllegalContentType);
         }
 
         // Accept only versions 0x03XX for any XX.
         match version {
             ProtocolVersion::Unknown(ref v) if (v & 0xff00) != 0x0300 => {
-                return None;
+                return Err(MessageError::IllegalProtocolVersion);
             }
             _ => (),
         };
 
-        Some(len as usize)
+        let mut sub = r.sub(len as usize)
+            .ok_or(MessageError::TooShortForLength)?;
+        let payload = Payload::read(&mut sub)
+            .unwrap();
+
+        Ok(Message {
+            typ,
+            version,
+            payload: MessagePayload::Opaque(payload),
+        })
     }
 
     pub fn is_content_type(&self, typ: ContentType) -> bool {