From 897c8d0ab9d3cb2cf1c112f31c6ac3e93d9884bc Mon Sep 17 00:00:00 2001 From: Alex Saveau Date: Sun, 13 Feb 2022 15:04:11 -0800 Subject: Add debug asserts to validate NUL terminator in c strings Signed-off-by: Alex Saveau --- library/std/src/ffi/c_str.rs | 41 +++++++++++++++++++++++++++----------- library/std/src/ffi/c_str/tests.rs | 8 -------- 2 files changed, 29 insertions(+), 20 deletions(-) (limited to 'library/std/src') diff --git a/library/std/src/ffi/c_str.rs b/library/std/src/ffi/c_str.rs index c3f024026ef..b52cc97504a 100644 --- a/library/std/src/ffi/c_str.rs +++ b/library/std/src/ffi/c_str.rs @@ -382,7 +382,7 @@ impl CString { let bytes: Vec = self.into(); match memchr::memchr(0, &bytes) { Some(i) => Err(NulError(i, bytes)), - None => Ok(unsafe { CString::from_vec_unchecked(bytes) }), + None => Ok(unsafe { CString::_from_vec_unchecked(bytes) }), } } } @@ -405,7 +405,7 @@ impl CString { // This allows better optimizations if lto enabled. match memchr::memchr(0, bytes) { Some(i) => Err(NulError(i, buffer)), - None => Ok(unsafe { CString::from_vec_unchecked(buffer) }), + None => Ok(unsafe { CString::_from_vec_unchecked(buffer) }), } } @@ -451,10 +451,15 @@ impl CString { /// ``` #[must_use] #[stable(feature = "rust1", since = "1.0.0")] - pub unsafe fn from_vec_unchecked(mut v: Vec) -> CString { + pub unsafe fn from_vec_unchecked(v: Vec) -> Self { + debug_assert!(memchr::memchr(0, &v).is_none()); + unsafe { Self::_from_vec_unchecked(v) } + } + + unsafe fn _from_vec_unchecked(mut v: Vec) -> Self { v.reserve_exact(1); v.push(0); - CString { inner: v.into_boxed_slice() } + Self { inner: v.into_boxed_slice() } } /// Retakes ownership of a `CString` that was transferred to C via @@ -578,7 +583,7 @@ impl CString { pub fn into_string(self) -> Result { String::from_utf8(self.into_bytes()).map_err(|e| IntoStringError { error: e.utf8_error(), - inner: unsafe { CString::from_vec_unchecked(e.into_bytes()) }, + inner: unsafe { Self::_from_vec_unchecked(e.into_bytes()) }, }) } @@ -735,6 +740,11 @@ impl CString { #[must_use] #[stable(feature = "cstring_from_vec_with_nul", since = "1.58.0")] pub unsafe fn from_vec_with_nul_unchecked(v: Vec) -> Self { + debug_assert!(memchr::memchr(0, &v).unwrap() + 1 == v.len()); + unsafe { Self::_from_vec_with_nul_unchecked(v) } + } + + unsafe fn _from_vec_with_nul_unchecked(v: Vec) -> Self { Self { inner: v.into_boxed_slice() } } @@ -778,7 +788,7 @@ impl CString { Some(nul_pos) if nul_pos + 1 == v.len() => { // SAFETY: We know there is only one nul byte, at the end // of the vec. - Ok(unsafe { Self::from_vec_with_nul_unchecked(v) }) + Ok(unsafe { Self::_from_vec_with_nul_unchecked(v) }) } Some(nul_pos) => Err(FromVecWithNulError { error_kind: FromBytesWithNulErrorKind::InteriorNul(nul_pos), @@ -811,7 +821,7 @@ impl ops::Deref for CString { #[inline] fn deref(&self) -> &CStr { - unsafe { CStr::from_bytes_with_nul_unchecked(self.as_bytes_with_nul()) } + unsafe { CStr::_from_bytes_with_nul_unchecked(self.as_bytes_with_nul()) } } } @@ -922,7 +932,7 @@ impl From> for CString { }; // SAFETY: `v` cannot contain null bytes, given the type-level // invariant of `NonZeroU8`. - CString::from_vec_unchecked(v) + Self::_from_vec_unchecked(v) } } } @@ -1215,7 +1225,7 @@ impl CStr { unsafe { let len = sys::strlen(ptr); let ptr = ptr as *const u8; - CStr::from_bytes_with_nul_unchecked(slice::from_raw_parts(ptr, len as usize + 1)) + Self::_from_bytes_with_nul_unchecked(slice::from_raw_parts(ptr, len as usize + 1)) } } @@ -1258,7 +1268,7 @@ impl CStr { Some(nul_pos) if nul_pos + 1 == bytes.len() => { // SAFETY: We know there is only one nul byte, at the end // of the byte slice. - Ok(unsafe { Self::from_bytes_with_nul_unchecked(bytes) }) + Ok(unsafe { Self::_from_bytes_with_nul_unchecked(bytes) }) } Some(nul_pos) => Err(FromBytesWithNulError::interior_nul(nul_pos)), None => Err(FromBytesWithNulError::not_nul_terminated()), @@ -1287,12 +1297,19 @@ impl CStr { #[stable(feature = "cstr_from_bytes", since = "1.10.0")] #[rustc_const_stable(feature = "const_cstr_unchecked", since = "1.59.0")] pub const unsafe fn from_bytes_with_nul_unchecked(bytes: &[u8]) -> &CStr { + // We're in a const fn, so this is the best we can do + debug_assert!(!bytes.is_empty() && bytes[bytes.len() - 1] == 0); + unsafe { Self::_from_bytes_with_nul_unchecked(bytes) } + } + + #[inline] + const unsafe fn _from_bytes_with_nul_unchecked(bytes: &[u8]) -> &Self { // SAFETY: Casting to CStr is safe because its internal representation // is a [u8] too (safe only inside std). // Dereferencing the obtained pointer is safe because it comes from a // reference. Making a reference is then safe because its lifetime // is bound by the lifetime of the given `bytes`. - unsafe { &*(bytes as *const [u8] as *const CStr) } + unsafe { &*(bytes as *const [u8] as *const Self) } } /// Returns the inner pointer to this C string. @@ -1555,7 +1572,7 @@ impl ops::Index> for CStr { // byte, since otherwise we could get an empty string that doesn't end // in a null. if index.start < bytes.len() { - unsafe { CStr::from_bytes_with_nul_unchecked(&bytes[index.start..]) } + unsafe { CStr::_from_bytes_with_nul_unchecked(&bytes[index.start..]) } } else { panic!( "index out of bounds: the len is {} but the index is {}", diff --git a/library/std/src/ffi/c_str/tests.rs b/library/std/src/ffi/c_str/tests.rs index 4f7ba9ad437..00ba5460821 100644 --- a/library/std/src/ffi/c_str/tests.rs +++ b/library/std/src/ffi/c_str/tests.rs @@ -32,14 +32,6 @@ fn build_with_zero2() { assert!(CString::new(vec![0]).is_err()); } -#[test] -fn build_with_zero3() { - unsafe { - let s = CString::from_vec_unchecked(vec![0]); - assert_eq!(s.as_bytes(), b"\0"); - } -} - #[test] fn formatted() { let s = CString::new(&b"abc\x01\x02\n\xE2\x80\xA6\xFF"[..]).unwrap(); -- cgit 1.4.1-3-g733a5 From a677e6084049ff4cb2e338d7e33dc034846cdb29 Mon Sep 17 00:00:00 2001 From: Stein Somers Date: Fri, 14 Jan 2022 20:28:04 +0100 Subject: Collections: improve the documentation of drain members --- library/alloc/src/collections/binary_heap.rs | 16 +++++++++++----- library/alloc/src/collections/vec_deque/mod.rs | 20 ++++++++++++-------- library/alloc/src/string.rs | 17 ++++++++++++----- library/alloc/src/vec/mod.rs | 19 ++++++++++++------- library/std/src/collections/hash/map.rs | 4 ++++ library/std/src/collections/hash/set.rs | 7 ++++++- 6 files changed, 57 insertions(+), 26 deletions(-) (limited to 'library/std/src') diff --git a/library/alloc/src/collections/binary_heap.rs b/library/alloc/src/collections/binary_heap.rs index 56a47001811..e18cd8cd464 100644 --- a/library/alloc/src/collections/binary_heap.rs +++ b/library/alloc/src/collections/binary_heap.rs @@ -746,9 +746,12 @@ impl BinaryHeap { self.rebuild_tail(start); } - /// Returns an iterator which retrieves elements in heap order. - /// The retrieved elements are removed from the original heap. - /// The remaining elements will be removed on drop in heap order. + /// Clears the binary heap, returning an iterator over the removed elements + /// in heap order. If the iterator is dropped before being fully consumed, + /// it drops the remaining elements in heap order. + /// + /// The returned iterator keeps a mutable borrow on the heap to optimize + /// its implementation. /// /// Note: /// * `.drain_sorted()` is *O*(*n* \* log(*n*)); much slower than `.drain()`. @@ -1158,9 +1161,12 @@ impl BinaryHeap { self.len() == 0 } - /// Clears the binary heap, returning an iterator over the removed elements. + /// Clears the binary heap, returning an iterator over the removed elements + /// in arbitrary order. If the iterator is dropped before being fully + /// consumed, it drops the remaining elements in arbitrary order. /// - /// The elements are removed in arbitrary order. + /// The returned iterator keeps a mutable borrow on the heap to optimize + /// its implementation. /// /// # Examples /// diff --git a/library/alloc/src/collections/vec_deque/mod.rs b/library/alloc/src/collections/vec_deque/mod.rs index 763175fc045..7139a0fb94d 100644 --- a/library/alloc/src/collections/vec_deque/mod.rs +++ b/library/alloc/src/collections/vec_deque/mod.rs @@ -1215,21 +1215,25 @@ impl VecDeque { unsafe { IterMut::new(ring, tail, head, PhantomData) } } - /// Creates a draining iterator that removes the specified range in the - /// deque and yields the removed items. + /// Removes the specified range from the deque in bulk, returning all + /// removed elements as an iterator. If the iterator is dropped before + /// being fully consumed, it drops the remaining removed elements. /// - /// Note 1: The element range is removed even if the iterator is not - /// consumed until the end. + /// The returned iterator keeps a mutable borrow on the queue to optimize + /// its implementation. /// - /// Note 2: It is unspecified how many elements are removed from the deque, - /// if the `Drain` value is not dropped, but the borrow it holds expires - /// (e.g., due to `mem::forget`). /// /// # Panics /// /// Panics if the starting point is greater than the end point or if /// the end point is greater than the length of the deque. /// + /// # Leaking + /// + /// If the returned iterator goes out of scope without being dropped (due to + /// [`mem::forget`], for example), the deque may have lost and leaked + /// elements arbitrarily, including elements outside the range. + /// /// # Examples /// /// ``` @@ -1240,7 +1244,7 @@ impl VecDeque { /// assert_eq!(drained, [3]); /// assert_eq!(deque, [1, 2]); /// - /// // A full range clears all contents + /// // A full range clears all contents, like `clear()` does /// deque.drain(..); /// assert!(deque.is_empty()); /// ``` diff --git a/library/alloc/src/string.rs b/library/alloc/src/string.rs index 7c0faf0659a..716bb4983a6 100644 --- a/library/alloc/src/string.rs +++ b/library/alloc/src/string.rs @@ -1628,17 +1628,24 @@ impl String { self.vec.clear() } - /// Creates a draining iterator that removes the specified range in the `String` - /// and yields the removed `chars`. + /// Removes the specified range from the string in bulk, returning all + /// removed characters as an iterator. /// - /// Note: The element range is removed even if the iterator is not - /// consumed until the end. + /// The returned iterator keeps a mutable borrow on the string to optimize + /// its implementation. /// /// # Panics /// /// Panics if the starting point or end point do not lie on a [`char`] /// boundary, or if they're out of bounds. /// + /// # Leaking + /// + /// If the returned iterator goes out of scope without being dropped (due to + /// [`core::mem::forget`], for example), the string may still contain a copy + /// of any drained characters, or may have lost characters arbitrarily, + /// including characters outside the range. + /// /// # Examples /// /// Basic usage: @@ -1652,7 +1659,7 @@ impl String { /// assert_eq!(t, "α is alpha, "); /// assert_eq!(s, "β is beta"); /// - /// // A full range clears the string + /// // A full range clears the string, like `clear()` does /// s.drain(..); /// assert_eq!(s, ""); /// ``` diff --git a/library/alloc/src/vec/mod.rs b/library/alloc/src/vec/mod.rs index 3dc3eee4133..c29aa0fec5b 100644 --- a/library/alloc/src/vec/mod.rs +++ b/library/alloc/src/vec/mod.rs @@ -1799,19 +1799,24 @@ impl Vec { self.len += count; } - /// Creates a draining iterator that removes the specified range in the vector - /// and yields the removed items. + /// Removes the specified range from the vector in bulk, returning all + /// removed elements as an iterator. If the iterator is dropped before + /// being fully consumed, it drops the remaining removed elements. /// - /// When the iterator **is** dropped, all elements in the range are removed - /// from the vector, even if the iterator was not fully consumed. If the - /// iterator **is not** dropped (with [`mem::forget`] for example), it is - /// unspecified how many elements are removed. + /// The returned iterator keeps a mutable borrow on the vector to optimize + /// its implementation. /// /// # Panics /// /// Panics if the starting point is greater than the end point or if /// the end point is greater than the length of the vector. /// + /// # Leaking + /// + /// If the returned iterator goes out of scope without being dropped (due to + /// [`mem::forget`], for example), the vector may have lost and leaked + /// elements arbitrarily, including elements outside the range. + /// /// # Examples /// /// ``` @@ -1820,7 +1825,7 @@ impl Vec { /// assert_eq!(v, &[1]); /// assert_eq!(u, &[2, 3]); /// - /// // A full range clears the vector + /// // A full range clears the vector, like `clear()` does /// v.drain(..); /// assert_eq!(v, &[]); /// ``` diff --git a/library/std/src/collections/hash/map.rs b/library/std/src/collections/hash/map.rs index 9e61defc31e..c9d91d2c03b 100644 --- a/library/std/src/collections/hash/map.rs +++ b/library/std/src/collections/hash/map.rs @@ -547,6 +547,10 @@ impl HashMap { /// Clears the map, returning all key-value pairs as an iterator. Keeps the /// allocated memory for reuse. /// + /// If the returned iterator is dropped before being fully consumed, it + /// drops the remaining key-value pairs. The returned iterator keeps a + /// mutable borrow on the vector to optimize its implementation. + /// /// # Examples /// /// ``` diff --git a/library/std/src/collections/hash/set.rs b/library/std/src/collections/hash/set.rs index d1450987e73..200667ae390 100644 --- a/library/std/src/collections/hash/set.rs +++ b/library/std/src/collections/hash/set.rs @@ -227,7 +227,12 @@ impl HashSet { self.base.is_empty() } - /// Clears the set, returning all elements in an iterator. + /// Clears the set, returning all elements as an iterator. Keeps the + /// allocated memory for reuse. + /// + /// If the returned iterator is dropped before being fully consumed, it + /// drops the remaining elements. The returned iterator keeps a mutable + /// borrow on the vector to optimize its implementation. /// /// # Examples /// -- cgit 1.4.1-3-g733a5