Skip to content

Commit b5338a1

Browse files
adri326mthom
authored andcommitted
Fix allocate_pstr randomly refusing to properly allocate memory
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.
1 parent b00578d commit b5338a1

File tree

2 files changed

+45
-24
lines changed

2 files changed

+45
-24
lines changed

src/heap_iter.rs

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2979,6 +2979,19 @@ mod tests {
29792979

29802980
wam.machine_st.heap.clear();
29812981

2982+
}
2983+
2984+
#[test]
2985+
fn heap_stackless_post_order_iter_pstr() {
2986+
let mut wam = MockWAM::new();
2987+
2988+
let f_atom = atom!("f");
2989+
let a_atom = atom!("a");
2990+
let b_atom = atom!("b");
2991+
2992+
// clear the heap of resource error data etc
2993+
wam.machine_st.heap.clear();
2994+
29822995
// first a 'dangling' partial string, later modified to be a
29832996
// two-part complete string, then a three-part cyclic string
29842997
// involving an uncompacted list of chars.
@@ -3004,9 +3017,13 @@ mod tests {
30043017
}
30053018

30063019
wam.machine_st.heap[2] = heap_loc_as_cell!(2);
3020+
assert_eq!(wam.machine_st.heap.cell_len(), 3);
3021+
30073022
wam.machine_st.allocate_pstr("def").unwrap();
3023+
assert_eq!(wam.machine_st.heap.cell_len(), 4);
30083024

30093025
wam.machine_st.heap.push_cell(pstr_loc_as_cell!(0)).unwrap();
3026+
assert_eq!(wam.machine_st.heap.cell_len(), 5);
30103027

30113028
{
30123029
let mut iter = stackless_post_order_iter(&mut wam.machine_st.heap, 4);

src/machine/heap.rs

Lines changed: 28 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -353,15 +353,7 @@ impl<'a> ReservedHeapSection<'a> {
353353

354354
let zero_region_idx = heap_index!(self.heap_cell_len) + str_byte_len;
355355

356-
let align_offset = self.heap_ptr
357-
.add(zero_region_idx)
358-
.align_offset(ALIGN_CELL);
359-
360-
let align_offset = if align_offset == 0 {
361-
ALIGN_CELL
362-
} else {
363-
align_offset
364-
};
356+
let align_offset = pstr_sentinel_length(zero_region_idx);
365357

366358
ptr::write_bytes(
367359
self.heap_ptr.add(zero_region_idx),
@@ -481,6 +473,22 @@ impl<'a> Index<usize> for ReservedHeapSection<'a> {
481473
}
482474
}
483475

476+
/// Computes the number of bytes required to pad a string of length `chunk_len`
477+
/// with zeroes, such that `chunk_len + pstr_sentinel_length(chunk_len)` is a
478+
/// multiple of `Heap::heap_cell_alignement()`.
479+
fn pstr_sentinel_length(chunk_len: usize) -> usize {
480+
const ALIGN: usize = Heap::heap_cell_alignment();
481+
482+
let res = chunk_len.next_multiple_of(ALIGN) - chunk_len;
483+
484+
// No bytes available in last chunk
485+
if res == 0 {
486+
ALIGN
487+
} else {
488+
res
489+
}
490+
}
491+
484492
#[must_use]
485493
#[derive(Debug)]
486494
pub struct HeapWriter<'a> {
@@ -635,7 +643,7 @@ impl Heap {
635643
if self.free_space() >= len {
636644
section = ReservedHeapSection {
637645
heap_ptr: self.inner.ptr,
638-
heap_cell_len: cell_index!(self.inner.byte_len),
646+
heap_cell_len: self.cell_len(),
639647
pstr_vec: &mut self.pstr_vec,
640648
};
641649
break;
@@ -810,6 +818,11 @@ impl Heap {
810818
}
811819
}
812820

821+
// SAFETY:
822+
// - Postcondition: from `self.grow()`, `self.inner.byte_len + size_of::<HeapCellValue>()`
823+
// is strictly less than `self.inner.byte_cap`.
824+
// - Asserted: `self.cell_len() * size_of::<HeapCellvalue>() <= self.inner.byte_cap`.
825+
// - Invariant: from `InnerHeap`, `self.inner.byte_cap < isize::MAX`.
813826
let cell_ptr = (self.inner.ptr as *mut HeapCellValue).add(self.cell_len());
814827
cell_ptr.write(cell);
815828
self.pstr_vec.push(false);
@@ -967,17 +980,7 @@ impl Heap {
967980

968981
const ALIGN_CELL: usize = Heap::heap_cell_alignment();
969982

970-
let align_offset = unsafe {
971-
self.inner.ptr
972-
.add(self.inner.byte_len + s_len)
973-
.align_offset(ALIGN_CELL)
974-
};
975-
976-
let align_offset = if align_offset == 0 {
977-
ALIGN_CELL
978-
} else {
979-
align_offset
980-
};
983+
let align_offset = pstr_sentinel_length(s_len);
981984

982985
let copy_size = s_len + align_offset;
983986

@@ -1040,8 +1043,9 @@ impl Heap {
10401043
Ok(())
10411044
}
10421045

1043-
// assumes the string will be allocated on a ALIGN_CELL-byte boundary
1044-
pub(crate) const fn compute_pstr_size(src: &str) -> usize {
1046+
/// Returns the number of bytes needed to store `src` as a `PStr`.
1047+
/// Assumes the string will be allocated on a ALIGN_CELL-byte boundary.
1048+
pub(crate) fn compute_pstr_size(src: &str) -> usize {
10451049
const ALIGN_CELL: usize = Heap::heap_cell_alignment();
10461050

10471051
if src.is_empty() {
@@ -1062,7 +1066,7 @@ impl Heap {
10621066
null_idx += 1;
10631067
}
10641068

1065-
byte_size += (null_idx & !(ALIGN_CELL - 1)) + ALIGN_CELL;
1069+
byte_size += null_idx.next_multiple_of(ALIGN_CELL);
10661070

10671071
if (null_idx + 1) % ALIGN_CELL == 0 {
10681072
byte_size += 2 * mem::size_of::<HeapCellValue>();

0 commit comments

Comments
 (0)