Merge pull request #401 from bytecodealliance/acf/alloc-layout-fix

Fix incorrect globals/sigstack allocation layout

benchmarks/lucet-benchmarks/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "lucet-benchmarks"
-version = "0.4.1"
+version = "0.5.1"
 description = "Benchmarks for the Lucet runtime"
 homepage = "https://github.com/fastly/lucet"
 repository = "https://github.com/fastly/lucet"

lucet-module/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "lucet-module"
-version = "0.5.0"
+version = "0.5.1"
 description = "A structured interface for Lucet modules"
 homepage = "https://github.com/fastly/lucet"
 repository = "https://github.com/fastly/lucet"

lucet-objdump/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "lucet-objdump"
-version = "0.5.0"
+version = "0.5.1"
 description = "Analyze object files emitted by the Lucet compiler"
 homepage = "https://github.com/fastly/lucet"
 repository = "https://github.com/fastly/lucet"
@@ -13,7 +13,7 @@ edition = "2018"
 goblin="0.0.24"
 byteorder="1.2.1"
 colored="1.8.0"
-lucet-module = { path = "../lucet-module", version = "=0.5.0" }
+lucet-module = { path = "../lucet-module", version = "=0.5.1" }
 
 [package.metadata.deb]
 name = "fst-lucet-objdump"

lucet-runtime/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "lucet-runtime"
-version = "0.5.0"
+version = "0.5.1"
 description = "Pure Rust runtime for Lucet WebAssembly toolchain"
 homepage = "https://github.com/fastly/lucet"
 repository = "https://github.com/fastly/lucet"
@@ -11,17 +11,17 @@ edition = "2018"
 
 [dependencies]
 libc = "0.2.65"
-lucet-runtime-internals = { path = "lucet-runtime-internals", version = "=0.5.0" }
-lucet-module = { path = "../lucet-module", version = "=0.5.0" }
+lucet-runtime-internals = { path = "lucet-runtime-internals", version = "=0.5.1" }
+lucet-module = { path = "../lucet-module", version = "=0.5.1" }
 num-traits = "0.2"
 num-derive = "0.3.0"
 
 [dev-dependencies]
 byteorder = "1.2"
 lazy_static = "1.1"
-lucetc = { path = "../lucetc", version = "=0.5.0" }
-lucet-runtime-tests = { path = "lucet-runtime-tests", version = "=0.5.0" }
-lucet-wasi-sdk = { path = "../lucet-wasi-sdk", version = "=0.5.0" }
+lucetc = { path = "../lucetc", version = "=0.5.1" }
+lucet-runtime-tests = { path = "lucet-runtime-tests", version = "=0.5.1" }
+lucet-wasi-sdk = { path = "../lucet-wasi-sdk", version = "=0.5.1" }
 nix = "0.15"
 rayon = "1.0"
 tempfile = "3.0"

lucet-runtime/include/lucet_types.h

@@ -123,6 +123,13 @@ struct lucet_alloc_limits {
      * Size of the globals region in bytes; each global uses 8 bytes. (default 4K)
      */
     uint64_t globals_size;
+    /**
+     * Size of the signal stack region in bytes.
+     *
+     * SIGSTKSZ from <signals.h> is a good default when linking against a Rust release build of
+     * lucet-runtime, but 12K or more is recommended when using a Rust debug build.
+     */
+    uint64_t signal_stack_size;
 };
 
 typedef enum lucet_signal_behavior (*lucet_signal_handler)(struct lucet_instance *   inst,

lucet-runtime/lucet-runtime-internals/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "lucet-runtime-internals"
-version = "0.5.0"
+version = "0.5.1"
 description = "Pure Rust runtime for Lucet WebAssembly toolchain (internals)"
 homepage = "https://github.com/fastly/lucet"
 repository = "https://github.com/fastly/lucet"
@@ -10,8 +10,8 @@ authors = ["Lucet team <lucet@fastly.com>"]
 edition = "2018"
 
 [dependencies]
-lucet-module = { path = "../../lucet-module", version = "=0.5.0" }
-lucet-runtime-macros = { path = "../lucet-runtime-macros", version = "=0.5.0" }
+lucet-module = { path = "../../lucet-module", version = "=0.5.1" }
+lucet-runtime-macros = { path = "../lucet-runtime-macros", version = "=0.5.1" }
 
 anyhow = "1.0"
 bitflags = "1.0"

lucet-runtime/lucet-runtime-internals/src/alloc/mod.rs

@@ -1,7 +1,7 @@
 use crate::error::Error;
 use crate::module::Module;
 use crate::region::RegionInternal;
-use libc::{c_void, SIGSTKSZ};
+use libc::c_void;
 use lucet_module::GlobalValue;
 use nix::unistd::{sysconf, SysconfVar};
 use std::sync::{Arc, Once, Weak};
@@ -112,26 +112,79 @@ impl Drop for Alloc {
     }
 }
 
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+pub enum AddrLocation {
+    Heap,
+    InaccessibleHeap,
+    StackGuard,
+    Stack,
+    Globals,
+    SigStackGuard,
+    SigStack,
+    Unknown,
+}
+
+impl AddrLocation {
+    /// If a fault occurs in this location, is it fatal to the entire process?
+    ///
+    /// This is currently a permissive baseline that only returns true for unknown locations and the
+    /// signal stack guard, in case a `Region` implementation uses faults to populate the accessible
+    /// locations like the heap and the globals.
+    pub fn is_fault_fatal(self) -> bool {
+        use AddrLocation::*;
+        match self {
+            SigStackGuard | Unknown => true,
+            _ => false,
+        }
+    }
+}
+
 impl Alloc {
-    pub fn addr_in_guard_page(&self, addr: *const c_void) -> bool {
+    /// Where in an `Alloc` does a particular address fall?
+    pub fn addr_location(&self, addr: *const c_void) -> AddrLocation {
         let addr = addr as usize;
-        let heap = self.slot().heap as usize;
-        let guard_start = heap + self.heap_accessible_size;
-        let guard_end = heap + self.slot().limits.heap_address_space_size;
-        // eprintln!(
-        //     "addr = {:p}, guard_start = {:p}, guard_end = {:p}",
-        //     addr, guard_start as *mut c_void, guard_end as *mut c_void
-        // );
-        let stack_guard_end = self.slot().stack as usize;
-        let stack_guard_start = stack_guard_end - host_page_size();
-        // eprintln!(
-        //     "addr = {:p}, stack_guard_start = {:p}, stack_guard_end = {:p}",
-        //     addr, stack_guard_start as *mut c_void, stack_guard_end as *mut c_void
-        // );
-        let in_heap_guard = (addr >= guard_start) && (addr < guard_end);
-        let in_stack_guard = (addr >= stack_guard_start) && (addr < stack_guard_end);
-
-        in_heap_guard || in_stack_guard
+
+        let heap_start = self.slot().heap as usize;
+        let heap_inaccessible_start = heap_start + self.heap_accessible_size;
+        let heap_inaccessible_end = heap_start + self.slot().limits.heap_address_space_size;
+
+        if (addr >= heap_start) && (addr < heap_inaccessible_start) {
+            return AddrLocation::Heap;
+        }
+        if (addr >= heap_inaccessible_start) && (addr < heap_inaccessible_end) {
+            return AddrLocation::InaccessibleHeap;
+        }
+
+        let stack_start = self.slot().stack as usize;
+        let stack_end = stack_start + self.slot().limits.stack_size;
+        let stack_guard_start = stack_start - host_page_size();
+
+        if (addr >= stack_guard_start) && (addr < stack_start) {
+            return AddrLocation::StackGuard;
+        }
+        if (addr >= stack_start) && (addr < stack_end) {
+            return AddrLocation::Stack;
+        }
+
+        let globals_start = self.slot().globals as usize;
+        let globals_end = globals_start + self.slot().limits.globals_size;
+
+        if (addr >= globals_start) && (addr < globals_end) {
+            return AddrLocation::Globals;
+        }
+
+        let sigstack_start = self.slot().sigstack as usize;
+        let sigstack_end = sigstack_start + self.slot().limits.signal_stack_size;
+        let sigstack_guard_start = sigstack_start - host_page_size();
+
+        if (addr >= sigstack_guard_start) && (addr < sigstack_start) {
+            return AddrLocation::SigStackGuard;
+        }
+        if (addr >= sigstack_start) && (addr < sigstack_end) {
+            return AddrLocation::SigStack;
+        }
+
+        AddrLocation::Unknown
     }
 
     pub fn expand_heap(&mut self, expand_bytes: u32, module: &dyn Module) -> Result<u32, Error> {
@@ -318,7 +371,10 @@ impl Alloc {
 
     /// Return the sigstack as a mutable byte slice.
     pub unsafe fn sigstack_mut(&mut self) -> &mut [u8] {
-        std::slice::from_raw_parts_mut(self.slot().sigstack as *mut u8, libc::SIGSTKSZ)
+        std::slice::from_raw_parts_mut(
+            self.slot().sigstack as *mut u8,
+            self.slot().limits.signal_stack_size,
+        )
     }
 
     pub fn mem_in_heap<T>(&self, ptr: *const T, len: usize) -> bool {
@@ -351,15 +407,30 @@ pub struct Limits {
     pub stack_size: usize,
     /// Size of the globals region in bytes; each global uses 8 bytes. (default 4K)
     pub globals_size: usize,
+    /// Size of the signal stack in bytes. (default SIGSTKSZ for release builds, 12K for debug builds)
+    ///
+    /// This difference is to account for the greatly increased stack size usage in the signal
+    /// handler when running without optimizations.
+    ///
+    /// Note that debug vs. release mode is determined by `cfg(debug_assertions)`, so if you are
+    /// specifically enabling debug assertions in your release builds, the default signal stack will
+    /// be larger.
+    pub signal_stack_size: usize,
 }
 
-impl Default for Limits {
-    fn default() -> Limits {
+#[cfg(debug_assertions)]
+pub const DEFAULT_SIGNAL_STACK_SIZE: usize = 12 * 1024;
+#[cfg(not(debug_assertions))]
+pub const DEFAULT_SIGNAL_STACK_SIZE: usize = libc::SIGSTKSZ;
+
+impl Limits {
+    pub const fn default() -> Limits {
         Limits {
             heap_memory_size: 16 * 64 * 1024,
             heap_address_space_size: 0x200000000,
             stack_size: 128 * 1024,
             globals_size: 4096,
+            signal_stack_size: DEFAULT_SIGNAL_STACK_SIZE,
         }
     }
 }
@@ -370,19 +441,18 @@ impl Limits {
         // * the instance (up to instance_heap_offset)
         // * the heap, followed by guard pages
         // * the stack (grows towards heap guard pages)
-        // * one guard page (for good luck?)
         // * globals
         // * one guard page (to catch signal stack overflow)
-        // * the signal stack (size given by signal.h SIGSTKSZ macro)
+        // * the signal stack
 
         [
             instance_heap_offset(),
             self.heap_address_space_size,
-            self.stack_size,
             host_page_size(),
+            self.stack_size,
             self.globals_size,
             host_page_size(),
-            SIGSTKSZ,
+            self.signal_stack_size,
         ]
         .iter()
         .try_fold(0usize, |acc, &x| acc.checked_add(x))
@@ -419,6 +489,11 @@ impl Limits {
         if self.stack_size <= 0 {
             return Err(Error::InvalidArgument("stack size must be greater than 0"));
         }
+        if self.signal_stack_size % host_page_size() != 0 {
+            return Err(Error::InvalidArgument(
+                "signal stack size must be a multiple of host page size",
+            ));
+        }
         Ok(())
     }
 }

lucet-runtime/lucet-runtime-internals/src/alloc/tests.rs

@@ -21,6 +21,7 @@ macro_rules! alloc_tests {
             heap_address_space_size: LIMITS_HEAP_ADDRSPACE_SIZE,
             stack_size: LIMITS_STACK_SIZE,
             globals_size: LIMITS_GLOBALS_SIZE,
+            ..Limits::default()
         };
 
         const SPEC_HEAP_RESERVED_SIZE: u64 = LIMITS_HEAP_ADDRSPACE_SIZE as u64 / 2;
@@ -264,6 +265,7 @@ macro_rules! alloc_tests {
                 heap_address_space_size: LIMITS_HEAP_ADDRSPACE_SIZE,
                 stack_size: LIMITS_STACK_SIZE,
                 globals_size: LIMITS_GLOBALS_SIZE,
+                ..Limits::default()
             };
             let res = TestRegion::create(10, &LIMITS);
             assert!(res.is_err(), "region creation fails");
@@ -366,7 +368,7 @@ macro_rules! alloc_tests {
                 }
 
                 let sigstack = unsafe { inst.alloc_mut().sigstack_mut() };
-                assert_eq!(sigstack.len(), libc::SIGSTKSZ);
+                assert_eq!(sigstack.len(), LIMITS.signal_stack_size);
 
                 assert_eq!(sigstack[0], 0);
                 sigstack[0] = 0xFF;
@@ -569,6 +571,7 @@ macro_rules! alloc_tests {
             heap_address_space_size: 2 * 4096,
             stack_size: 4096,
             globals_size: 4096,
+            ..Limits::default()
         };
         const CONTEXT_TEST_INITIAL_SIZE: u64 = 4096;
         const CONTEXT_TEST_HEAP: HeapSpec = HeapSpec {

lucet-runtime/lucet-runtime-internals/src/c_api.rs

@@ -140,6 +140,16 @@ pub struct lucet_alloc_limits {
     pub stack_size: u64,
     /// Size of the globals region in bytes; each global uses 8 bytes. (default 4K)
     pub globals_size: u64,
+    /// Size of the signal stack in bytes. (default SIGSTKSZ for Rust release builds, 12K for Rust
+    /// debug builds)
+    ///
+    /// This difference is to account for the greatly increased stack size usage in the signal
+    /// handler when running without optimizations.
+    ///
+    /// Note that debug vs. release mode is determined by `cfg(debug_assertions)`, so if you are
+    /// specifically enabling Rust debug assertions in your Cargo release builds, the default signal
+    /// stack will be larger.
+    pub signal_stack_size: u64,
 }
 
 impl From<Limits> for lucet_alloc_limits {
@@ -155,6 +165,7 @@ impl From<&Limits> for lucet_alloc_limits {
             heap_address_space_size: limits.heap_address_space_size as u64,
             stack_size: limits.stack_size as u64,
             globals_size: limits.globals_size as u64,
+            signal_stack_size: limits.signal_stack_size as u64,
         }
     }
 }
@@ -172,6 +183,7 @@ impl From<&lucet_alloc_limits> for Limits {
             heap_address_space_size: limits.heap_address_space_size as usize,
             stack_size: limits.stack_size as usize,
             globals_size: limits.globals_size as usize,
+            signal_stack_size: limits.signal_stack_size as usize,
         }
     }
 }

lucet-runtime/lucet-runtime-internals/src/instance/signals.rs

@@ -61,7 +61,7 @@ impl Instance {
         let guest_sigstack = SigStack::new(
             self.alloc.slot().sigstack,
             SigStackFlags::empty(),
-            libc::SIGSTKSZ,
+            self.alloc.slot().limits.signal_stack_size,
         );
         let previous_sigstack = unsafe { sigaltstack(Some(guest_sigstack)) }
             .expect("enabling or changing the signal stack succeeds");
@@ -218,10 +218,13 @@ extern "C" fn handle_signal(signum: c_int, siginfo_ptr: *mut siginfo_t, ucontext
                     // If the trap table lookup returned unknown, it is a fatal error
                     let unknown_fault = trapcode.is_none();
 
-                    // If the trap was a segv or bus fault and the addressed memory was outside the
-                    // guard pages, it is also a fatal error
+                    // If the trap was a segv or bus fault and the addressed memory was in the
+                    // signal stack guard page or outside the alloc entirely, the fault is fatal
                     let outside_guard = (siginfo.si_signo == SIGSEGV || siginfo.si_signo == SIGBUS)
-                        && !inst.alloc.addr_in_guard_page(siginfo.si_addr_ext());
+                        && inst
+                            .alloc
+                            .addr_location(siginfo.si_addr_ext())
+                            .is_fault_fatal();
 
                     // record the fault and jump back to the host context
                     inst.state = State::Faulted {