Repository: tokio-rs/prost (language: rust)

Issue:
Return error for insufficient buffer size from decode_length_delimited
Add a special case for reporting the buffer underflow when decoding a length delimited message. This is useful when using protobufs with tokio-io, which will give you a buffer of bytes that it has gotten so far and then asks you if there are enough bytes to decode a message. 

Proof of Concept:

```diff
diff --git a/src/encoding.rs b/src/encoding.rs
index 78b4105..44c2a5b 100644
--- a/src/encoding.rs
+++ b/src/encoding.rs
@@ -224,7 +224,7 @@ where
         let len = decode_varint(buf)?;
         let remaining = buf.remaining();
         if len > remaining as u64 {
-            return Err(DecodeError::new("buffer underflow"))
+            return Err(DecodeError::BufferUnderflow { required: len as usize })
         }
 
         let limit = remaining - len as usize;
diff --git a/src/error.rs b/src/error.rs
index eab88a8..dbd6d15 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -7,17 +7,22 @@ use std::io;
 
 /// A Protobuf message decoding error.
 ///
-/// `DecodeError` indicates that the input buffer does not caontain a valid
+/// `DecodeError` indicates that the input buffer does not contain a valid
 /// Protobuf message. The error details should be considered 'best effort': in
 /// general it is not possible to exactly pinpoint why data is malformed.
 #[derive(Clone, Debug, PartialEq, Eq)]
-pub struct DecodeError {
-    /// A 'best effort' root cause description.
-    description: Cow<'static, str>,
-    /// A stack of (message, field) name pairs, which identify the specific
-    /// message type and field where decoding failed. The stack contains an
-    /// entry per level of nesting.
-    stack: Vec<(&'static str, &'static str)>,
+pub enum DecodeError {
+    BufferUnderflow {
+        required: usize,
+    },
+    DecodeError {
+        /// A 'best effort' root cause description.
+        description: Cow<'static, str>,
+        /// A stack of (message, field) name pairs, which identify the specific
+        /// message type and field where decoding failed. The stack contains an
+        /// entry per level of nesting.
+        stack: Vec<(&'static str, &'static str)>,
+    },
 }
 
 impl DecodeError {
@@ -27,7 +32,7 @@ impl DecodeError {
     /// Meant to be used only by `Message` implementations.
     #[doc(hidden)]
     pub fn new<S>(description: S) -> DecodeError where S: Into<Cow<'static, str>> {
-        DecodeError {
+        DecodeError::DecodeError {
             description: description.into(),
             stack: Vec::new(),
         }
@@ -38,23 +43,36 @@ impl DecodeError {
     /// Meant to be used only by `Message` implementations.
     #[doc(hidden)]
     pub fn push(&mut self, message: &'static str, field: &'static str) {
-        self.stack.push((message, field));
+        match self {
+            DecodeError::DecodeError { ref mut stack, .. } => stack.push((message, field)),
+            _ => (),
+        }
     }
 }
 
 impl fmt::Display for DecodeError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        f.write_str("failed to decode Protobuf message: ")?;
-        for &(message, field) in &self.stack {
-            write!(f, "{}.{}: ", message, field)?;
+        match self {
+            DecodeError::DecodeError { description, stack } => {
+                f.write_str("failed to decode Protobuf message: ")?;
+                for &(message, field) in stack {
+                    write!(f, "{}.{}: ", message, field)?;
+                }
+                f.write_str(description)
+            },
+            DecodeError::BufferUnderflow { required } => {
+                write!(f, "required buffer size: {}", required)
+            },
         }
-        f.write_str(&self.description)
     }
 }
 
 impl error::Error for DecodeError {
     fn description(&self) -> &str {
-        &self.description
+        match self {
+            DecodeError::DecodeError { description, .. } => description,
+            DecodeError::BufferUnderflow { .. } => "buffer underflow",
+        }
     }
 }
 
 ```


Discussion / hints:
Hi @YetAnotherMinion, I've definitely felt this pain myself working with protobuf in an async context.  

The first thing I'd suggest is _not_ using the built-in length delimiting, and instead roll your own length delimiting with a fixed size integer using something like `my_buf.put_u32_le(my_message.encoded_len())`.  Variable-width length delimiters are a pain to work with in an async context, and should be avoided if you have control over the protocol.

That being said, I understand there exist (and have personally encountered) situations where variable length delimiting is used, and it's not possible to change the protocol.  I'm not keen on changing the `Decode` error type, since I find error-based flow control for non-exceptional situations pretty unpalatable, and I've found the existing field stack extremely useful in the past.  Instead, I propose that `prost` provide a few helper methods for working with length delimiters independently of messages.  I'm going to put up a PR shortly with what I'm thinking and link to it here.
> I find error-based flow control for non-exceptional situations pretty unpalatable

How *do* you handle parse errors then? The way I think of prost::decode is as a parser. I have some bytes that I would like to *parse* into a data structure. Parser libraries in languages that have a Result Monad universally use the Result::Error to report parse errors.

