v0.5.3

Closes #340

CHANGELOG.md

@@ -1,3 +1,12 @@
+# 0.5.3 (December 12, 2019)
+
+### Added
+- `must_use` attributes to `split`, `split_off`, and `split_to` methods (#337).
+
+### Fix
+- Potential freeing of a null pointer in `Bytes` when constructed with an empty `Vec<u8>` (#341, #342).
+- Calling `Bytes::truncate` with a size large than the length will no longer clear the `Bytes` (#333).
+
 # 0.5.2 (November 27, 2019)
 
 ### Added

Cargo.toml

@@ -6,9 +6,12 @@ name = "bytes"
 # - Update CHANGELOG.md.
 # - Update doc URL.
 # - Create "v0.5.x" git tag.
-version = "0.5.2"
+version = "0.5.3"
 license = "MIT"
-authors = ["Carl Lerche <[email protected]>"]
+authors = [
+    "Carl Lerche <[email protected]>",
+    "Sean McArthur <[email protected]>",
+]
 description = "Types and traits for working with bytes"
 documentation = "https://docs.rs/bytes"
 repository = "https://github.com/tokio-rs/bytes"

src/buf/buf_impl.rs

@@ -809,7 +809,8 @@ pub trait Buf {
     }
 }
 
-impl<T: Buf + ?Sized> Buf for &mut T {
+macro_rules! deref_forward_buf {
+    () => (
     fn remaining(&self) -> usize {
         (**self).remaining()
     }
@@ -826,25 +827,100 @@ impl<T: Buf + ?Sized> Buf for &mut T {
     fn advance(&mut self, cnt: usize) {
         (**self).advance(cnt)
     }
+
+    fn has_remaining(&self) -> bool {
+        (**self).has_remaining()
+    }
+
+    fn copy_to_slice(&mut self, dst: &mut [u8]) {
+        (**self).copy_to_slice(dst)
+    }
+
+    fn get_u8(&mut self) -> u8 {
+        (**self).get_u8()
+    }
+
+    fn get_i8(&mut self) -> i8 {
+        (**self).get_i8()
+    }
+
+    fn get_u16(&mut self) -> u16 {
+        (**self).get_u16()
+    }
+
+    fn get_u16_le(&mut self) -> u16 {
+        (**self).get_u16_le()
+    }
+
+    fn get_i16(&mut self) -> i16 {
+        (**self).get_i16()
+    }
+
+    fn get_i16_le(&mut self) -> i16 {
+        (**self).get_i16_le()
+    }
+
+    fn get_u32(&mut self) -> u32 {
+        (**self).get_u32()
+    }
+
+    fn get_u32_le(&mut self) -> u32 {
+        (**self).get_u32_le()
+    }
+
+    fn get_i32(&mut self) -> i32 {
+        (**self).get_i32()
+    }
+
+    fn get_i32_le(&mut self) -> i32 {
+        (**self).get_i32_le()
+    }
+
+    fn get_u64(&mut self) -> u64 {
+        (**self).get_u64()
+    }
+
+    fn get_u64_le(&mut self) -> u64 {
+        (**self).get_u64_le()
+    }
+
+    fn get_i64(&mut self) -> i64 {
+        (**self).get_i64()
+    }
+
+    fn get_i64_le(&mut self) -> i64 {
+        (**self).get_i64_le()
+    }
+
+    fn get_uint(&mut self, nbytes: usize) -> u64 {
+        (**self).get_uint(nbytes)
+    }
+
+    fn get_uint_le(&mut self, nbytes: usize) -> u64 {
+        (**self).get_uint_le(nbytes)
+    }
+
+    fn get_int(&mut self, nbytes: usize) -> i64 {
+        (**self).get_int(nbytes)
+    }
+
+    fn get_int_le(&mut self, nbytes: usize) -> i64 {
+        (**self).get_int_le(nbytes)
+    }
+
+    fn to_bytes(&mut self) -> crate::Bytes {
+        (**self).to_bytes()
+    }
+
+    )
+}
+
+impl<T: Buf + ?Sized> Buf for &mut T {
+    deref_forward_buf!();
 }
 
 impl<T: Buf + ?Sized> Buf for Box<T> {
-    fn remaining(&self) -> usize {
-        (**self).remaining()
-    }
-
-    fn bytes(&self) -> &[u8] {
-        (**self).bytes()
-    }
-
-    #[cfg(feature = "std")]
-    fn bytes_vectored<'b>(&'b self, dst: &mut [IoSlice<'b>]) -> usize {
-        (**self).bytes_vectored(dst)
-    }
-
-    fn advance(&mut self, cnt: usize) {
-        (**self).advance(cnt)
-    }
+    deref_forward_buf!();
 }
 
 impl Buf for &[u8] {

src/buf/buf_mut.rs

@@ -871,7 +871,8 @@ pub trait BufMut {
     }
 }
 
-impl<T: BufMut + ?Sized> BufMut for &mut T {
+macro_rules! deref_forward_bufmut {
+    () => (
     fn remaining_mut(&self) -> usize {
         (**self).remaining_mut()
     }
@@ -888,25 +889,75 @@ impl<T: BufMut + ?Sized> BufMut for &mut T {
     unsafe fn advance_mut(&mut self, cnt: usize) {
         (**self).advance_mut(cnt)
     }
+
+    fn put_slice(&mut self, src: &[u8]) {
+        (**self).put_slice(src)
+    }
+
+    fn put_u8(&mut self, n: u8) {
+        (**self).put_u8(n)
+    }
+
+    fn put_i8(&mut self, n: i8) {
+        (**self).put_i8(n)
+    }
+
+    fn put_u16(&mut self, n: u16) {
+        (**self).put_u16(n)
+    }
+
+    fn put_u16_le(&mut self, n: u16) {
+        (**self).put_u16_le(n)
+    }
+
+    fn put_i16(&mut self, n: i16) {
+        (**self).put_i16(n)
+    }
+
+    fn put_i16_le(&mut self, n: i16) {
+        (**self).put_i16_le(n)
+    }
+
+    fn put_u32(&mut self, n: u32) {
+        (**self).put_u32(n)
+    }
+
+    fn put_u32_le(&mut self, n: u32) {
+        (**self).put_u32_le(n)
+    }
+
+    fn put_i32(&mut self, n: i32) {
+        (**self).put_i32(n)
+    }
+
+    fn put_i32_le(&mut self, n: i32) {
+        (**self).put_i32_le(n)
+    }
+
+    fn put_u64(&mut self, n: u64) {
+        (**self).put_u64(n)
+    }
+
+    fn put_u64_le(&mut self, n: u64) {
+        (**self).put_u64_le(n)
+    }
+
+    fn put_i64(&mut self, n: i64) {
+        (**self).put_i64(n)
+    }
+
+    fn put_i64_le(&mut self, n: i64) {
+        (**self).put_i64_le(n)
+    }
+    )
+}
+
+impl<T: BufMut + ?Sized> BufMut for &mut T {
+    deref_forward_bufmut!();
 }
 
 impl<T: BufMut + ?Sized> BufMut for Box<T> {
-    fn remaining_mut(&self) -> usize {
-        (**self).remaining_mut()
-    }
-
-    fn bytes_mut(&mut self) -> &mut [MaybeUninit<u8>] {
-        (**self).bytes_mut()
-    }
-
-    #[cfg(feature = "std")]
-    fn bytes_vectored_mut<'b>(&'b mut self, dst: &mut [IoSliceMut<'b>]) -> usize {
-        (**self).bytes_vectored_mut(dst)
-    }
-
-    unsafe fn advance_mut(&mut self, cnt: usize) {
-        (**self).advance_mut(cnt)
-    }
+    deref_forward_bufmut!();
 }
 
 impl BufMut for &mut [u8] {
@@ -964,6 +1015,31 @@ impl BufMut for Vec<u8> {
             &mut slice::from_raw_parts_mut(ptr, cap)[len..]
         }
     }
+
+    // Specialize these methods so they can skip checking `remaining_mut`
+    // and `advance_mut`.
+
+    fn put<T: super::Buf>(&mut self, mut src: T) where Self: Sized {
+        // In case the src isn't contiguous, reserve upfront
+        self.reserve(src.remaining());
+
+        while src.has_remaining() {
+            let l;
+
+            // a block to contain the src.bytes() borrow
+            {
+                let s = src.bytes();
+                l = s.len();
+                self.extend_from_slice(s);
+            }
+
+            src.advance(l);
+        }
+    }
+
+    fn put_slice(&mut self, src: &[u8]) {
+        self.extend_from_slice(src);
+    }
 }
 
 // The existence of this function makes the compiler catch if the BufMut

src/bytes.rs

@@ -257,8 +257,20 @@ impl Bytes {
         let sub_p = subset.as_ptr() as usize;
         let sub_len = subset.len();
 
-        assert!(sub_p >= bytes_p);
-        assert!(sub_p + sub_len <= bytes_p + bytes_len);
+        assert!(
+            sub_p >= bytes_p,
+            "subset pointer ({:p}) is smaller than self pointer ({:p})",
+            sub_p as *const u8,
+            bytes_p as *const u8,
+        );
+        assert!(
+            sub_p + sub_len <= bytes_p + bytes_len,
+            "subset is out of bounds: self = ({:p}, {}), subset = ({:p}, {})",
+            bytes_p as *const u8,
+            bytes_len,
+            sub_p as *const u8,
+            sub_len,
+        );
 
         let sub_offset = sub_p - bytes_p;
 
@@ -288,6 +300,7 @@ impl Bytes {
     /// # Panics
     ///
     /// Panics if `at > len`.
+    #[must_use = "consider Bytes::truncate if you don't need the other half"]
     pub fn split_off(&mut self, at: usize) -> Bytes {
         assert!(at <= self.len());
 
@@ -331,6 +344,7 @@ impl Bytes {
     /// # Panics
     ///
     /// Panics if `at > len`.
+    #[must_use = "consider Bytes::advance if you don't need the other half"]
     pub fn split_to(&mut self, at: usize) -> Bytes {
         assert!(at <= self.len());
 
@@ -373,9 +387,7 @@ impl Bytes {
     /// [`split_off`]: #method.split_off
     #[inline]
     pub fn truncate(&mut self, len: usize) {
-        if len >= self.len {
-            self.len = 0;
-        } else {
+        if len < self.len {
             self.len = len;
         }
     }
@@ -709,9 +721,22 @@ impl From<&'static str> for Bytes {
 
 impl From<Vec<u8>> for Bytes {
     fn from(vec: Vec<u8>) -> Bytes {
+        // into_boxed_slice doesn't return a heap allocation for empty vectors,
+        // so the pointer isn't aligned enough for the KIND_VEC stashing to
+        // work.
+        if vec.is_empty() {
+            return Bytes::new();
+        }
+
         let slice = vec.into_boxed_slice();
         let len = slice.len();
         let ptr = slice.as_ptr();
+
+        assert!(
+            ptr as usize & KIND_VEC == 0,
+            "Vec pointer should not have LSB set: {:p}",
+            ptr,
+        );
         drop(Box::into_raw(slice));
 
         let data = ptr as usize | KIND_VEC;
@@ -801,7 +826,15 @@ unsafe fn shared_drop(data: &mut AtomicPtr<()>, ptr: *const u8, len: usize) {
 }
 
 unsafe fn rebuild_vec(shared: *const (), offset: *const u8, len: usize) -> Vec<u8> {
-    debug_assert_eq!(shared as usize & KIND_MASK, KIND_VEC);
+    debug_assert!(
+        shared as usize & KIND_MASK == KIND_VEC,
+        "rebuild_vec should have beeen called with KIND_VEC",
+    );
+    debug_assert!(
+        shared as usize & !KIND_MASK != 0,
+        "rebuild_vec should be called with non-null pointer: {:p}",
+        shared,
+    );
 
     let buf = (shared as usize & !KIND_MASK) as *mut u8;
     let cap = (offset as usize - buf as usize) + len;
@@ -915,6 +948,28 @@ unsafe fn release_shared(ptr: *mut Shared) {
     Box::from_raw(ptr);
 }
 
+// compile-fails
+
+/// ```compile_fail
+/// use bytes::Bytes;
+/// #[deny(unused_must_use)]
+/// {
+///     let mut b1 = Bytes::from("hello world");
+///     b1.split_to(6);
+/// }
+/// ```
+fn _split_to_must_use() {}
+
+/// ```compile_fail
+/// use bytes::Bytes;
+/// #[deny(unused_must_use)]
+/// {
+///     let mut b1 = Bytes::from("hello world");
+///     b1.split_off(6);
+/// }
+/// ```
+fn _split_off_must_use() {}
+
 // fuzz tests
 #[cfg(all(test, loom))]
 mod fuzz {

src/bytes_mut.rs

@@ -275,6 +275,7 @@ impl BytesMut {
     /// # Panics
     ///
     /// Panics if `at > capacity`.
+    #[must_use = "consider BytesMut::truncate if you don't need the other half"]
     pub fn split_off(&mut self, at: usize) -> BytesMut {
         assert!(at <= self.capacity());
         unsafe {
@@ -310,6 +311,7 @@ impl BytesMut {
     ///
     /// assert_eq!(other, b"hello world"[..]);
     /// ```
+    #[must_use = "consider BytesMut::advance(len()) if you don't need the other half"]
     pub fn split(&mut self) -> BytesMut {
         let len = self.len();
         self.split_to(len)
@@ -341,6 +343,7 @@ impl BytesMut {
     /// # Panics
     ///
     /// Panics if `at > len`.
+    #[must_use = "consider BytesMut::advance if you don't need the other half"]
     pub fn split_to(&mut self, at: usize) -> BytesMut {
         assert!(at <= self.len());
 
@@ -657,8 +660,22 @@ impl BytesMut {
     /// assert_eq!(b"aaabbbcccddd", &buf[..]);
     /// ```
     pub fn extend_from_slice(&mut self, extend: &[u8]) {
-        self.reserve(extend.len());
-        self.put_slice(extend);
+        let cnt = extend.len();
+        self.reserve(cnt);
+
+        unsafe {
+            let dst = self.maybe_uninit_bytes();
+            // Reserved above
+            debug_assert!(dst.len() >= cnt);
+
+            ptr::copy_nonoverlapping(
+                extend.as_ptr(),
+                dst.as_mut_ptr() as *mut u8,
+                cnt);
+
+        }
+
+        unsafe { self.advance_mut(cnt); }
     }
 
     /// Combine splitted BytesMut objects back as contiguous.
@@ -873,6 +890,16 @@ impl BytesMut {
 
         self.data = ((pos << VEC_POS_OFFSET) | (prev & NOT_VEC_POS_MASK)) as *mut _;
     }
+
+    #[inline]
+    fn maybe_uninit_bytes(&mut self) -> &mut [mem::MaybeUninit<u8>] {
+        unsafe {
+            let ptr = self.ptr.as_ptr().offset(self.len as isize);
+            let len = self.cap - self.len;
+
+            slice::from_raw_parts_mut(ptr as *mut mem::MaybeUninit<u8>, len)
+        }
+    }
 }
 
 impl Drop for BytesMut {
@@ -932,14 +959,24 @@ impl BufMut for BytesMut {
         if self.capacity() == self.len() {
             self.reserve(64);
         }
+        self.maybe_uninit_bytes()
+    }
 
-        unsafe {
-            let ptr = self.ptr.as_ptr().offset(self.len as isize);
-            let len = self.cap - self.len;
+    // Specialize these methods so they can skip checking `remaining_mut`
+    // and `advance_mut`.
 
-            slice::from_raw_parts_mut(ptr as *mut mem::MaybeUninit<u8>, len)
+    fn put<T: crate::Buf>(&mut self, mut src: T) where Self: Sized {
+        while src.has_remaining() {
+            let s = src.bytes();
+            let l = s.len();
+            self.extend_from_slice(s);
+            src.advance(l);
         }
     }
+
+    fn put_slice(&mut self, src: &[u8]) {
+        self.extend_from_slice(src);
+    }
 }
 
 impl AsRef<[u8]> for BytesMut {
@@ -1428,6 +1465,38 @@ unsafe fn shared_v_drop(data: &mut AtomicPtr<()>, _ptr: *const u8, _len: usize)
     release_shared(shared as *mut Shared);
 }
 
+// compile-fails
+
+/// ```compile_fail
+/// use bytes::BytesMut;
+/// #[deny(unused_must_use)]
+/// {
+///     let mut b1 = BytesMut::from("hello world");
+///     b1.split_to(6);
+/// }
+/// ```
+fn _split_to_must_use() {}
+
+/// ```compile_fail
+/// use bytes::BytesMut;
+/// #[deny(unused_must_use)]
+/// {
+///     let mut b1 = BytesMut::from("hello world");
+///     b1.split_off(6);
+/// }
+/// ```
+fn _split_off_must_use() {}
+
+/// ```compile_fail
+/// use bytes::BytesMut;
+/// #[deny(unused_must_use)]
+/// {
+///     let mut b1 = BytesMut::from("hello world");
+///     b1.split();
+/// }
+/// ```
+fn _split_must_use() {}
+
 // fuzz tests
 #[cfg(all(test, loom))]
 mod fuzz {

src/lib.rs

@@ -1,5 +1,5 @@
 #![deny(warnings, missing_docs, missing_debug_implementations, rust_2018_idioms)]
-#![doc(html_root_url = "https://docs.rs/bytes/0.5.2")]
+#![doc(html_root_url = "https://docs.rs/bytes/0.5.3")]
 #![no_std]
 
 //! Provides abstractions for working with bytes.

tests/test_buf.rs

@@ -69,3 +69,33 @@ fn test_vec_deque() {
     buffer.copy_to_slice(&mut out);
     assert_eq!(b"world piece", &out[..]);
 }
+
+#[test]
+fn test_deref_buf_forwards() {
+    struct Special;
+
+    impl Buf for Special {
+        fn remaining(&self) -> usize {
+            unreachable!("remaining");
+        }
+
+        fn bytes(&self) -> &[u8] {
+            unreachable!("bytes");
+        }
+
+        fn advance(&mut self, _: usize) {
+            unreachable!("advance");
+        }
+
+        fn get_u8(&mut self) -> u8 {
+            // specialized!
+            b'x'
+        }
+    }
+
+    // these should all use the specialized method
+    assert_eq!(Special.get_u8(), b'x');
+    assert_eq!((&mut Special as &mut dyn Buf).get_u8(), b'x');
+    assert_eq!((Box::new(Special) as Box<dyn Buf>).get_u8(), b'x');
+    assert_eq!(Box::new(Special).get_u8(), b'x');
+}

tests/test_buf_mut.rs

@@ -87,3 +87,32 @@ fn test_mut_slice() {
     let mut s = &mut v[..];
     s.put_u32(42);
 }
+
+#[test]
+fn test_deref_bufmut_forwards() {
+    struct Special;
+
+    impl BufMut for Special {
+        fn remaining_mut(&self) -> usize {
+            unreachable!("remaining_mut");
+        }
+
+        fn bytes_mut(&mut self) -> &mut [std::mem::MaybeUninit<u8>] {
+            unreachable!("bytes_mut");
+        }
+
+        unsafe fn advance_mut(&mut self, _: usize) {
+            unreachable!("advance");
+        }
+
+        fn put_u8(&mut self, _: u8) {
+            // specialized!
+        }
+    }
+
+    // these should all use the specialized method
+    Special.put_u8(b'x');
+    (&mut Special as &mut dyn BufMut).put_u8(b'x');
+    (Box::new(Special) as Box<dyn BufMut>).put_u8(b'x');
+    Box::new(Special).put_u8(b'x');
+}

tests/test_bytes.rs

@@ -176,7 +176,7 @@ fn split_off() {
 #[should_panic]
 fn split_off_oob() {
     let mut hello = Bytes::from(&b"helloworld"[..]);
-    hello.split_off(44);
+    let _ = hello.split_off(44);
 }
 
 #[test]
@@ -273,14 +273,14 @@ fn split_to_2() {
 #[should_panic]
 fn split_to_oob() {
     let mut hello = Bytes::from(&b"helloworld"[..]);
-    hello.split_to(33);
+    let _ = hello.split_to(33);
 }
 
 #[test]
 #[should_panic]
 fn split_to_oob_mut() {
     let mut hello = BytesMut::from(&b"helloworld"[..]);
-    hello.split_to(33);
+    let _ = hello.split_to(33);
 }
 
 #[test]
@@ -300,18 +300,30 @@ fn split_off_to_at_gt_len() {
 
     use std::panic;
 
-    make_bytes().split_to(4);
-    make_bytes().split_off(4);
+    let _ = make_bytes().split_to(4);
+    let _ = make_bytes().split_off(4);
 
     assert!(panic::catch_unwind(move || {
-        make_bytes().split_to(5);
+        let _ = make_bytes().split_to(5);
     }).is_err());
 
     assert!(panic::catch_unwind(move || {
-        make_bytes().split_off(5);
+        let _ = make_bytes().split_off(5);
     }).is_err());
 }
 
+#[test]
+fn truncate() {
+    let s = &b"helloworld"[..];
+    let mut hello = Bytes::from(s);
+    hello.truncate(15);
+    assert_eq!(hello, s);
+    hello.truncate(10);
+    assert_eq!(hello, s);
+    hello.truncate(5);
+    assert_eq!(hello, "hello");
+}
+
 #[test]
 fn freeze_clone_shared() {
     let s = &b"abcdefgh"[..];
@@ -416,7 +428,7 @@ fn reserve_vec_recycling() {
 #[test]
 fn reserve_in_arc_unique_does_not_overallocate() {
     let mut bytes = BytesMut::with_capacity(1000);
-    bytes.split();
+    let _ = bytes.split();
 
     // now bytes is Arc and refcount == 1
 
@@ -428,7 +440,7 @@ fn reserve_in_arc_unique_does_not_overallocate() {
 #[test]
 fn reserve_in_arc_unique_doubles() {
     let mut bytes = BytesMut::with_capacity(1000);
-    bytes.split();
+    let _ = bytes.split();
 
     // now bytes is Arc and refcount == 1
 
@@ -711,12 +723,12 @@ fn bytes_mut_unsplit_arc_different() {
     let mut buf = BytesMut::with_capacity(64);
     buf.extend_from_slice(b"aaaabbbbeeee");
 
-    buf.split_off(8); //arc
+    let _ = buf.split_off(8); //arc
 
     let mut buf2 = BytesMut::with_capacity(64);
     buf2.extend_from_slice(b"ccccddddeeee");
 
-    buf2.split_off(8); //arc
+    let _ = buf2.split_off(8); //arc
 
     buf.unsplit(buf2);
     assert_eq!(b"aaaabbbbccccdddd", &buf[..]);
@@ -817,8 +829,17 @@ fn slice_ref_catches_not_an_empty_subset() {
 #[test]
 #[should_panic]
 fn empty_slice_ref_catches_not_an_empty_subset() {
-    let bytes = Bytes::copy_from_slice(&b""[..]);
-    let slice = &b""[0..0];
+    let bytes = Bytes::new();
+    let slice = &b"some other slice"[0..0];
+
+    // Protect this test against Bytes internals.
+    //
+    // This should panic *because* the slice's ptr doesn't fit in the range
+    // of the `bytes`.
+    if bytes.as_ptr() as usize == slice.as_ptr() as usize {
+        // don't panic, failing the test
+        return;
+    }
 
     bytes.slice_ref(slice);
 }
@@ -853,3 +874,10 @@ fn bytes_reserve_overflow() {
 
     bytes.reserve(usize::MAX);
 }
+
+#[test]
+fn bytes_with_capacity_but_empty() {
+    // See https://github.com/tokio-rs/bytes/issues/340
+    let vec = Vec::with_capacity(1);
+    let _ = Bytes::from(vec);
+}