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

[Robert Obkircher <r.obkircher@proxmox.com>]

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..0ffd9c80 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:
+/// ```no_run
+/// 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