[PATCH v1 proxmox 0/6] uninitialized memory allocations fixes

[PATCH v1 proxmox 0/6] uninitialized memory allocations fixes

[Robert Obkircher]

Fix multiple issues around uninitialized allocations and remove unused
buggy code.

All changes are in the proxmox repo. I chose the pbs mailing list 
because proxmox-io is mainly used there.

Robert Obkircher (6):
  uuid: avoid potential null dereference and memory leaks
  io: request zeroed memory instead of manually clearing it
  io: avoid potential null dereference and memory leak on error path
  io: remove boxed::uninitialized because it is unsound
  io: remove unused append_to_vec functions
  io: remove unused ByteVecExt trait with grow_ and resize_uninitialized

 proxmox-io/src/boxed.rs        |  19 +-----
 proxmox-io/src/read.rs         |  44 ++-----------
 proxmox-io/src/vec/byte_vec.rs | 115 ---------------------------------
 proxmox-io/src/vec/mod.rs      |  37 ++++++-----
 proxmox-uuid/src/lib.rs        |  14 ++--
 5 files changed, 34 insertions(+), 195 deletions(-)
 delete mode 100644 proxmox-io/src/vec/byte_vec.rs

-- 
2.47.3

[PATCH v1 proxmox 1/6] uuid: avoid potential null dereference and memory leaks

[Robert Obkircher]

Avoid null pointer dereferences on allocation failures and do not leak
memory on the error paths.

I'm also not fully convinced that writing to uninitialized memory via
assingment instead of ptr::write was guaranteed to be safe, but Miri
doesn't complain about it. See the link for some additional context.

