Merge pull request #413 from bytecodealliance/acf/minsigstksz

Check sigstack size against MINSIGSTKSZ and use a larger sigstack in debug mode on macOS

Author: acfoltzer

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 - Added `free_slots()`, `used_slots()`, and `capacity()` methods to the `Region` trait.
 
+- Added a check to ensure the `Limits` signal stack size is at least `MINSIGSTKSZ`, and increased
+  the default signal stack size on macOS debug builds to fit this constraint.
+
 ### 0.5.1 (2020-01-24)
 
 - Fixed a memory corruption bug that could arise in certain runtime

lucet-runtime/include/lucet_types.h

@@ -124,7 +124,7 @@ struct lucet_alloc_limits {
      */
     uint64_t globals_size;
     /**
-     * Size of the signal stack region in bytes.
+     * Size of the signal stack region in bytes. (minimum MINSIGSTKSZ)
      *
      * 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.

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

@@ -407,19 +407,34 @@ 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)
+    /// Size of the signal stack in bytes. (default SIGSTKSZ for release builds, at least 12K for
+    /// debug builds; minimum MINSIGSTKSZ)
     ///
     /// 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
+    /// specifically enabling debug assertions in your release builds, the default signal stack may
     /// be larger.
     pub signal_stack_size: usize,
 }
 
-#[cfg(debug_assertions)]
+// this constant isn't exported by `libc` on Mac
+#[cfg(target_os = "macos")]
+pub const MINSIGSTKSZ: usize = 32 * 1024;
+
+#[cfg(not(target_os = "macos"))]
+pub const MINSIGSTKSZ: usize = libc::MINSIGSTKSZ;
+
+// on Linux, `SIGSTKSZ` is too small for the signal handler when compiled in debug mode
+#[cfg(all(debug_assertions, not(target_os = "macos")))]
 pub const DEFAULT_SIGNAL_STACK_SIZE: usize = 12 * 1024;
+
+// on Mac, `SIGSTKSZ` is way larger than we need; it would be nice to combine these debug cases once
+// `std::cmp::max` is a const fn
+#[cfg(all(debug_assertions, target_os = "macos"))]
+pub const DEFAULT_SIGNAL_STACK_SIZE: usize = libc::SIGSTKSZ;
+
 #[cfg(not(debug_assertions))]
 pub const DEFAULT_SIGNAL_STACK_SIZE: usize = libc::SIGSTKSZ;
 
@@ -489,6 +504,16 @@ impl Limits {
         if self.stack_size <= 0 {
             return Err(Error::InvalidArgument("stack size must be greater than 0"));
         }
+        if self.signal_stack_size < MINSIGSTKSZ {
+            return Err(Error::InvalidArgument(
+                "signal stack size must be at least MINSIGSTKSZ (defined in <signal.h>)",
+            ));
+        }
+        if cfg!(debug_assertions) && self.signal_stack_size < 12 * 1024 {
+            return Err(Error::InvalidArgument(
+                "signal stack size must be at least 12KiB for debug builds",
+            ));
+        }
         if self.signal_stack_size % host_page_size() != 0 {
             return Err(Error::InvalidArgument(
                 "signal stack size must be a multiple of host page size",

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

@@ -4,8 +4,9 @@ macro_rules! alloc_tests {
         use libc::c_void;
         use std::sync::Arc;
         use $TestRegion as TestRegion;
-        use $crate::alloc::Limits;
+        use $crate::alloc::{host_page_size, Limits, MINSIGSTKSZ};
         use $crate::context::{Context, ContextHandle};
+        use $crate::error::Error;
         use $crate::instance::InstanceInternal;
         use $crate::module::{GlobalValue, HeapSpec, MockModuleBuilder};
         use $crate::region::Region;
@@ -713,6 +714,69 @@ macro_rules! alloc_tests {
             assert_eq!(region.free_slots(), 2);
             assert_eq!(region.used_slots(), 0);
         }
+
+        #[test]
+        fn reject_sigstack_smaller_than_min() {
+            if MINSIGSTKSZ == 0 {
+                // can't trigger the error on this platform
+                return;
+            }
+            let limits = Limits {
+                // keep it page-aligned but make it too small
+                signal_stack_size: (MINSIGSTKSZ.checked_sub(1).unwrap() / host_page_size())
+                    * host_page_size(),
+                ..Limits::default()
+            };
+            let res = TestRegion::create(1, &limits);
+            match res {
+                Err(Error::InvalidArgument(
+                    "signal stack size must be at least MINSIGSTKSZ (defined in <signal.h>)",
+                )) => (),
+                Err(e) => panic!("unexpected error: {}", e),
+                Ok(_) => panic!("unexpected success"),
+            }
+        }
+
+        /// This test ensures that a signal stack smaller than 12KiB is rejected when Lucet is
+        /// compiled in debug mode.
+        #[test]
+        #[cfg(debug_assertions)]
+        fn reject_debug_sigstack_smaller_than_12kib() {
+            if 8192 < MINSIGSTKSZ {
+                // can't trigger the error on this platform, as the MINSIGSTKSZ check runs first
+                return;
+            }
+            let limits = Limits {
+                signal_stack_size: 8192,
+                ..Limits::default()
+            };
+            let res = TestRegion::create(1, &limits);
+            match res {
+                Err(Error::InvalidArgument(
+                    "signal stack size must be at least 12KiB for debug builds",
+                )) => (),
+                Err(e) => panic!("unexpected error: {}", e),
+                Ok(_) => panic!("unexpected success"),
+            }
+        }
+
+        #[test]
+        fn reject_unaligned_sigstack() {
+            let limits = Limits {
+                signal_stack_size: std::cmp::max(libc::SIGSTKSZ, 12 * 1024)
+                    .checked_add(1)
+                    .unwrap(),
+                ..Limits::default()
+            };
+            let res = TestRegion::create(1, &limits);
+            match res {
+                Err(Error::InvalidArgument(
+                    "signal stack size must be a multiple of host page size",
+                )) => (),
+                Err(e) => panic!("unexpected error: {}", e),
+                Ok(_) => panic!("unexpected success"),
+            }
+        }
     };
 }
 

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

@@ -140,15 +140,15 @@ 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)
+    /// Size of the signal stack in bytes. (default SIGSTKSZ for release builds, at least 12K for
+    /// debug builds; minimum MINSIGSTKSZ)
     ///
     /// 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.
+    /// specifically enabling debug assertions in your release builds, the default signal stack may
+    /// be larger.
     pub signal_stack_size: u64,
 }