From d84f74916bc978a02cf85da97d91d90f56984c0a Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Fri, 16 Jun 2023 19:10:54 -0400 Subject: [PATCH] Make munmap throw unsup errors instead of trying to work --- src/shims/unix/mem.rs | 71 +++++++++++++------------------- tests/fail/munmap.rs | 22 ++++++++++ tests/fail/munmap.stderr | 16 ++++--- tests/fail/munmap_partial.rs | 2 +- tests/fail/munmap_partial.stderr | 4 +- tests/pass-dep/shims/mmap.rs | 13 ------ 6 files changed, 64 insertions(+), 64 deletions(-) create mode 100644 tests/fail/munmap.rs diff --git a/src/shims/unix/mem.rs b/src/shims/unix/mem.rs index 1d01bc82a8..f133dca609 100644 --- a/src/shims/unix/mem.rs +++ b/src/shims/unix/mem.rs @@ -8,7 +8,7 @@ //! else that goes beyond a basic allocation API. use crate::*; -use rustc_target::abi::{Align, Size}; +use rustc_target::abi::Size; impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { @@ -88,7 +88,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { throw_unsup_format!("Miri does not support non-zero offsets to mmap"); } - let align = Align::from_bytes(this.machine.page_size).unwrap(); + let align = this.machine.page_align(); let map_length = this.machine.round_up_to_multiple_of_page_size(length).unwrap_or(u64::MAX); let ptr = @@ -115,14 +115,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { ) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - let old_address = this.read_pointer(old_address)?; + let old_address = this.read_scalar(old_address)?.to_target_usize(this)?; let old_size = this.read_scalar(old_size)?.to_target_usize(this)?; let new_size = this.read_scalar(new_size)?.to_target_usize(this)?; let flags = this.read_scalar(flags)?.to_i32()?; // old_address must be a multiple of the page size #[allow(clippy::arithmetic_side_effects)] // PAGE_SIZE is nonzero - if old_address.addr().bytes() % this.machine.page_size != 0 || new_size == 0 { + if old_address % this.machine.page_size != 0 || new_size == 0 { this.set_last_error(Scalar::from_i32(this.eval_libc_i32("EINVAL")))?; return Ok(this.eval_libc("MAP_FAILED")); } @@ -141,6 +141,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { return Ok(Scalar::from_maybe_pointer(Pointer::null(), this)); } + let old_address = Machine::ptr_from_addr_cast(this, old_address)?; let align = this.machine.page_align(); let ptr = this.reallocate_ptr( old_address, @@ -171,57 +172,41 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { ) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - let addr = this.read_pointer(addr)?; + let addr = this.read_scalar(addr)?.to_target_usize(this)?; let length = this.read_scalar(length)?.to_target_usize(this)?; // addr must be a multiple of the page size #[allow(clippy::arithmetic_side_effects)] // PAGE_SIZE is nonzero - if addr.addr().bytes() % this.machine.page_size != 0 { + if addr % this.machine.page_size != 0 { this.set_last_error(Scalar::from_i32(this.eval_libc_i32("EINVAL")))?; return Ok(Scalar::from_i32(-1)); } let length = this.machine.round_up_to_multiple_of_page_size(length).unwrap_or(u64::MAX); - let mut addr = addr.addr().bytes(); - let mut bytes_unmapped = 0; - while bytes_unmapped < length { - // munmap specifies: - // It is not an error if the indicated range does not contain any mapped pages. - // So we make sure that if our address is not that of an exposed allocation, we just - // step forward to the next page. - let ptr = Machine::ptr_from_addr_cast(this, addr)?; - let Ok(ptr) = ptr.into_pointer_or_addr() else { - bytes_unmapped = bytes_unmapped.checked_add(this.machine.page_size).unwrap(); - addr = addr.wrapping_add(this.machine.page_size); - continue; - }; - // FIXME: This should fail if the pointer is to an unexposed allocation. But it - // doesn't. - let Some((alloc_id, offset, _prov)) = Machine::ptr_get_alloc(this, ptr) else { - bytes_unmapped = bytes_unmapped.checked_add(this.machine.page_size).unwrap(); - addr = addr.wrapping_add(this.machine.page_size); - continue; - }; - - if offset != Size::ZERO { - throw_unsup_format!("Miri does not support partial munmap"); - } - let (_kind, alloc) = this.memory.alloc_map().get(alloc_id).unwrap(); - let this_alloc_len = alloc.len() as u64; - bytes_unmapped = bytes_unmapped.checked_add(this_alloc_len).unwrap(); - if bytes_unmapped > length { - throw_unsup_format!("Miri does not support partial munmap"); - } - - this.deallocate_ptr( - Pointer::new(Some(Provenance::Wildcard), Size::from_bytes(addr)), - Some((Size::from_bytes(this_alloc_len), this.machine.page_align())), - MemoryKind::Machine(MiriMemoryKind::Mmap), - )?; - addr = addr.wrapping_add(this_alloc_len); + let ptr = Machine::ptr_from_addr_cast(this, addr)?; + + let Ok(ptr) = ptr.into_pointer_or_addr() else { + throw_unsup_format!("Miri only supports munmap on memory allocated directly by mmap"); + }; + let Some((alloc_id, offset, _prov)) = Machine::ptr_get_alloc(this, ptr) else { + throw_unsup_format!("Miri only supports munmap on memory allocated directly by mmap"); + }; + + let (_kind, alloc) = this.memory.alloc_map().get(alloc_id).unwrap(); + if offset != Size::ZERO || alloc.len() as u64 != length { + throw_unsup_format!( + "Miri only supports munmap calls that exactly unmap a region previously returned by mmap" + ); } + let len = Size::from_bytes(alloc.len() as u64); + this.deallocate_ptr( + Pointer::new(Some(Provenance::Wildcard), Size::from_bytes(addr)), + Some((len, this.machine.page_align())), + MemoryKind::Machine(MiriMemoryKind::Mmap), + )?; + Ok(Scalar::from_i32(0)) } } diff --git a/tests/fail/munmap.rs b/tests/fail/munmap.rs new file mode 100644 index 0000000000..679de2c92c --- /dev/null +++ b/tests/fail/munmap.rs @@ -0,0 +1,22 @@ +//@compile-flags: -Zmiri-disable-isolation +//@ignore-target-windows: No libc on Windows + +#![feature(rustc_private)] +#![feature(strict_provenance)] + +use std::ptr; + +fn main() { + // Linux specifies that it is not an error if the specified range does not contain any pages. + // But we simply do not support such calls. This test checks that we report this as + // unsupported, not Undefined Behavior. + let res = unsafe { + libc::munmap( + //~ ERROR: unsupported operation + // Some high address we surely have not allocated anything at + ptr::invalid_mut(1 << 30), + 4096, + ) + }; + assert_eq!(res, 0); +} diff --git a/tests/fail/munmap.stderr b/tests/fail/munmap.stderr index a8bcbd098f..e1d9180fa1 100644 --- a/tests/fail/munmap.stderr +++ b/tests/fail/munmap.stderr @@ -1,8 +1,11 @@ warning: integer-to-pointer cast --> $DIR/munmap.rs:LL:CC | -LL | libc::munmap(ptr, 1); - | ^^^^^^^^^^^^^^^^^^^^ integer-to-pointer cast +LL | / libc::munmap( +LL | | ptr::invalid_mut(1 << 30), // Some high address we surely have no allocated anything at +LL | | 4096, +LL | | ) + | |_________^ integer-to-pointer cast | = help: This program is using integer-to-pointer casts or (equivalently) `ptr::from_exposed_addr`, = help: which means that Miri might miss pointer bugs in this program. @@ -13,11 +16,14 @@ LL | libc::munmap(ptr, 1); = note: BACKTRACE: = note: inside `main` at $DIR/munmap.rs:LL:CC -error: unsupported operation: Miri does not support partial munmap +error: unsupported operation: Miri only supports munmap on memory allocated directly by mmap --> $DIR/munmap.rs:LL:CC | -LL | libc::munmap(ptr, 1); - | ^^^^^^^^^^^^^^^^^^^^ Miri does not support partial munmap +LL | / libc::munmap( +LL | | ptr::invalid_mut(1 << 30), // Some high address we surely have no allocated anything at +LL | | 4096, +LL | | ) + | |_________^ Miri only supports munmap on memory allocated directly by mmap | = help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support = note: BACKTRACE: diff --git a/tests/fail/munmap_partial.rs b/tests/fail/munmap_partial.rs index 41387ae0b9..938850ee28 100644 --- a/tests/fail/munmap_partial.rs +++ b/tests/fail/munmap_partial.rs @@ -13,6 +13,6 @@ fn main() { 0, ); libc::munmap(ptr, 1); - //~^ ERROR: unsupported operation: Miri does not support partial munmap + //~^ ERROR: unsupported operation } } diff --git a/tests/fail/munmap_partial.stderr b/tests/fail/munmap_partial.stderr index a8bd999e66..9a084c5043 100644 --- a/tests/fail/munmap_partial.stderr +++ b/tests/fail/munmap_partial.stderr @@ -13,11 +13,11 @@ LL | libc::munmap(ptr, 1); = note: BACKTRACE: = note: inside `main` at $DIR/munmap_partial.rs:LL:CC -error: unsupported operation: Miri does not support partial munmap +error: unsupported operation: Miri only supports munmap calls that exactly unmap a region previously returned by mmap --> $DIR/munmap_partial.rs:LL:CC | LL | libc::munmap(ptr, 1); - | ^^^^^^^^^^^^^^^^^^^^ Miri does not support partial munmap + | ^^^^^^^^^^^^^^^^^^^^ Miri only supports munmap calls that exactly unmap a region previously returned by mmap | = help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support = note: BACKTRACE: diff --git a/tests/pass-dep/shims/mmap.rs b/tests/pass-dep/shims/mmap.rs index 4e5060e85a..03ffc3b389 100644 --- a/tests/pass-dep/shims/mmap.rs +++ b/tests/pass-dep/shims/mmap.rs @@ -63,21 +63,8 @@ fn test_mremap() { assert_eq!(res, 0i32); } -fn test_munmap() { - // Linux specifies that it is not an error if the specified range does not contain any pages. - let res = unsafe { - libc::munmap( - // Some high address we surely have no allocated anything at - ptr::invalid_mut(1 << 30), - 4096, - ) - }; - assert_eq!(res, 0); -} - fn main() { test_mmap(); - test_munmap(); #[cfg(target_os = "linux")] test_mremap(); }