From reading the code, it appears to me that there are several different, mutually exclusive kinds of errors.
1) Decode length delimited buffer where the length indicated by header does not match length of buffer. This happens before any processing of the message bytes, and therefore does not need a stack
2) Parse error when decoding specific field. In this case you want a stack to report where in the message it occurred.
     1) The kind of error could be invalid varint
     2) Invalid wire type field
     3) Invalid key value
     4) Invalid tag value 0
     5) Buffer underflow
     6) Invalid string data not UTF-8 encoded

I don't care about matching on specific parse errors in case 2) because parsing a protobuf is really an all or nothing thing. I am not going to programmatically modify the input in response to the specific error.

On the other hand case 1) comes up clearly when preforming async network operations. When case 1) error occurs, I *can* programmatically modify the input to fix the problem. While it is possible for me compare the description string, I would greatly prefer to use the type system. That way changes in library API (aka you change wording of error string) would be caught by the compiler instead of silently failing.

> Variable-width length delimiters are a pain to work with in an async context, and should be avoided if you have control over the protocol.

I can understand your perspective here as a general observation, but in this specific instance we are talking about a protocol (Protocol Buffers) which elusively uses variable length delimiters. That Rubicon has already been crossed. My understanding is the entire point of the protocol buffer format is to reduce wire size of messages by paying an acceptable runtime cost. 

> Instead, I propose that prost provide a few helper methods for working with length delimiters independently of messages. I'm going to put up a PR shortly with what I'm thinking and link to it here.

If you don't want to properly handle the possible usecases that a length delimited message is useful for, then don't expose that length delimited message API. That would be the simplest solution. Expose the encode and code varint methods and users can do their own length delimited message schemes to their hearts content. No need for extra helpers in the library.
```diff
diff --git a/src/lib.rs b/src/lib.rs
index d0dc88a..b11d0e2 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -13,5 +13,8 @@ mod types;
 #[doc(hidden)]
 pub mod encoding;
 
+pub use encoding::encode_varint;
+pub use encoding::decode_varint;
+
 pub use message::Message;
 pub use error::{DecodeError, EncodeError}; 
```




Fix patch:
diff --git a/src/lib.rs b/src/lib.rs
index d0dc88ac3..cd78770ad 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -15,3 +15,59 @@ pub mod encoding;
 
 pub use message::Message;
 pub use error::{DecodeError, EncodeError};
+
+use bytes::{
+    BufMut,
+    IntoBuf,
+};
+
+use encoding::{
+    decode_varint,
+    encode_varint,
+    encoded_len_varint,
+};
+
+/// Encodes a length delimiter to the buffer.
+///
+/// See [Message.encode_length_delimited] for more info.
+///
+/// An error will be returned if the buffer does not have sufficient capacity to encode the
+/// delimiter.
+pub fn encode_length_delimiter<B>(length: usize, buf: &mut B) -> Result<(), EncodeError> where B: BufMut {
+    let length = length as u64;
+    let required = encoded_len_varint(length);
+    let remaining = buf.remaining_mut();
+    if required > remaining {
+        return Err(EncodeError::new(required, remaining));
+    }
+    encode_varint(length, buf);
+    Ok(())
+}
+
+/// Returns the encoded length of a length delimiter.
+///
+/// Applications may use this method to ensure sufficient buffer capacity before calling
+/// `encode_length_delimiter`. The returned size will be between 1 and 10, inclusive.
+pub fn length_delimiter_len(length: usize) -> usize {
+    encoded_len_varint(length as u64)
+}
+
+/// Decodes a length delimiter from the buffer.
+///
+/// This method allows the length delimiter to be decoded independently of the message, when the
+/// message is encoded with [Message.encode_length_delimited].
+///
+/// An error may be returned in two cases:
+///
+///  * If the supplied buffer contains fewer than 10 bytes, then an error indicates that more
+///    input is required to decode the full delimiter.
+///  * If the supplied buffer contains more than 10 bytes, then the buffer contains an invalid
+///    delimiter, and typically the buffer should be considered corrupt.
+pub fn decode_length_delimiter<B>(buf: B) -> Result<usize, DecodeError> where B: IntoBuf {
+    let mut buf = buf.into_buf();
+    let length = decode_varint(&mut buf)?;
+    if length > usize::max_value() as u64 {
+        return Err(DecodeError::new("length delimiter exceeds maximum usize value"));
+    }
+    Ok(length as usize)
+}
diff --git a/src/message.rs b/src/message.rs
index 5648a9dd4..cce51da51 100644
--- a/src/message.rs
+++ b/src/message.rs
@@ -49,7 +49,7 @@ pub trait Message: Debug + Send + Sync {
         let required = len + encoded_len_varint(len as u64);
         let remaining = buf.remaining_mut();
         if required > remaining {
-            return Err(EncodeError::new(required, remaining))
+            return Err(EncodeError::new(required, remaining));
         }
         encode_varint(len as u64, buf);
         self.encode_raw(buf);


Test patch: