Merge pull request from GHSA-wh6w-3828-g9qf

* Unconditionally use `MemoryImageSlot`

This commit removes the internal branching within the pooling instance
allocator to sometimes use a `MemoryImageSlot` and sometimes now.
Instead this is now unconditionally used in all situations on all
platforms. This fixes an issue where the state of a slot could get
corrupted if modules being instantiated switched from having images to
not having an image or vice versa.

The bulk of this commit is the removal of the `memory-init-cow`
compile-time feature in addition to adding Windows support to the
`cow.rs` file.

* Fix compile on Unix

* Add a stricter assertion for static memory bounds

Double-check that when a memory is allocated the configuration required
is satisfied by the pooling allocator.

Cargo.toml

@@ -181,13 +181,11 @@ default = [
   "vtune",
   "wasi-nn",
   "pooling-allocator",
-  "memory-init-cow",
 ]
 jitdump = ["wasmtime/jitdump"]
 vtune = ["wasmtime/vtune"]
 wasi-crypto = ["dep:wasmtime-wasi-crypto"]
 wasi-nn = ["dep:wasmtime-wasi-nn"]
-memory-init-cow = ["wasmtime/memory-init-cow", "wasmtime-cli-flags/memory-init-cow"]
 pooling-allocator = ["wasmtime/pooling-allocator", "wasmtime-cli-flags/pooling-allocator"]
 all-arch = ["wasmtime/all-arch"]
 posix-signals-on-macos = ["wasmtime/posix-signals-on-macos"]

crates/c-api/Cargo.toml

@@ -33,9 +33,8 @@ cap-std = { workspace = true, optional = true }
 wasi-common = { workspace = true, optional = true }
 
 [features]
-default = ['jitdump', 'wat', 'wasi', 'cache', 'parallel-compilation', 'memory-init-cow']
+default = ['jitdump', 'wat', 'wasi', 'cache', 'parallel-compilation']
 jitdump = ["wasmtime/jitdump"]
 cache = ["wasmtime/cache"]
 parallel-compilation = ['wasmtime/parallel-compilation']
 wasi = ['wasi-cap-std-sync', 'wasmtime-wasi', 'cap-std', 'wasi-common']
-memory-init-cow = ["wasmtime/memory-init-cow"]

crates/cli-flags/Cargo.toml

@@ -25,5 +25,4 @@ default = [
     "wasmtime/parallel-compilation",
 ]
 pooling-allocator = []
-memory-init-cow = []
 component-model = []

crates/cli-flags/src/lib.rs

