Make munmap throw unsup errors instead of trying to work

saethlin · saethlin · commit d84f74916bc9 · 2023-06-16T19:10:54.000-04:00
diff --git 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<Provenance>> {
         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<Provenance>> {
         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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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();
 }

Original file line number	Diff line number	Diff line change
`@@ -13,6 +13,6 @@ fn main() {`
`13`	`13`	`0,`
`14`	`14`	`);`
`15`	`15`	`libc::munmap(ptr, 1);`
`16`		`- //~^ ERROR: unsupported operation: Miri does not support partial munmap`
	`16`	`+ //~^ ERROR: unsupported operation`
`17`	`17`	`}`
`18`	`18`	`}`
Original file line number	Diff line number	Diff line change
`@@ -13,11 +13,11 @@ LL \| libc::munmap(ptr, 1);`
`13`	`13`	`= note: BACKTRACE:`
`14`	`14`	= note: inside `main` at $DIR/munmap_partial.rs:LL:CC
`15`	`15`
`16`		`-error: unsupported operation: Miri does not support partial munmap`
	`16`	`+error: unsupported operation: Miri only supports munmap calls that exactly unmap a region previously returned by mmap`
`17`	`17`	`--> $DIR/munmap_partial.rs:LL:CC`
`18`	`18`	`\|`
`19`	`19`	`LL \| libc::munmap(ptr, 1);`
`20`		`- \| ^^^^^^^^^^^^^^^^^^^^ Miri does not support partial munmap`
	`20`	`+ \| ^^^^^^^^^^^^^^^^^^^^ Miri only supports munmap calls that exactly unmap a region previously returned by mmap`
`21`	`21`	`\|`
`22`	`22`	`= help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support`
`23`	`23`	`= note: BACKTRACE:`