Skip to content
This repository was archived by the owner on Mar 24, 2022. It is now read-only.

Commit ae348d2

Browse files
acfoltzeracfoltzer
authored
Merge pull request #413 from bytecodealliance/acf/minsigstksz
Check sigstack size against MINSIGSTKSZ and use a larger sigstack in debug mode on macOS
2 parents e8e5f9c + 6a3bff2 commit ae348d2

5 files changed

Lines changed: 101 additions & 9 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@
22

33
- Added `free_slots()`, `used_slots()`, and `capacity()` methods to the `Region` trait.
44

5+
- Added a check to ensure the `Limits` signal stack size is at least `MINSIGSTKSZ`, and increased
6+
the default signal stack size on macOS debug builds to fit this constraint.
7+
58
### 0.5.1 (2020-01-24)
69

710
- Fixed a memory corruption bug that could arise in certain runtime

‎lucet-runtime/include/lucet_types.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ struct lucet_alloc_limits {
124124
*/
125125
uint64_t globals_size;
126126
/**
127-
* Size of the signal stack region in bytes.
127+
* Size of the signal stack region in bytes. (minimum MINSIGSTKSZ)
128128
*
129129
* SIGSTKSZ from <signals.h> is a good default when linking against a Rust release build of
130130
* lucet-runtime, but 12K or more is recommended when using a Rust debug build.

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

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -407,19 +407,34 @@ pub struct Limits {
407407
pub stack_size: usize,
408408
/// Size of the globals region in bytes; each global uses 8 bytes. (default 4K)
409409
pub globals_size: usize,
410-
/// Size of the signal stack in bytes. (default SIGSTKSZ for release builds, 12K for debug builds)
410+
/// Size of the signal stack in bytes. (default SIGSTKSZ for release builds, at least 12K for
411+
/// debug builds; minimum MINSIGSTKSZ)
411412
///
412413
/// This difference is to account for the greatly increased stack size usage in the signal
413414
/// handler when running without optimizations.
414415
///
415416
/// Note that debug vs. release mode is determined by `cfg(debug_assertions)`, so if you are
416-
/// specifically enabling debug assertions in your release builds, the default signal stack will
417+
/// specifically enabling debug assertions in your release builds, the default signal stack may
417418
/// be larger.
418419
pub signal_stack_size: usize,
419420
}
420421

421-
#[cfg(debug_assertions)]
422+
// this constant isn't exported by `libc` on Mac
423+
#[cfg(target_os = "macos")]
424+
pub const MINSIGSTKSZ: usize = 32 * 1024;
425+
426+
#[cfg(not(target_os = "macos"))]
427+
pub const MINSIGSTKSZ: usize = libc::MINSIGSTKSZ;
428+
429+
// on Linux, `SIGSTKSZ` is too small for the signal handler when compiled in debug mode
430+
#[cfg(all(debug_assertions, not(target_os = "macos")))]
422431
pub const DEFAULT_SIGNAL_STACK_SIZE: usize = 12 * 1024;
432+
433+
// on Mac, `SIGSTKSZ` is way larger than we need; it would be nice to combine these debug cases once
434+
// `std::cmp::max` is a const fn
435+
#[cfg(all(debug_assertions, target_os = "macos"))]
436+
pub const DEFAULT_SIGNAL_STACK_SIZE: usize = libc::SIGSTKSZ;
437+
423438
#[cfg(not(debug_assertions))]
424439
pub const DEFAULT_SIGNAL_STACK_SIZE: usize = libc::SIGSTKSZ;
425440

@@ -489,6 +504,16 @@ impl Limits {
489504
if self.stack_size <= 0 {
490505
return Err(Error::InvalidArgument("stack size must be greater than 0"));
491506
}
507+
if self.signal_stack_size < MINSIGSTKSZ {
508+
return Err(Error::InvalidArgument(
509+
"signal stack size must be at least MINSIGSTKSZ (defined in <signal.h>)",
510+
));
511+
}
512+
if cfg!(debug_assertions) && self.signal_stack_size < 12 * 1024 {
513+
return Err(Error::InvalidArgument(
514+
"signal stack size must be at least 12KiB for debug builds",
515+
));
516+
}
492517
if self.signal_stack_size % host_page_size() != 0 {
493518
return Err(Error::InvalidArgument(
494519
"signal stack size must be a multiple of host page size",

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

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,9 @@ macro_rules! alloc_tests {
44
use libc::c_void;
55
use std::sync::Arc;
66
use $TestRegion as TestRegion;
7-
use $crate::alloc::Limits;
7+
use $crate::alloc::{host_page_size, Limits, MINSIGSTKSZ};
88
use $crate::context::{Context, ContextHandle};
9+
use $crate::error::Error;
910
use $crate::instance::InstanceInternal;
1011
use $crate::module::{GlobalValue, HeapSpec, MockModuleBuilder};
1112
use $crate::region::Region;
@@ -713,6 +714,69 @@ macro_rules! alloc_tests {
713714
assert_eq!(region.free_slots(), 2);
714715
assert_eq!(region.used_slots(), 0);
715716
}
717+
718+
#[test]
719+
fn reject_sigstack_smaller_than_min() {
720+
if MINSIGSTKSZ == 0 {
721+
// can't trigger the error on this platform
722+
return;
723+
}
724+
let limits = Limits {
725+
// keep it page-aligned but make it too small
726+
signal_stack_size: (MINSIGSTKSZ.checked_sub(1).unwrap() / host_page_size())
727+
* host_page_size(),
728+
..Limits::default()
729+
};
730+
let res = TestRegion::create(1, &limits);
731+
match res {
732+
Err(Error::InvalidArgument(
733+
"signal stack size must be at least MINSIGSTKSZ (defined in <signal.h>)",
734+
)) => (),
735+
Err(e) => panic!("unexpected error: {}", e),
736+
Ok(_) => panic!("unexpected success"),
737+
}
738+
}
739+
740+
/// This test ensures that a signal stack smaller than 12KiB is rejected when Lucet is
741+
/// compiled in debug mode.
742+
#[test]
743+
#[cfg(debug_assertions)]
744+
fn reject_debug_sigstack_smaller_than_12kib() {
745+
if 8192 < MINSIGSTKSZ {
746+
// can't trigger the error on this platform, as the MINSIGSTKSZ check runs first
747+
return;
748+
}
749+
let limits = Limits {
750+
signal_stack_size: 8192,
751+
..Limits::default()
752+
};
753+
let res = TestRegion::create(1, &limits);
754+
match res {
755+
Err(Error::InvalidArgument(
756+
"signal stack size must be at least 12KiB for debug builds",
757+
)) => (),
758+
Err(e) => panic!("unexpected error: {}", e),
759+
Ok(_) => panic!("unexpected success"),
760+
}
761+
}
762+
763+
#[test]
764+
fn reject_unaligned_sigstack() {
765+
let limits = Limits {
766+
signal_stack_size: std::cmp::max(libc::SIGSTKSZ, 12 * 1024)
767+
.checked_add(1)
768+
.unwrap(),
769+
..Limits::default()
770+
};
771+
let res = TestRegion::create(1, &limits);
772+
match res {
773+
Err(Error::InvalidArgument(
774+
"signal stack size must be a multiple of host page size",
775+
)) => (),
776+
Err(e) => panic!("unexpected error: {}", e),
777+
Ok(_) => panic!("unexpected success"),
778+
}
779+
}
716780
};
717781
}
718782

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -140,15 +140,15 @@ pub struct lucet_alloc_limits {
140140
pub stack_size: u64,
141141
/// Size of the globals region in bytes; each global uses 8 bytes. (default 4K)
142142
pub globals_size: u64,
143-
/// Size of the signal stack in bytes. (default SIGSTKSZ for Rust release builds, 12K for Rust
144-
/// debug builds)
143+
/// Size of the signal stack in bytes. (default SIGSTKSZ for release builds, at least 12K for
144+
/// debug builds; minimum MINSIGSTKSZ)
145145
///
146146
/// This difference is to account for the greatly increased stack size usage in the signal
147147
/// handler when running without optimizations.
148148
///
149149
/// Note that debug vs. release mode is determined by `cfg(debug_assertions)`, so if you are
150-
/// specifically enabling Rust debug assertions in your Cargo release builds, the default signal
151-
/// stack will be larger.
150+
/// specifically enabling debug assertions in your release builds, the default signal stack may
151+
/// be larger.
152152
pub signal_stack_size: u64,
153153
}
154154

0 commit comments

Comments
 (0)