@@ -218,7 +218,6 @@ pub struct CommonOptions {
 
     /// Disable the default of attempting to initialize linear memory via a
     /// copy-on-write mapping
-    #[cfg(feature = "memory-init-cow")]
     #[clap(long)]
     pub disable_memory_init_cow: bool,
 
@@ -324,7 +323,6 @@ impl CommonOptions {
 
         config.epoch_interruption(self.epoch_interruption);
         config.generate_address_map(!self.disable_address_map);
-        #[cfg(feature = "memory-init-cow")]
         config.memory_init_cow(!self.disable_memory_init_cow);
 
         #[cfg(feature = "pooling-allocator")]

crates/runtime/Cargo.toml

@@ -23,7 +23,7 @@ thiserror = "1.0.4"
 cfg-if = "1.0"
 rand = "0.8.3"
 anyhow = { workspace = true }
-memfd = { version = "0.6.1", optional = true }
+memfd = "0.6.1"
 paste = "1.0.3"
 encoding_rs = { version = "0.8.31", optional = true }
 
@@ -52,8 +52,6 @@ cc = "1.0"
 maintenance = { status = "actively-developed" }
 
 [features]
-memory-init-cow = ['memfd']
-
 async = ["wasmtime-fiber"]
 
 # Enables support for the pooling instance allocator

crates/runtime/build.rs

@@ -14,13 +14,4 @@ fn main() {
     println!("cargo:rerun-if-changed=src/helpers.c");
     build.file("src/helpers.c");
     build.compile("wasmtime-helpers");
-
-    // Check to see if we are on Unix and the `memory-init-cow` feature is
-    // active. If so, enable the `memory_init_cow` rustc cfg so
-    // `#[cfg(memory_init_cow)]` will work.
-    let family = env::var("CARGO_CFG_TARGET_FAMILY").unwrap();
-    let memory_init_cow = env::var("CARGO_FEATURE_MEMORY_INIT_COW").is_ok();
-    if &family == "unix" && memory_init_cow {
-        println!("cargo:rustc-cfg=memory_init_cow");
-    }
 }

crates/runtime/src/cow.rs

@@ -1,11 +1,12 @@
 //! Copy-on-write initialization support: creation of backing images for
 //! modules, and logic to support mapping these backing images into memory.
 
+#![cfg_attr(not(unix), allow(unused_imports, unused_variables))]
+
 use crate::InstantiationError;
 use crate::MmapVec;
 use anyhow::Result;
 use libc::c_void;
-use rustix::fd::AsRawFd;
 use std::fs::File;
 use std::sync::Arc;
 use std::{convert::TryFrom, ops::Range};
@@ -60,24 +61,34 @@ pub struct MemoryImage {
 
 #[derive(Debug)]
 enum FdSource {
+    #[cfg(unix)]
     Mmap(Arc<File>),
     #[cfg(target_os = "linux")]
     Memfd(memfd::Memfd),
 }
 
 impl FdSource {
+    #[cfg(unix)]
     fn as_file(&self) -> &File {
         match self {
-            FdSource::Mmap(file) => file,
+            FdSource::Mmap(ref file) => file,
             #[cfg(target_os = "linux")]
-            FdSource::Memfd(memfd) => memfd.as_file(),
+            FdSource::Memfd(ref memfd) => memfd.as_file(),
         }
     }
 }
 
 impl PartialEq for FdSource {
     fn eq(&self, other: &FdSource) -> bool {
-        self.as_file().as_raw_fd() == other.as_file().as_raw_fd()
+        cfg_if::cfg_if! {
+            if #[cfg(unix)] {
+                use rustix::fd::AsRawFd;
+                self.as_file().as_raw_fd() == other.as_file().as_raw_fd()
+            } else {
+                drop(other);
+                match *self {}
+            }
+        }
     }
 }
 
@@ -111,6 +122,7 @@ impl MemoryImage {
         // files, but for now this is still a Linux-specific region of Wasmtime.
         // Some work will be needed to get this file compiling for macOS and
         // Windows.
+        #[cfg(not(windows))]
         if let Some(mmap) = mmap {
             let start = mmap.as_ptr() as usize;
             let end = start + mmap.len();
@@ -185,6 +197,42 @@ impl MemoryImage {
             }
         }
     }
+
+    unsafe fn map_at(&self, base: usize) -> Result<()> {
+        cfg_if::cfg_if! {
+            if #[cfg(unix)] {
+                let ptr = rustix::mm::mmap(
+                    (base + self.linear_memory_offset) as *mut c_void,
+                    self.len,
+                    rustix::mm::ProtFlags::READ | rustix::mm::ProtFlags::WRITE,
+                    rustix::mm::MapFlags::PRIVATE | rustix::mm::MapFlags::FIXED,
+                    self.fd.as_file(),
+                    self.fd_offset,
+                )?;
+                assert_eq!(ptr as usize, base + self.linear_memory_offset);
+                Ok(())
+            } else {
+                match self.fd {}
+            }
+        }
+    }
+
+    unsafe fn remap_as_zeros_at(&self, base: usize) -> Result<()> {
+        cfg_if::cfg_if! {
+            if #[cfg(unix)] {
+                let ptr = rustix::mm::mmap_anonymous(
+                    (base + self.linear_memory_offset) as *mut c_void,
+                    self.len,
+                    rustix::mm::ProtFlags::READ | rustix::mm::ProtFlags::WRITE,
+                    rustix::mm::MapFlags::PRIVATE | rustix::mm::MapFlags::FIXED,
+                )?;
+                assert_eq!(ptr as usize, base + self.linear_memory_offset);
+                Ok(())
+            } else {
+                match self.fd {}
+            }
+        }
+    }
 }
 
 #[cfg(target_os = "linux")]
@@ -374,6 +422,17 @@ impl MemoryImageSlot {
         }
     }
 
+    pub(crate) fn dummy() -> MemoryImageSlot {
+        MemoryImageSlot {
+            base: 0,
+            static_size: 0,
+            image: None,
+            accessible: 0,
+            dirty: false,
+            clear_on_drop: false,
+        }
+    }
+
     /// Inform the MemoryImageSlot that it should *not* clear the underlying
     /// address space when dropped. This should be used only when the
     /// caller will clear or reuse the address space in some other
@@ -396,10 +455,7 @@ impl MemoryImageSlot {
         }
 
         // Otherwise use `mprotect` to make the new pages read/write.
-        self.set_protection(
-            self.accessible..size_bytes,
-            rustix::mm::MprotectFlags::READ | rustix::mm::MprotectFlags::WRITE,
-        )?;
+        self.set_protection(self.accessible..size_bytes, true)?;
         self.accessible = size_bytes;
 
         Ok(())
