potentially confused mmap code in lucet-runtime internals

[froydnj]

I was trying to figure out what might be involved in adding Windows support to lucet-runtime-internals and came across these two pieces of code:

lucet/lucet-runtime/lucet-runtime-internals/src/region/mmap.rs

Lines 167 to 179 in 0b51fe7

	unsafe {
	// MADV_DONTNEED is not guaranteed to clear pages on non-Linux systems
	#[cfg(not(target_os = "linux"))]
	{
	mprotect(*ptr, *len, ProtFlags::PROT_READ | ProtFlags::PROT_WRITE)
	.expect("mprotect succeeds during drop");
	memset(*ptr, 0, *len);
	}
	mprotect(*ptr, *len, ProtFlags::PROT_NONE).expect("mprotect succeeds during drop");
	madvise(*ptr, *len, MmapAdvise::MADV_DONTNEED)
	.expect("madvise succeeds during drop");
	}
	}

lucet/lucet-runtime/lucet-runtime-internals/src/region/mmap.rs

Lines 199 to 216 in 0b51fe7

	// zero the whole heap, if any of it is currently accessible
	let heap_size = alloc.slot().limits.heap_address_space_size;
	unsafe {
	// `mprotect()` and `madvise()` are sufficient to zero a page on Linux,
	// but not necessarily on all POSIX operating systems, and on macOS in particular.
	#[cfg(not(target_os = "linux"))]
	{
	mprotect(
	heap,
	alloc.heap_accessible_size,
	ProtFlags::PROT_READ | ProtFlags::PROT_WRITE,
	)?;
	memset(heap, 0, alloc.heap_accessible_size);
	}
	mprotect(heap, heap_size, ProtFlags::PROT_NONE)?;
	madvise(heap, heap_size, MmapAdvise::MADV_DONTNEED)?;
	}

The two pieces are subtle enough that it's worth factoring out a separate function to handle them, but the code and the comments in the second one appear to be at odds with one another: zeroing the whole heap would seem to suggest zeroing more than just the currently accessible (?) heap. And the code itself in the second one appears to be at odds with itself as well: why the difference in what we memset vs. what we mprotect(NONE)/madvise?

Am I just insufficiently knowledgeable about what's going on, or is there a real problem here?

[iximeow]

I agree that these could be factored out into a separate function, the high-level intent is to quickly zero pages we know will have to be reset, so just calling it by sysdep::reset_pages or similar would be an improvement. Then we can gate that by OS to use whichever APIs are appropriate.

I suspect the difference in zeroing size when clearing the heap is an accident, and should either be heap_size in both places, or alloc.heap_accessible_size in both places. heap_accessible_size should be sufficient since excess heap ought to still be zeroed from its last reset, or instance creation, if memory serves for those limits.. It shouldn't be an error, but it's definitely not a good place to disagree on sizes!