Fix UTF-8 unsoundness in string::merge (#194)

* Merge string value without as_mut_vec unsoundness

In case of an encoding error in string::merge, the appended string value
is left with broken UTF-8 in case of an error. The same can happen if
any of the Buf methods panics. This results in UB if the string value is used
after the error return or in unwind, respectively.

Change the implementation to truncate the string back to valid UTF-8
content in any code path that does not go through validation of the
newly appended bytes.

* Unit test for string::merge failure

Test that adding an invalid UTF-8 sequence results in an error,
and that the string is reverted to its state prior to the merge call.

* Safe reimplementation of string::merge

As suggested by Dan Burkert:
https://github.com/danburkert/prost/pull/194#discussion_r292096009

The existing string value is dropped in case of a merge failure,
but this is better than exposing an invalid value.

src/encoding.rs

@@ -3,7 +3,7 @@
 //! Meant to be used only from `Message` implementations.
 
 use std::cmp::min;
-use std::str;
+use std::mem;
 use std::u32;
 use std::usize;
 
@@ -728,14 +728,10 @@ pub mod string {
     where
         B: Buf,
     {
-        unsafe {
-            // String::as_mut_vec is unsafe because it doesn't check that the bytes
-            // inserted into it the resulting vec are valid UTF-8. We check
-            // explicitly in order to ensure this is safe.
-            super::bytes::merge(wire_type, value.as_mut_vec(), buf)?;
-            str::from_utf8(value.as_bytes())
-                .map_err(|_| DecodeError::new("invalid string value: data is not UTF-8 encoded"))?;
-        }
+        let mut value_bytes = mem::replace(value, String::new()).into_bytes();
+        bytes::merge(wire_type, &mut value_bytes, buf)?;
+        *value = String::from_utf8(value_bytes)
+            .map_err(|_| DecodeError::new("invalid string value: data is not UTF-8 encoded"))?;
         Ok(())
     }
 
@@ -1307,6 +1303,15 @@ mod test {
         }
     }
 
+    #[test]
+    fn string_merge_failure() {
+        let mut s = String::new();
+        let mut buf = Cursor::new(b"\x80\x80");
+        let r = string::merge(WireType::LengthDelimited, &mut s, &mut buf);
+        r.expect_err("must be an error");
+        assert!(s.is_empty());
+    }
+
     #[test]
     fn varint() {
         fn check(value: u64, encoded: &[u8]) {