Panic in BytesMut::split_to when out of bounds (#252) (#253)

Shared references assert immutability, so any concurrent access would be UB
disregarding data race concerns.

.travis.yml

@@ -39,6 +39,9 @@ matrix:
     # 128 bit numbers
     - env: EXTRA_ARGS="--features i128"
 
+    # `Either` impls
+    - env: EXTRA_ARGS="--features either"
+
     # WASM support
     - rust: beta
       script:

CHANGELOG.md

@@ -1,3 +1,20 @@
+# 0.4.12 (March 6, 2019)
+
+### Added
+- Implement `FromIterator<&'a u8>` for `BytesMut`/`Bytes` (#244).
+- Implement `Buf` for `VecDeque` (#249).
+
+# 0.4.11 (November 17, 2018)
+
+* Use raw pointers for potentially racy loads (#233).
+* Implement `BufRead` for `buf::Reader` (#232).
+* Documentation tweaks (#234).
+
+# 0.4.10 (September 4, 2018)
+
+* impl `Buf` and `BufMut` for `Either` (#225).
+* Add `Bytes::slice_ref` (#208).
+
 # 0.4.9 (July 12, 2018)
 
 * Add 128 bit number support behind a feature flag (#209).

Cargo.toml

@@ -1,11 +1,16 @@
 [package]
 
 name          = "bytes"
-version       = "0.4.9" # don't forget to update html_root_url
+# When releasing to crates.io:
+# - Update html_root_url.
+# - Update CHANGELOG.md.
+# - Update doc URL.
+# - Create "v0.4.x" git tag.
+version       = "0.4.12"
 license       = "MIT"
 authors       = ["Carl Lerche <[email protected]>"]
 description   = "Types and traits for working with bytes"
-documentation = "https://carllerche.github.io/bytes/bytes"
+documentation = "https://docs.rs/bytes/0.4.12/bytes"
 homepage      = "https://github.com/carllerche/bytes"
 repository    = "https://github.com/carllerche/bytes"
 readme        = "README.md"
@@ -26,6 +31,7 @@ features = ["i128"]
 byteorder = "1.1.0"
 iovec = "0.1"
 serde = { version = "1.0", optional = true }
+either = { version = "1.5", default-features = false, optional = true }
 
 [dev-dependencies]
 serde_test = "1.0"

README.md

@@ -5,7 +5,7 @@ A utility library for working with bytes.
 [![Crates.io](https://img.shields.io/crates/v/bytes.svg?maxAge=2592000)](https://crates.io/crates/bytes)
 [![Build Status](https://travis-ci.org/carllerche/bytes.svg?branch=master)](https://travis-ci.org/carllerche/bytes)
 
-[Documentation](https://carllerche.github.io/bytes/bytes/index.html)
+[Documentation](https://docs.rs/bytes/0.4.12/bytes/)
 
 ## Usage
 
@@ -13,7 +13,7 @@ To use `bytes`, first add this to your `Cargo.toml`:
 
 ```toml
 [dependencies]
-bytes = "0.4"
+bytes = "0.4.12"
 ```
 
 Next, add this to your crate:
@@ -30,7 +30,7 @@ Serde support is optional and disabled by default. To enable use the feature `se
 
 ```toml
 [dependencies]
-bytes = { version = "0.4", features = ["serde"] }
+bytes = { version = "0.4.12", features = ["serde"] }
 ```
 
 ## License

ci/tsan

@@ -9,6 +9,9 @@ race:arc*Weak*drop
 # rust runtime logic.
 race:std*mpsc_queue
 
+# Some test runtime races. Allocation should be race free
+race:alloc::alloc
+
 # Not sure why this is warning, but it is in the test harness and not the library.
 race:TestEvent*clone
 race:test::run_tests_console::*closure
@@ -19,3 +22,7 @@ race:__call_tls_dtors
 # `is_inline_or_static` is explicitly called concurrently without synchronization.
 # The safety explanation can be found in a comment.
 race:Inner::is_inline_or_static
+
+# This ignores a false positive caused by `thread::park()`/`thread::unpark()`.
+# See: https://github.com/rust-lang/rust/pull/54806#issuecomment-436193353
+race:pthread_cond_destroy

src/buf/buf.rs

@@ -91,7 +91,8 @@ pub trait Buf {
     fn remaining(&self) -> usize;
 
     /// Returns a slice starting at the current position and of length between 0
-    /// and `Buf::remaining()`.
+    /// and `Buf::remaining()`. Note that this *can* return shorter slice (this allows
+    /// non-continuous internal representation).
     ///
     /// This is a lower level function. Most operations are done with other
     /// functions.

src/buf/buf_mut.rs

@@ -121,7 +121,8 @@ pub trait BufMut {
     }
 
     /// Returns a mutable slice starting at the current BufMut position and of
-    /// length between 0 and `BufMut::remaining_mut()`.
+    /// length between 0 and `BufMut::remaining_mut()`. Note that this *can* be shorter than the
+    /// whole remainder of the buffer (this allows non-continuous implementation).
     ///
     /// This is a lower level function. Most operations are done with other
     /// functions.

src/buf/mod.rs

@@ -24,6 +24,7 @@ mod into_buf;
 mod iter;
 mod reader;
 mod take;
+mod vec_deque;
 mod writer;
 
 pub use self::buf::Buf;

src/buf/reader.rs

@@ -86,3 +86,12 @@ impl<B: Buf + Sized> io::Read for Reader<B> {
         Ok(len)
     }
 }
+
+impl<B: Buf + Sized> io::BufRead for Reader<B> {
+    fn fill_buf(&mut self) -> io::Result<&[u8]> {
+        Ok(self.buf.bytes())
+    }
+    fn consume(&mut self, amt: usize) {
+        self.buf.advance(amt)
+    }
+}

src/buf/vec_deque.rs

@@ -0,0 +1,39 @@
+use std::collections::VecDeque;
+
+use super::Buf;
+
+impl Buf for VecDeque<u8> {
+    fn remaining(&self) -> usize {
+        self.len()
+    }
+
+    fn bytes(&self) -> &[u8] {
+        let (s1, s2) = self.as_slices();
+        if s1.is_empty() {
+            s2
+        } else {
+            s1
+        }
+    }
+
+    fn advance(&mut self, cnt: usize) {
+        self.drain(..cnt);
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn hello_world() {
+        let mut buffer: VecDeque<u8> = VecDeque::new();
+        buffer.extend(b"hello world");
+        assert_eq!(11, buffer.remaining());
+        assert_eq!(b"hello world", buffer.bytes());
+        buffer.advance(6);
+        assert_eq!(b"world", buffer.bytes());
+        buffer.extend(b" piece");
+        assert_eq!(b"world piece" as &[u8], &buffer.collect::<Vec<u8>>()[..]);
+    }
+}

src/bytes.rs

@@ -273,7 +273,7 @@ pub struct BytesMut {
 // The rest of `arc`'s bytes are used as part of the inline buffer, which means
 // that those bytes need to be located next to the `ptr`, `len`, and `cap`
 // fields, which make up the rest of the inline buffer. This requires special
-// casing the layout of `Inner` depending on if the target platform is bit or
+// casing the layout of `Inner` depending on if the target platform is big or
 // little endian.
 //
 // On little endian platforms, the `arc` field must be the first field in the
@@ -576,6 +576,46 @@ impl Bytes {
         self.slice(0, end)
     }
 
+    /// Returns a slice of self that is equivalent to the given `subset`.
+    ///
+    /// When processing a `Bytes` buffer with other tools, one often gets a
+    /// `&[u8]` which is in fact a slice of the `Bytes`, i.e. a subset of it.
+    /// This function turns that `&[u8]` into another `Bytes`, as if one had
+    /// called `self.slice()` with the offsets that correspond to `subset`.
+    ///
+    /// This operation is `O(1)`.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use bytes::Bytes;
+    ///
+    /// let bytes = Bytes::from(&b"012345678"[..]);
+    /// let as_slice = bytes.as_ref();
+    /// let subset = &as_slice[2..6];
+    /// let subslice = bytes.slice_ref(&subset);
+    /// assert_eq!(&subslice[..], b"2345");
+    /// ```
+    ///
+    /// # Panics
+    ///
+    /// Requires that the given `sub` slice is in fact contained within the
+    /// `Bytes` buffer; otherwise this function will panic.
+    pub fn slice_ref(&self, subset: &[u8]) -> Bytes {
+        let bytes_p = self.as_ptr() as usize;
+        let bytes_len = self.len();
+
+        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);
+
+        let sub_offset = sub_p - bytes_p;
+
+        self.slice(sub_offset, sub_offset + sub_len)
+    }
+
     /// Splits the bytes into two at the given index.
     ///
     /// Afterwards `self` contains elements `[0, at)`, and the returned `Bytes`
@@ -886,6 +926,18 @@ impl FromIterator<u8> for Bytes {
     }
 }
 
+impl<'a> FromIterator<&'a u8> for BytesMut {
+    fn from_iter<T: IntoIterator<Item = &'a u8>>(into_iter: T) -> Self {
+        BytesMut::from_iter(into_iter.into_iter().map(|b| *b))
+    }
+}
+
+impl<'a> FromIterator<&'a u8> for Bytes {
+    fn from_iter<T: IntoIterator<Item = &'a u8>>(into_iter: T) -> Self {
+        BytesMut::from_iter(into_iter).freeze()
+    }
+}
+
 impl PartialEq for Bytes {
     fn eq(&self, other: &Bytes) -> bool {
         self.inner.as_ref() == other.inner.as_ref()
@@ -1213,6 +1265,8 @@ impl BytesMut {
     ///
     /// Panics if `at > len`.
     pub fn split_to(&mut self, at: usize) -> BytesMut {
+        assert!(at <= self.len());
+
         BytesMut {
             inner: self.inner.split_to(at),
         }
@@ -2389,6 +2443,10 @@ impl Inner {
         // bits, so even without any explicit atomic operations, reading the
         // flag will be correct.
         //
+        // This is undefind behavior due to a data race, but experimental
+        // evidence shows that it works in practice (discussion:
+        // https://internals.rust-lang.org/t/bit-wise-reasoning-for-atomic-accesses/8853).
+        //
         // This function is very critical performance wise as it is called for
         // every operation. Performing an atomic load would mess with the
         // compiler's ability to optimize. Simple benchmarks show up to a 10%
@@ -2398,7 +2456,7 @@ impl Inner {
         #[inline]
         fn imp(arc: &AtomicPtr<Shared>) -> usize {
             unsafe {
-                let p: &u8 = mem::transmute(arc);
+                let p: *const u8 = mem::transmute(arc);
                 (*p as usize) & KIND_MASK
             }
         }
@@ -2407,7 +2465,7 @@ impl Inner {
         #[inline]
         fn imp(arc: &AtomicPtr<Shared>) -> usize {
             unsafe {
-                let p: &usize = mem::transmute(arc);
+                let p: *const usize = mem::transmute(arc);
                 *p & KIND_MASK
             }
         }
@@ -2423,7 +2481,7 @@ impl Inner {
         // function.
         let prev = unsafe {
             let p: &AtomicPtr<Shared> = &self.arc;
-            let p: &usize = mem::transmute(p);
+            let p: *const usize = mem::transmute(p);
             *p
         };
 
@@ -2530,35 +2588,51 @@ fn original_capacity_from_repr(repr: usize) -> usize {
 
 #[test]
 fn test_original_capacity_to_repr() {
-    for &cap in &[0, 1, 16, 1000] {
-        assert_eq!(0, original_capacity_to_repr(cap));
-    }
+    assert_eq!(original_capacity_to_repr(0), 0);
 
-    for &cap in &[1024, 1025, 1100, 2000, 2047] {
-        assert_eq!(1, original_capacity_to_repr(cap));
-    }
+    let max_width = 32;
 
-    for &cap in &[2048, 2049] {
-        assert_eq!(2, original_capacity_to_repr(cap));
-    }
+    for width in 1..(max_width + 1) {
+        let cap = 1 << width - 1;
 
-    // TODO: more
+        let expected = if width < MIN_ORIGINAL_CAPACITY_WIDTH {
+            0
+        } else if width < MAX_ORIGINAL_CAPACITY_WIDTH {
+            width - MIN_ORIGINAL_CAPACITY_WIDTH
+        } else {
+            MAX_ORIGINAL_CAPACITY_WIDTH - MIN_ORIGINAL_CAPACITY_WIDTH
+        };
 
-    for &cap in &[65536, 65537, 68000, 1 << 17, 1 << 18, 1 << 20, 1 << 30] {
-        assert_eq!(7, original_capacity_to_repr(cap), "cap={}", cap);
+        assert_eq!(original_capacity_to_repr(cap), expected);
+
+        if width > 1 {
+            assert_eq!(original_capacity_to_repr(cap + 1), expected);
+        }
+
+        //  MIN_ORIGINAL_CAPACITY_WIDTH must be bigger than 7 to pass tests below
+        if width == MIN_ORIGINAL_CAPACITY_WIDTH + 1 {
+            assert_eq!(original_capacity_to_repr(cap - 24), expected - 1);
+            assert_eq!(original_capacity_to_repr(cap + 76), expected);
+        } else if width == MIN_ORIGINAL_CAPACITY_WIDTH + 2 {
+            assert_eq!(original_capacity_to_repr(cap - 1), expected - 1);
+            assert_eq!(original_capacity_to_repr(cap - 48), expected - 1);
+        }
     }
 }
 
 #[test]
 fn test_original_capacity_from_repr() {
     assert_eq!(0, original_capacity_from_repr(0));
-    assert_eq!(1024, original_capacity_from_repr(1));
-    assert_eq!(1024 * 2, original_capacity_from_repr(2));
-    assert_eq!(1024 * 4, original_capacity_from_repr(3));
-    assert_eq!(1024 * 8, original_capacity_from_repr(4));
-    assert_eq!(1024 * 16, original_capacity_from_repr(5));
-    assert_eq!(1024 * 32, original_capacity_from_repr(6));
-    assert_eq!(1024 * 64, original_capacity_from_repr(7));
+
+    let min_cap = 1 << MIN_ORIGINAL_CAPACITY_WIDTH;
+
+    assert_eq!(min_cap, original_capacity_from_repr(1));
+    assert_eq!(min_cap * 2, original_capacity_from_repr(2));
+    assert_eq!(min_cap * 4, original_capacity_from_repr(3));
+    assert_eq!(min_cap * 8, original_capacity_from_repr(4));
+    assert_eq!(min_cap * 16, original_capacity_from_repr(5));
+    assert_eq!(min_cap * 32, original_capacity_from_repr(6));
+    assert_eq!(min_cap * 64, original_capacity_from_repr(7));
 }
 
 unsafe impl Send for Inner {}

src/either.rs

@@ -0,0 +1,89 @@
+extern crate either;
+
+use {Buf, BufMut};
+
+use self::either::Either;
+use self::either::Either::*;
+use iovec::IoVec;
+
+impl<L, R> Buf for Either<L, R>
+where
+    L: Buf,
+    R: Buf,
+{
+    fn remaining(&self) -> usize {
+        match *self {
+            Left(ref b) => b.remaining(),
+            Right(ref b) => b.remaining(),
+        }
+    }
+
+    fn bytes(&self) -> &[u8] {
+        match *self {
+            Left(ref b) => b.bytes(),
+            Right(ref b) => b.bytes(),
+        }
+    }
+
+    fn bytes_vec<'a>(&'a self, dst: &mut [&'a IoVec]) -> usize {
+        match *self {
+            Left(ref b) => b.bytes_vec(dst),
+            Right(ref b) => b.bytes_vec(dst),
+        }
+    }
+
+    fn advance(&mut self, cnt: usize) {
+        match *self {
+            Left(ref mut b) => b.advance(cnt),
+            Right(ref mut b) => b.advance(cnt),
+        }
+    }
+
+    fn copy_to_slice(&mut self, dst: &mut [u8]) {
+        match *self {
+            Left(ref mut b) => b.copy_to_slice(dst),
+            Right(ref mut b) => b.copy_to_slice(dst),
+        }
+    }
+}
+
+impl<L, R> BufMut for Either<L, R>
+where
+    L: BufMut,
+    R: BufMut,
+{
+    fn remaining_mut(&self) -> usize {
+        match *self {
+            Left(ref b) => b.remaining_mut(),
+            Right(ref b) => b.remaining_mut(),
+        }
+    }
+
+    unsafe fn bytes_mut(&mut self) -> &mut [u8] {
+        match *self {
+            Left(ref mut b) => b.bytes_mut(),
+            Right(ref mut b) => b.bytes_mut(),
+        }
+    }
+
+    unsafe fn bytes_vec_mut<'a>(&'a mut self, dst: &mut [&'a mut IoVec]) -> usize {
+        match *self {
+            Left(ref mut b) => b.bytes_vec_mut(dst),
+            Right(ref mut b) => b.bytes_vec_mut(dst),
+        }
+    }
+
+    unsafe fn advance_mut(&mut self, cnt: usize) {
+        match *self {
+            Left(ref mut b) => b.advance_mut(cnt),
+            Right(ref mut b) => b.advance_mut(cnt),
+        }
+    }
+
+    fn put_slice(&mut self, src: &[u8]) {
+        match *self {
+            Left(ref mut b) => b.put_slice(src),
+            Right(ref mut b) => b.put_slice(src),
+        }
+    }
+}

src/lib.rs

@@ -69,7 +69,7 @@
 //! and `BufMut` are infallible.
 
 #![deny(warnings, missing_docs, missing_debug_implementations)]
-#![doc(html_root_url = "https://docs.rs/bytes/0.4.9")]
+#![doc(html_root_url = "https://docs.rs/bytes/0.4.12")]
 
 extern crate byteorder;
 extern crate iovec;
@@ -99,3 +99,7 @@ pub use byteorder::{ByteOrder, BigEndian, LittleEndian};
 #[cfg(feature = "serde")]
 #[doc(hidden)]
 pub mod serde;
+
+// Optional `Either` support
+#[cfg(feature = "either")]
+mod either;

tests/test_bytes.rs

@@ -258,15 +258,10 @@ fn split_to_oob_mut() {
 }
 
 #[test]
+#[should_panic]
 fn split_to_uninitialized() {
     let mut bytes = BytesMut::with_capacity(1024);
-    let other = bytes.split_to(128);
-
-    assert_eq!(bytes.len(), 0);
-    assert_eq!(bytes.capacity(), 896);
-
-    assert_eq!(other.len(), 0);
-    assert_eq!(other.capacity(), 128);
+    let _other = bytes.split_to(128);
 }
 
 #[test]
@@ -717,3 +712,57 @@ fn from_iter_no_size_hint() {
 
     assert_eq!(&actual[..], &expect[..]);
 }
+
+fn test_slice_ref(bytes: &Bytes, start: usize, end: usize, expected: &[u8]) {
+    let slice = &(bytes.as_ref()[start..end]);
+    let sub = bytes.slice_ref(&slice);
+    assert_eq!(&sub[..], expected);
+}
+
+#[test]
+fn slice_ref_works() {
+    let bytes = Bytes::from(&b"012345678"[..]);
+
+    test_slice_ref(&bytes, 0, 0, b"");
+    test_slice_ref(&bytes, 0, 3, b"012");
+    test_slice_ref(&bytes, 2, 6, b"2345");
+    test_slice_ref(&bytes, 7, 9, b"78");
+    test_slice_ref(&bytes, 9, 9, b"");
+}
+
+
+#[test]
+fn slice_ref_empty() {
+    let bytes = Bytes::from(&b""[..]);
+    let slice = &(bytes.as_ref()[0..0]);
+
+    let sub = bytes.slice_ref(&slice);
+    assert_eq!(&sub[..], b"");
+}
+
+#[test]
+#[should_panic]
+fn slice_ref_catches_not_a_subset() {
+    let bytes = Bytes::from(&b"012345678"[..]);
+    let slice = &b"012345"[0..4];
+
+    bytes.slice_ref(slice);
+}
+
+#[test]
+#[should_panic]
+fn slice_ref_catches_not_an_empty_subset() {
+    let bytes = Bytes::from(&b"012345678"[..]);
+    let slice = &b""[0..0];
+
+    bytes.slice_ref(slice);
+}
+
+#[test]
+#[should_panic]
+fn empty_slice_ref_catches_not_an_empty_subset() {
+    let bytes = Bytes::from(&b""[..]);
+    let slice = &b""[0..0];
+
+    bytes.slice_ref(slice);
+}

tests/test_reader.rs

@@ -0,0 +1,28 @@
+extern crate bytes;
+
+use std::io::{BufRead, Cursor, Read};
+
+use bytes::Buf;
+
+#[test]
+fn read() {
+    let buf1 = Cursor::new(b"hello ");
+    let buf2 = Cursor::new(b"world");
+    let buf = Buf::chain(buf1, buf2); // Disambiguate with Read::chain
+    let mut buffer = Vec::new();
+    buf.reader().read_to_end(&mut buffer).unwrap();
+    assert_eq!(b"hello world", &buffer[..]);
+}
+
+#[test]
+fn buf_read() {
+    let buf1 = Cursor::new(b"hell");
+    let buf2 = Cursor::new(b"o\nworld");
+    let mut reader = Buf::chain(buf1, buf2).reader();
+    let mut line = String::new();
+    reader.read_line(&mut line).unwrap();
+    assert_eq!("hello\n", &line);
+    line.clear();
+    reader.read_line(&mut line).unwrap();
+    assert_eq!("world", &line);
+}