Link: https://github.com/rust-lang/unsafe-code-guidelines/issues/346
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 proxmox-uuid/src/lib.rs | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/proxmox-uuid/src/lib.rs b/proxmox-uuid/src/lib.rs
index f2658ec6..59750a3a 100644
--- a/proxmox-uuid/src/lib.rs
+++ b/proxmox-uuid/src/lib.rs
@@ -61,10 +61,9 @@ pub struct Uuid(Box<[u8; 16]>);
 impl Uuid {
     /// Generate a uuid with `uuid_generate(3)`.
     pub fn generate() -> Self {
-        use std::alloc::{Layout, alloc};
-        let uuid = unsafe { alloc(Layout::new::<[u8; 16]>()) as *mut [u8; 16] };
-        unsafe { uuid_generate(uuid) };
-        Self(unsafe { Box::from_raw(uuid) })
+        let mut uuid = Box::new_uninit();
+        unsafe { uuid_generate(uuid.as_mut_ptr()) };
+        Self(unsafe { uuid.assume_init() })
     }
 
     /// Get a reference to the internal 16 byte array.
@@ -92,12 +91,10 @@ impl Uuid {
     /// assert_eq!(uuid1, uuid2);
     /// ```
     pub fn parse_str(src: &str) -> Result<Self, UuidError> {
-        use std::alloc::{Layout, alloc};
-        let uuid: *mut [u8; 16] = unsafe { alloc(Layout::new::<[u8; 16]>()) as *mut [u8; 16] };
+        let mut uuid = [0; 16];
         if src.len() == 36 {
             // Unfortunately the manpage of `uuid_parse(3)` states that it technically requires a
             // terminating null byte at the end, which we don't have, so do this manually:
-            let uuid: &mut [u8] = unsafe { &mut (&mut *uuid)[..] };
             let src = src.as_bytes();
             if src[8] != b'-' || src[13] != b'-' || src[18] != b'-' || src[23] != b'-' {
                 return Err(UuidError);
@@ -118,7 +115,6 @@ impl Uuid {
                 uuid[i] = (hex_digit(src[2 * i + 4])? << 4) | hex_digit(src[2 * i + 5])?;
             }
         } else if src.len() == 32 {
-            let uuid: &mut [u8] = unsafe { &mut (&mut *uuid)[..] };
             let src = src.as_bytes();
             for i in 0..16 {
                 uuid[i] = (hex_digit(src[2 * i])? << 4) | hex_digit(src[2 * i + 1])?;
@@ -126,7 +122,7 @@ impl Uuid {
         } else {
             return Err(UuidError);
         }
-        Ok(Self(unsafe { Box::from_raw(uuid) }))
+        Ok(Self(Box::new(uuid)))
     }
 }
 
-- 
2.47.3

[PATCH v1 proxmox 2/6] io: request zeroed memory instead of manually clearing it

[Robert Obkircher]

A quick comparison using cargo bench showed no notable differences in
release mode, but in debug mode 1 GiB allocations by themselves got
43879 times faster. This is because the standard library continued to
call alloc_zeroed, matching release mode, while the previous version
actually had to touch the memory. With an additional .fill(1) the
speedup shrank to around 1.09x.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 proxmox-io/src/boxed.rs   | 7 ++-----
 proxmox-io/src/vec/mod.rs | 6 +-----
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/proxmox-io/src/boxed.rs b/proxmox-io/src/boxed.rs
index 7af51cfc..fb67bfd7 100644
--- a/proxmox-io/src/boxed.rs
+++ b/proxmox-io/src/boxed.rs
@@ -12,9 +12,6 @@ pub fn uninitialized(len: usize) -> Box<[u8]> {
 
 /// Zero-initialized bytes, meant for large sizes to avoid busting the stack.
 pub fn zeroed(len: usize) -> Box<[u8]> {
-    let mut bytes = uninitialized(len);
-    unsafe {
-        std::ptr::write_bytes(bytes.as_mut_ptr(), 0, bytes.len());
-    }
-    bytes
+    // vec! guarantees not to over-allocate, so shrink_to_fit inside into_boxed_slice does nothing
+    vec![0; len].into_boxed_slice()
 }
diff --git a/proxmox-io/src/vec/mod.rs b/proxmox-io/src/vec/mod.rs
index b68fc750..7fe106e8 100644
--- a/proxmox-io/src/vec/mod.rs
+++ b/proxmox-io/src/vec/mod.rs
@@ -69,11 +69,7 @@ pub fn clear(data: &mut [u8]) {
 /// Create a newly allocated, zero initialized byte vector.
 #[inline]
 pub fn zeroed(len: usize) -> Vec<u8> {
-    unsafe {
-        let mut out = uninitialized(len);
-        clear(&mut out);
-        out
-    }
+    vec![0; len]
 }
 
 /// Create a newly allocated byte vector of a specific size with "undefined" content.
-- 
2.47.3

[PATCH v1 proxmox 3/6] io: avoid potential null dereference and memory leak on error path

[Robert Obkircher]

Note that using uninitialized memory is still highly unsafe, because
reading from it can cause miscompilations. The remaining uses should
be cleaned up eventually, possibly when a better API like read_buf is
stabilized.

Link: https://godbolt.org/z/3xTnnYxnj
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 proxmox-io/src/read.rs    | 12 ++++++------
 proxmox-io/src/vec/mod.rs | 22 ++++++++++++++++++----
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/proxmox-io/src/read.rs b/proxmox-io/src/read.rs
index 67a608dc..0a73eea8 100644
--- a/proxmox-io/src/read.rs
+++ b/proxmox-io/src/read.rs
@@ -285,15 +285,15 @@ impl<R: io::Read> ReadExt for R {
     }
 
     unsafe fn read_host_value_boxed<T>(&mut self) -> io::Result<Box<T>> {
-        // FIXME: Change this once #![feature(new_uninit)] lands for Box<T>!
-
         unsafe {
-            let ptr = std::alloc::alloc(std::alloc::Layout::new::<T>()) as *mut T;
+            let mut result = Box::<T>::new_uninit();
+            // If this reads uninitialized memory from result or lies about
+            // having written to it that would be undefined behavior.
             self.read_exact(std::slice::from_raw_parts_mut(
-                ptr as *mut u8,
-                mem::size_of::<T>(),
+                result.as_mut_ptr().cast::<u8>(),
+                size_of::<T>(),
             ))?;
-            Ok(Box::from_raw(ptr))
+            Ok(result.assume_init())
         }
     }
 
diff --git a/proxmox-io/src/vec/mod.rs b/proxmox-io/src/vec/mod.rs
index 7fe106e8..e4cc9892 100644
--- a/proxmox-io/src/vec/mod.rs
+++ b/proxmox-io/src/vec/mod.rs
@@ -47,13 +47,27 @@ pub use byte_vec::ByteVecExt;
 ///
 /// # Safety
 ///
-/// It's generally not unsafe to use this method, but the contents are uninitialized, and since
-/// this does not return a `MaybeUninit` type to track the initialization state, this is simply
-/// marked as unsafe for good measure.
+/// It is unsafe to use this method because reading uninitialized memory is
+/// undefined behavior and allows the compiler to do anything. It can delete
+/// code-paths leading to such reads and optimize (x == x + 1) to true.
+///
+/// The following example prints "hello 0 0" with opt-level=3:
+/// ```rust
+/// unsafe {
+///     let ptr = std::alloc::alloc(std::alloc::Layout::array::<u8>(42).unwrap());
+///     let data = Box::from_raw(std::ptr::slice_from_raw_parts_mut(ptr, 42));
+///     let x = data[0];
+///     println!("hello {} {}", x, x + 1); // prints hello 0 0
+/// }
+/// ```
 #[inline]
 pub unsafe fn uninitialized(len: usize) -> Vec<u8> {
     unsafe {
-        let data = std::alloc::alloc(std::alloc::Layout::array::<u8>(len).unwrap());
+        let layout = std::alloc::Layout::array::<u8>(len).unwrap();
+        let data = std::alloc::alloc(layout);
+        if data.is_null() {
+            std::alloc::handle_alloc_error(layout);
+        }
         Vec::from_raw_parts(data, len, len)
     }
 }
-- 
2.47.3

[PATCH v1 proxmox 4/6] io: remove boxed::uninitialized because it is unsound

[Robert Obkircher]

Using this function could result in miscompilations, and it was also
missing a null check. Remove it, since it appears to be unused.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 proxmox-io/src/boxed.rs | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/proxmox-io/src/boxed.rs b/proxmox-io/src/boxed.rs
index fb67bfd7..abd60a1f 100644
--- a/proxmox-io/src/boxed.rs
+++ b/proxmox-io/src/boxed.rs
@@ -1,15 +1,3 @@
-/// Uninitialized bytes, without `MaybeUninit`.
-///
-/// We're talking about bytes here, which we *allocate*. That is, we call a function, get a pointer
-/// to stuff, and can use it. It's not UB in that there's nothing undefined about what's going on
-/// here.
-pub fn uninitialized(len: usize) -> Box<[u8]> {
-    unsafe {
-        let data = std::alloc::alloc(std::alloc::Layout::array::<u8>(len).unwrap());
-        Box::from_raw(std::ptr::slice_from_raw_parts_mut(data, len))
-    }
-}
-
 /// Zero-initialized bytes, meant for large sizes to avoid busting the stack.
 pub fn zeroed(len: usize) -> Box<[u8]> {
     // vec! guarantees not to over-allocate, so shrink_to_fit inside into_boxed_slice does nothing
-- 
2.47.3

[PATCH v1 proxmox 5/6] io: remove unused append_to_vec functions

[Robert Obkircher]

These functions leaked uninitialized bytes on the error paths, which
could cause undefined behavior or return sensitive data from memory.
Remove them for now, as they appear to be unused.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 proxmox-io/src/read.rs         | 32 +-------------------------------
 proxmox-io/src/vec/byte_vec.rs | 14 --------------
 2 files changed, 1 insertion(+), 45 deletions(-)

diff --git a/proxmox-io/src/read.rs b/proxmox-io/src/read.rs
index 0a73eea8..2cced5e0 100644
--- a/proxmox-io/src/read.rs
+++ b/proxmox-io/src/read.rs
@@ -5,7 +5,7 @@ use std::mem;
 
 use endian_trait::Endian;
 
-use crate::vec::{self, ByteVecExt};
+use crate::vec;
 
 /// Adds some additional related functionality for types implementing [`Read`](std::io::Read).
 ///
@@ -22,9 +22,6 @@ use crate::vec::{self, ByteVecExt};
 /// // read some bytes into a newly allocated Vec<u8>:
 /// let mut data = file.read_exact_allocated(64)?;
 ///
-/// // appending data to a vector:
-/// let actually_appended = file.append_to_vec(&mut data, 64)?; // .read() version
-/// file.append_exact_to_vec(&mut data, 64)?; // .read_exact() version
 /// # Ok(())
 /// # }
 /// ```
@@ -71,12 +68,6 @@ pub trait ReadExt {
     /// ```
     fn read_exact_allocated(&mut self, size: usize) -> io::Result<Vec<u8>>;
 
-    /// Append data to a vector, growing it as necessary. Returns the amount of data appended.
-    fn append_to_vec(&mut self, out: &mut Vec<u8>, size: usize) -> io::Result<usize>;
-
-    /// Append an exact amount of data to a vector, growing it as necessary.
-    fn append_exact_to_vec(&mut self, out: &mut Vec<u8>, size: usize) -> io::Result<()>;
-
     /// Read a value with host endianness.
     ///
     /// This is limited to types implementing the [`Endian`] trait under the assumption that
@@ -244,27 +235,6 @@ impl<R: io::Read> ReadExt for R {
         Ok(out)
     }
 
-    fn append_to_vec(&mut self, out: &mut Vec<u8>, size: usize) -> io::Result<usize> {
-        let pos = out.len();
-        unsafe {
-            out.grow_uninitialized(size);
-        }
-        let got = self.read(&mut out[pos..])?;
-        unsafe {
-            out.set_len(pos + got);
-        }
-        Ok(got)
-    }
-
-    fn append_exact_to_vec(&mut self, out: &mut Vec<u8>, size: usize) -> io::Result<()> {
-        let pos = out.len();
-        unsafe {
-            out.grow_uninitialized(size);
-        }
-        self.read_exact(&mut out[pos..])?;
-        Ok(())
-    }
-
     unsafe fn read_host_value<T: Endian>(&mut self) -> io::Result<T> {
         let mut value = std::mem::MaybeUninit::<T>::uninit();
         unsafe {
diff --git a/proxmox-io/src/vec/byte_vec.rs b/proxmox-io/src/vec/byte_vec.rs
index e8d1962b..0a45ba0a 100644
--- a/proxmox-io/src/vec/byte_vec.rs
+++ b/proxmox-io/src/vec/byte_vec.rs
@@ -25,13 +25,6 @@
 /// # }
 /// ```
 ///
-/// Note that this module also provides a safe alternative for the case where
-/// `grow_uninitialized()` is directly followed by a `read_exact()` call via the [`ReadExt`]
-/// trait:
-/// ```ignore
-/// file.append_to_vec(&mut data, 1024)?;
-/// ```
-///
 /// [`ReadExt`]: crate::ReadExt
 pub trait ByteVecExt {
     /// Grow a vector without initializing its elements. The difference to simply using `reserve`
@@ -57,13 +50,6 @@ pub trait ByteVecExt {
     /// # }
     /// ```
     ///
-    /// Although for the above case it is recommended to use the even shorter version from the
-    /// [`ReadExt`] trait:
-    /// ```ignore
-    /// // use crate::tools::vec::ByteVecExt;
-    /// file.append_to_vec(&mut buffer, 1024)?;
-    /// ```
-    ///
     /// # Safety
     ///
     /// When increasing the size, the new contents are uninitialized and have nothing to do with
-- 
2.47.3

[PATCH v1 proxmox 6/6] io: remove unused ByteVecExt trait with grow_ and resize_uninitialized

[Robert Obkircher]

These methods appear to be unused now, and in cases where dealing with
uninitialized memory is necessary, it would be more appropriate to use
safer methods like Vec::spare_capacity_mut.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 proxmox-io/src/vec/byte_vec.rs | 101 ---------------------------------
 proxmox-io/src/vec/mod.rs      |   9 +--
 2 files changed, 1 insertion(+), 109 deletions(-)
 delete mode 100644 proxmox-io/src/vec/byte_vec.rs

diff --git a/proxmox-io/src/vec/byte_vec.rs b/proxmox-io/src/vec/byte_vec.rs
deleted file mode 100644
index 0a45ba0a..00000000
--- a/proxmox-io/src/vec/byte_vec.rs
+++ /dev/null
@@ -1,101 +0,0 @@
-//! This module provides additional operations for `Vec<u8>`.
-//!
-//! Example:
-//! ```
-//! # use std::io::Read;
-//! use proxmox_io::vec::{self, ByteVecExt};
-//!
-//! fn append_1024_to_vec<T: Read>(mut input: T, buffer: &mut Vec<u8>) -> std::io::Result<()> {
-//!     input.read_exact(unsafe { buffer.grow_uninitialized(1024) })
-//! }
-//! ```
-
-/// Some additional byte vector operations useful for I/O code.
-/// Example:
-/// ```
-/// # use std::io::Read;
-/// # use proxmox_io::ReadExt;
-/// use proxmox_io::vec::{self, ByteVecExt};
-///
-/// # fn code(mut file: std::fs::File, mut data: Vec<u8>) -> std::io::Result<()> {
-/// file.read_exact(unsafe {
-///     data.grow_uninitialized(1024)
-/// })?;
-/// # Ok(())
-/// # }
-/// ```
-///
-/// [`ReadExt`]: crate::ReadExt
-pub trait ByteVecExt {
-    /// Grow a vector without initializing its elements. The difference to simply using `reserve`
-    /// is that it also updates the actual length, making the newly allocated data part of the
-    /// slice.
-    ///
-    /// This is a shortcut for:
-    /// ```ignore
-    /// vec.reserve(more);
-    /// let total = vec.len() + more;
-    /// unsafe {
-    ///     vec.set_len(total);
-    /// }
-    /// ```
-    ///
-    /// This returns a mutable slice to the newly allocated space, so it can be used inline:
-    /// ```
-    /// # use std::io::Read;
-    /// # use proxmox_io::vec::ByteVecExt;
-    /// # fn test(mut file: std::fs::File, buffer: &mut Vec<u8>) -> std::io::Result<()> {
-    /// file.read_exact(unsafe { buffer.grow_uninitialized(1024) })?;
-    /// # Ok(())
-    /// # }
-    /// ```
-    ///
-    /// # Safety
-    ///
-    /// When increasing the size, the new contents are uninitialized and have nothing to do with
-    /// the previously contained content. Since we cannot track this state through the type system,
-    /// this method is marked as an unsafe API for good measure.
-    ///
-    /// [`ReadExt`]: crate::ReadExt
-    unsafe fn grow_uninitialized(&mut self, more: usize) -> &mut [u8];
-
-    /// Resize a vector to a specific size without initializing its data. This is a shortcut for:
-    /// ```ignore
-    /// if new_size <= vec.len() {
-    ///     vec.truncate(new_size);
-    /// } else {
-    ///     unsafe {
-    ///         vec.grow_uninitialized(new_size - vec.len());
-    ///     }
-    /// }
-    /// ```
-    ///
-    /// # Safety
-    ///
-    /// When increasing the size, the new contents are uninitialized and have nothing to do with
-    /// the previously contained content. Since we cannot track this state through the type system,
-    /// this method is marked as an unsafe API for good measure.
-    unsafe fn resize_uninitialized(&mut self, total: usize);
-}
-
-impl ByteVecExt for Vec<u8> {
-    unsafe fn grow_uninitialized(&mut self, more: usize) -> &mut [u8] {
-        let old_len = self.len();
-        self.reserve(more);
-        let total = old_len + more;
-        unsafe {
-            self.set_len(total);
-        }
-        &mut self[old_len..]
-    }
-
-    unsafe fn resize_uninitialized(&mut self, new_size: usize) {
-        if new_size <= self.len() {
-            self.truncate(new_size);
-        } else {
-            unsafe {
-                self.grow_uninitialized(new_size - self.len());
-            }
-        }
-    }
-}
diff --git a/proxmox-io/src/vec/mod.rs b/proxmox-io/src/vec/mod.rs
index e4cc9892..94f87735 100644
--- a/proxmox-io/src/vec/mod.rs
+++ b/proxmox-io/src/vec/mod.rs
@@ -17,7 +17,7 @@
 //!
 //! Examples:
 //! ```no_run
-//! use proxmox_io::vec::{self, ByteVecExt};
+//! use proxmox_io::vec;
 //!
 //! # let size = 64usize;
 //! # let more = 64usize;
@@ -25,15 +25,8 @@
 //!
 //! let mut buffer = unsafe { vec::uninitialized(size) }; // an actually uninitialized buffer
 //! vec::clear(&mut buffer); // zero out an &mut [u8]
-//!
-//! vec::clear(unsafe {
-//!     buffer.grow_uninitialized(more) // grow the buffer with uninitialized bytes
-//! });
 //! ```
 
-mod byte_vec;
-pub use byte_vec::ByteVecExt;
-
 /// Create an uninitialized byte vector of a specific size.
 ///
 /// This is just a shortcut for:
-- 
2.47.3