Merge pull request #2729 from adri326/rebis-dev-fix-miri

rebis-dev: Fix UB detected by miri in tests

Author: mthom

src/heap_iter.rs

@@ -2979,6 +2979,19 @@ mod tests {
 
         wam.machine_st.heap.clear();
 
+    }
+
+    #[test]
+    fn heap_stackless_post_order_iter_pstr() {
+        let mut wam = MockWAM::new();
+
+        let f_atom = atom!("f");
+        let a_atom = atom!("a");
+        let b_atom = atom!("b");
+
+        // clear the heap of resource error data etc
+        wam.machine_st.heap.clear();
+
         // first a 'dangling' partial string, later modified to be a
         // two-part complete string, then a three-part cyclic string
         // involving an uncompacted list of chars.
@@ -3004,9 +3017,13 @@ mod tests {
         }
 
         wam.machine_st.heap[2] = heap_loc_as_cell!(2);
+        assert_eq!(wam.machine_st.heap.cell_len(), 3);
+
         wam.machine_st.allocate_pstr("def").unwrap();
+        assert_eq!(wam.machine_st.heap.cell_len(), 4);
 
         wam.machine_st.heap.push_cell(pstr_loc_as_cell!(0)).unwrap();
+        assert_eq!(wam.machine_st.heap.cell_len(), 5);
 
         {
             let mut iter = stackless_post_order_iter(&mut wam.machine_st.heap, 4);

src/machine/heap.rs

@@ -24,17 +24,29 @@ pub struct Heap {
 
 impl Drop for Heap {
     fn drop(&mut self) {
-        unsafe {
-            let layout = alloc::Layout::array::<u8>(self.inner.byte_cap).unwrap();
-            alloc::dealloc(self.inner.ptr, layout);
+        if !self.inner.ptr.is_null() {
+            unsafe {
+                let layout = alloc::Layout::array::<u8>(self.inner.byte_cap).unwrap();
+                alloc::dealloc(self.inner.ptr, layout);
+            }
         }
     }
 }
 
+// TODO: verify the soundness of the various accesses to `ptr`,
+// or rely on a Vec-like library with fallible allocations.
 #[derive(Debug)]
 struct InnerHeap {
     ptr: *mut u8,
+
+    /// # Safety
+    ///
+    /// Must be equal to zero when `ptr.is_null()`.
     byte_len: usize,
+
+    /// # Safety
+    ///
+    /// Must be equal to zero when `ptr.is_null()`.
     byte_cap: usize,
 }
 
@@ -341,15 +353,7 @@ impl<'a> ReservedHeapSection<'a> {
 
             let zero_region_idx = heap_index!(self.heap_cell_len) + str_byte_len;
 
-            let align_offset = self.heap_ptr
-                .add(zero_region_idx)
-                .align_offset(ALIGN_CELL);
-
-            let align_offset = if align_offset == 0 {
-                ALIGN_CELL
-            } else {
-                align_offset
-            };
+            let align_offset = pstr_sentinel_length(zero_region_idx);
 
             ptr::write_bytes(
                 self.heap_ptr.add(zero_region_idx),
@@ -469,6 +473,22 @@ impl<'a> Index<usize> for ReservedHeapSection<'a> {
     }
 }
 
+/// Computes the number of bytes required to pad a string of length `chunk_len`
+/// with zeroes, such that `chunk_len + pstr_sentinel_length(chunk_len)` is a
+/// multiple of `Heap::heap_cell_alignement()`.
+fn pstr_sentinel_length(chunk_len: usize) -> usize {
+    const ALIGN: usize = Heap::heap_cell_alignment();
+
+    let res = chunk_len.next_multiple_of(ALIGN) - chunk_len;
+
+    // No bytes available in last chunk
+    if res == 0 {
+        ALIGN
+    } else {
+        res
+    }
+}
+
 #[must_use]
 #[derive(Debug)]
 pub struct HeapWriter<'a> {
@@ -623,7 +643,7 @@ impl Heap {
                 if self.free_space() >= len {
                     section = ReservedHeapSection {
                         heap_ptr: self.inner.ptr,
-                        heap_cell_len: cell_index!(self.inner.byte_len),
+                        heap_cell_len: self.cell_len(),
                         pstr_vec: &mut self.pstr_vec,
                     };
                     break;
@@ -798,6 +818,11 @@ impl Heap {
                 }
             }
 
+            // SAFETY:
+            // - Postcondition: from `self.grow()`, `self.inner.byte_len + size_of::<HeapCellValue>()`
+            //   is strictly less than `self.inner.byte_cap`.
+            // - Asserted: `self.cell_len() * size_of::<HeapCellvalue>() <= self.inner.byte_cap`.
+            // - Invariant: from `InnerHeap`, `self.inner.byte_cap < isize::MAX`.
             let cell_ptr = (self.inner.ptr as *mut HeapCellValue).add(self.cell_len());
             cell_ptr.write(cell);
             self.pstr_vec.push(false);
@@ -954,17 +979,7 @@ impl Heap {
 
         const ALIGN_CELL: usize = Heap::heap_cell_alignment();
 
-        let align_offset = unsafe {
-            self.inner.ptr
-                .add(self.inner.byte_len + s_len)
-                .align_offset(ALIGN_CELL)
-        };
-
-        let align_offset = if align_offset == 0 {
-            ALIGN_CELL
-        } else {
-            align_offset
-        };
+        let align_offset = pstr_sentinel_length(s_len);
 
         let copy_size = s_len + align_offset;
 
@@ -1027,7 +1042,8 @@ impl Heap {
         Ok(())
     }
 
-    pub(crate) const fn compute_pstr_size(src: &str) -> usize {
+    /// Returns the number of bytes needed to store `src` as a `PStr`.
+    pub(crate) fn compute_pstr_size(src: &str) -> usize {
         const ALIGN_CELL: usize = Heap::heap_cell_alignment();
 
         let mut byte_size = 0;
@@ -1044,7 +1060,7 @@ impl Heap {
                 null_idx += 1;
             }
 
-            byte_size += (null_idx & !(ALIGN_CELL - 1)) + ALIGN_CELL;
+            byte_size += null_idx.next_multiple_of(ALIGN_CELL);
             byte_size += mem::size_of::<HeapCellValue>();
 
             if null_idx + 1 >= src.len() {