@@ -444,14 +500,9 @@ impl MemoryImageSlot {
         if self.image.as_ref() != maybe_image {
             if let Some(image) = &self.image {
                 unsafe {
-                    let ptr = rustix::mm::mmap_anonymous(
-                        (self.base + image.linear_memory_offset) as *mut c_void,
-                        image.len,
-                        rustix::mm::ProtFlags::READ | rustix::mm::ProtFlags::WRITE,
-                        rustix::mm::MapFlags::PRIVATE | rustix::mm::MapFlags::FIXED,
-                    )
-                    .map_err(|e| InstantiationError::Resource(e.into()))?;
-                    assert_eq!(ptr as usize, self.base + image.linear_memory_offset);
+                    image
+                        .remap_as_zeros_at(self.base)
+                        .map_err(|e| InstantiationError::Resource(e.into()))?;
                 }
                 self.image = None;
             }
@@ -461,11 +512,8 @@ impl MemoryImageSlot {
         // appropriate. First up is to grow the read/write portion of memory if
         // it's not large enough to accommodate `initial_size_bytes`.
         if self.accessible < initial_size_bytes {
-            self.set_protection(
-                self.accessible..initial_size_bytes,
-                rustix::mm::MprotectFlags::READ | rustix::mm::MprotectFlags::WRITE,
-            )
-            .map_err(|e| InstantiationError::Resource(e.into()))?;
+            self.set_protection(self.accessible..initial_size_bytes, true)
+                .map_err(|e| InstantiationError::Resource(e.into()))?;
             self.accessible = initial_size_bytes;
         }
 
@@ -480,11 +528,8 @@ impl MemoryImageSlot {
         if initial_size_bytes < self.accessible {
             match style {
                 MemoryStyle::Static { .. } => {
-                    self.set_protection(
-                        initial_size_bytes..self.accessible,
-                        rustix::mm::MprotectFlags::empty(),
-                    )
-                    .map_err(|e| InstantiationError::Resource(e.into()))?;
+                    self.set_protection(initial_size_bytes..self.accessible, false)
+                        .map_err(|e| InstantiationError::Resource(e.into()))?;
                     self.accessible = initial_size_bytes;
                 }
                 MemoryStyle::Dynamic { .. } => {}
@@ -503,16 +548,9 @@ impl MemoryImageSlot {
                 );
                 if image.len > 0 {
                     unsafe {
-                        let ptr = rustix::mm::mmap(
-                            (self.base + image.linear_memory_offset) as *mut c_void,
-                            image.len,
-                            rustix::mm::ProtFlags::READ | rustix::mm::ProtFlags::WRITE,
-                            rustix::mm::MapFlags::PRIVATE | rustix::mm::MapFlags::FIXED,
-                            image.fd.as_file(),
-                            image.fd_offset,
-                        )
-                        .map_err(|e| InstantiationError::Resource(e.into()))?;
-                        assert_eq!(ptr as usize, self.base + image.linear_memory_offset);
+                        image
+                            .map_at(self.base)
+                            .map_err(|e| InstantiationError::Resource(e.into()))?;
                     }
                 }
             }
@@ -675,13 +713,35 @@ impl MemoryImageSlot {
         }
     }
 
-    fn set_protection(&self, range: Range<usize>, flags: rustix::mm::MprotectFlags) -> Result<()> {
+    fn set_protection(&self, range: Range<usize>, readwrite: bool) -> Result<()> {
         assert!(range.start <= range.end);
         assert!(range.end <= self.static_size);
-        let mprotect_start = self.base.checked_add(range.start).unwrap();
-        if range.len() > 0 {
-            unsafe {
-                rustix::mm::mprotect(mprotect_start as *mut _, range.len(), flags)?;
+        let start = self.base.checked_add(range.start).unwrap();
+        if range.len() == 0 {
+            return Ok(());
+        }
+
+        unsafe {
+            cfg_if::cfg_if! {
+                if #[cfg(unix)] {
+                    let flags = if readwrite {
+                        rustix::mm::MprotectFlags::READ | rustix::mm::MprotectFlags::WRITE
+                    } else {
+                        rustix::mm::MprotectFlags::empty()
+                    };
+                    rustix::mm::mprotect(start as *mut _, range.len(), flags)?;
+                } else {
+                    use windows_sys::Win32::System::Memory::*;
+
+                    let failure = if readwrite {
+                        VirtualAlloc(start as _, range.len(), MEM_COMMIT, PAGE_READWRITE).is_null()
+                    } else {
+                        VirtualFree(start as _, range.len(), MEM_DECOMMIT) == 0
+                    };
+                    if failure {
+                        return Err(std::io::Error::last_os_error().into());
+                    }
+                }
             }
         }
 
@@ -701,13 +761,22 @@ impl MemoryImageSlot {
     /// inaccessible. Used both during instantiate and during drop.
     fn reset_with_anon_memory(&mut self) -> Result<()> {
         unsafe {
-            let ptr = rustix::mm::mmap_anonymous(
-                self.base as *mut c_void,
-                self.static_size,
-                rustix::mm::ProtFlags::empty(),
-                rustix::mm::MapFlags::PRIVATE | rustix::mm::MapFlags::FIXED,
-            )?;
-            assert_eq!(ptr as usize, self.base);
+            cfg_if::cfg_if! {
+                if #[cfg(unix)] {
+                    let ptr = rustix::mm::mmap_anonymous(
+                        self.base as *mut c_void,
+                        self.static_size,
+                        rustix::mm::ProtFlags::empty(),
+                        rustix::mm::MapFlags::PRIVATE | rustix::mm::MapFlags::FIXED,
+                    )?;
+                    assert_eq!(ptr as usize, self.base);
+                } else {
+                    use windows_sys::Win32::System::Memory::*;
+                    if VirtualFree(self.base as _, self.static_size, MEM_DECOMMIT) == 0 {
+                        return Err(std::io::Error::last_os_error().into());
+                    }
+                }
+            }
         }
 
         self.image = None;