Skip to content

Commit

Permalink
Fix allocate_pstr randomly refusing to properly allocate memory
Browse files Browse the repository at this point in the history
This one was a toughie: it turns out that using `ptr::align_of()`` was
a bad idea, since the buffer in `Heap` itself is not aligned to
`Heap::heap_cell_alignment()`, so `ptr::align_of()` would sometimes
return lower values than expected.

That made for an heisenbug: if the alignment of the heap happened to be 4,
then the bug wouldn't trigger.
  • Loading branch information
adri326 committed Dec 29, 2024
1 parent d695198 commit eb16e54
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 23 deletions.
17 changes: 17 additions & 0 deletions src/heap_iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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);
Expand Down
50 changes: 27 additions & 23 deletions src/machine/heap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -353,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),
Expand Down Expand Up @@ -481,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> {
Expand Down Expand Up @@ -635,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;
Expand Down Expand Up @@ -810,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);
Expand Down Expand Up @@ -966,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;

Expand Down Expand Up @@ -1039,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;
Expand All @@ -1056,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() {
Expand Down

0 comments on commit eb16e54

Please sign in to comment.