Code review feedback.

* Improve comments.
* Drop old table element *after* updating the table.
* Extract out the same `cfg_if!` to a single constant.
This commit is contained in:
Peter Huene
2021-03-08 09:04:13 -08:00
parent 8e51aefb2c
commit 7a93132ffa
4 changed files with 19 additions and 25 deletions

View File

@@ -45,7 +45,11 @@ fn decommit(addr: *mut u8, len: usize) -> Result<()> {
}
unsafe {
// On Linux, this is enough to cause the kernel to initialize the pages to 0 on next access
// On Linux, this tells the kernel to discard the backing of the pages in the range.
// If the discarded pages are part of a uffd region, then the next access will fault
// and the user fault handler will receive the event.
// If the pages are not monitored by uffd, the kernel will zero the page on next access,
// as if it were mmap'd for the first time.
if libc::madvise(addr as _, len, libc::MADV_DONTNEED) != 0 {
bail!(
"madvise failed to decommit: {}",
@@ -93,8 +97,6 @@ pub fn decommit_stack_pages(addr: *mut u8, len: usize) -> Result<()> {
/// With uffd, however, the potentially accessible pages of the each linear memory are made `READ_WRITE` and
/// the page fault handler will detect an out of bounds access and treat the page, temporarily,
/// as a guard page.
///
/// This me
pub(super) fn initialize_memory_pool(pool: &MemoryPool) -> Result<()> {
if pool.memory_size == 0 || pool.max_wasm_pages == 0 {
return Ok(());

View File

@@ -5,6 +5,10 @@ fn decommit(addr: *mut u8, len: usize, protect: bool) -> Result<()> {
return Ok(());
}
// By creating a new mapping at the same location, this will discard the
// mapping for the pages in the given range.
// The new mapping will be to the CoW zero page, so this effectively
// zeroes the pages.
if unsafe {
libc::mmap(
addr as _,

View File

@@ -372,11 +372,13 @@ impl Table {
}
}
fn set_raw(ty: TableElementType, e: &mut *mut u8, val: TableElement) {
fn set_raw(ty: TableElementType, elem: &mut *mut u8, val: TableElement) {
unsafe {
// Drop the existing element
let _ = TableElement::from_raw(ty, *e);
*e = val.into_raw();
let old = *elem;
*elem = val.into_raw();
// Drop the old element
let _ = TableElement::from_raw(ty, old);
}
}

View File

@@ -307,6 +307,8 @@ impl Module {
/// # }
/// ```
pub fn from_binary(engine: &Engine, binary: &[u8]) -> Result<Module> {
const USE_PAGED_MEM_INIT: bool = cfg!(all(feature = "uffd", target_os = "linux"));
cfg_if::cfg_if! {
if #[cfg(feature = "cache")] {
let (main_module, artifacts, types) = ModuleCacheEntry::new(
@@ -314,27 +316,11 @@ impl Module {
engine.cache_config(),
)
.get_data((engine.compiler(), binary), |(compiler, binary)| {
cfg_if::cfg_if! {
if #[cfg(all(feature = "uffd", target_os = "linux"))] {
let use_paged_mem_init = true;
} else {
let use_paged_mem_init = false;
}
};
CompilationArtifacts::build(compiler, binary, use_paged_mem_init)
CompilationArtifacts::build(compiler, binary, USE_PAGED_MEM_INIT)
})?;
} else {
cfg_if::cfg_if! {
if #[cfg(all(feature = "uffd", target_os = "linux"))] {
let use_paged_mem_init = true;
} else {
let use_paged_mem_init = false;
}
};
let (main_module, artifacts, types) =
CompilationArtifacts::build(engine.compiler(), binary, use_paged_mem_init)?;
CompilationArtifacts::build(engine.compiler(), binary, USE_PAGED_MEM_INIT)?;
}
};