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

Commit

Permalink
lucet-runtime: pr review iii
Browse files Browse the repository at this point in the history
  • Loading branch information
@data-pup
data-pup committed Feb 25, 2020
1 parent 2e27187 commit b566730
Show file tree
Hide file tree
Showing 3 changed files with 69 additions and 103 deletions.
40 changes: 8 additions & 32 deletions lucet-runtime/lucet-runtime-internals/src/instance/execution.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,10 @@ pub unsafe extern "C" fn enter_guest_region(instance: *mut Instance) {
Domain::Cancelled => {
// A KillSwitch terminated our Instance before it began executing guest code. We should
// not enter the guest region. We will instead terminate the Instance, and then swap
// back to the host context. Note that `Instance::terminate` will never return.
// back to the host context.
//
// Note that we manually drop the domain because`Instance::terminate` never returns.
mem::drop(current_domain);
(*instance).terminate(TerminationDetails::Remote);
}
}
Expand Down Expand Up @@ -304,34 +307,6 @@ impl KillState {
*self.thread_id.lock().unwrap() = None;
self.tid_change_notifier.notify_all();
}

/// Check if the execution domain is currently locked.
///
/// This uses [`Mutex::try_lock`](std/sync/struct.Mutex.html#method.try_lock), to see if
/// acquiring a lock of the execution domain's mutex, would block the current thread. If
/// so, return `true`. If the mutex is not locked, or is in a poisoned state, return false.
///
/// This is used by an [`Instance`](struct.Instance.html)'s signal handler to check that a
/// `SIGALRM` received during execution came from a kill switch, rather than some other source.
pub(crate) fn domain_is_locked(&self) -> bool {
use std::sync::TryLockError::WouldBlock;
if let Err(WouldBlock) = self.execution_domain.try_lock() {
true
} else {
false
}
}

/// Acquire a lock on the execution domain.
///
/// This is used by an [`Instance`](struct.Instance.html)'s signal handler to disable
/// termination before switching to the host context, for signals _other than_ a `SIGALRM`
/// that came from a kill switch, rather than some other source.
pub(crate) fn execution_domain(
&self,
) -> std::sync::LockResult<std::sync::MutexGuard<'_, Domain>> {
self.execution_domain.lock()
}
}

/// Instance execution domains.
Expand All @@ -340,7 +315,6 @@ impl KillState {
#[derive(Debug, PartialEq)]
pub enum Domain {
/// Represents an instance that is not currently running.
/// FIXME KTM 2020-02-20: Maybe we should call this `Ready` or `Paused`.
Pending,
/// Represents an instance that is executing guest code.
Guest,
Expand Down Expand Up @@ -425,10 +399,12 @@ impl KillSwitch {
while curr_tid.is_some() {
curr_tid = state.tid_change_notifier.wait(curr_tid).unwrap();
}
*execution_domain = Domain::Terminated;
Ok(KillSuccess::Signalled)
} else {
// If we are here, we probably faulted without updating the execution domain.
panic!("logic error: instance is terminable but not actually running.");
// If the execution domain is marked as `Guest`, but there is not a thread
// running that we wan signal, the guest most likely faulted.
Err(KillError::NotTerminable)
}
}
Domain::Hostcall => {
Expand Down
35 changes: 2 additions & 33 deletions lucet-runtime/lucet-runtime-internals/src/instance/signals.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use crate::context::Context;
use crate::error::Error;
use crate::instance::execution;
use crate::instance::{
siginfo_ext::SiginfoExt, FaultDetails, Instance, State, TerminationDetails, CURRENT_INSTANCE,
HOST_CTX,
Expand Down Expand Up @@ -167,15 +166,9 @@ extern "C" fn handle_signal(signum: c_int, siginfo_ptr: *mut siginfo_t, ucontext
};

if signal == Signal::SIGALRM {
// If have gotten a SIGALRM, the KillSwitch that sent this signal must have acquired a
// lock on the execution domain, and currently be waiting for the signal handler (us)
// to deschedule the instance. If this assert fails, the SIGALRM came from some other
// source, or we have a bug that allows SIGALRM to be sent when they should not.
//
// set the state before jumping back to the host context
// TODO: once we have a notion of logging in `lucet-runtime`, this should be a logged
// error.
debug_assert!(inst.kill_state.domain_is_locked());
// set the state before jumping back to the host context
inst.state = State::Terminating {
details: TerminationDetails::Remote,
};
Expand All @@ -185,7 +178,7 @@ extern "C" fn handle_signal(signum: c_int, siginfo_ptr: *mut siginfo_t, ucontext
let trapcode = inst.module.lookup_trapcode(rip);

let behavior = (inst.signal_handler)(inst, &trapcode, signum, siginfo_ptr, ucontext_ptr);
let switch_to_host = match behavior {
match behavior {
SignalBehavior::Continue => {
// return to the guest context without making any modifications to the instance
false
Expand Down Expand Up @@ -246,31 +239,7 @@ extern "C" fn handle_signal(signum: c_int, siginfo_ptr: *mut siginfo_t, ucontext
thunk();
true
}
};
// If we are switching to the host context and we are not processing a SIGALRM from a
// KillSwitch, then we were either externally signalled in some other manner, or we faulted
// during execution in a guest region.
//
// In either case, we will need to unlock and update the execution domain ourselves.
if switch_to_host {
// This debug assert exists to make sure that this is not a blocking operation.
debug_assert_eq!(inst.kill_state.domain_is_locked(), false);
let current_domain = inst.kill_state.execution_domain();
match current_domain {
Ok(mut current_domain) => {
// If we are here, we should still be in the guest domain.
debug_assert_eq!(*current_domain, execution::Domain::Guest);
*current_domain = execution::Domain::Terminated;
}
Err(poison_error) => {
// Because a signal must never be sent while the host context is active, we
// will update the domain even if the mutex has been poisoned.
let mut current_domain = poison_error.into_inner();
*current_domain = execution::Domain::Terminated;
}
};
}
switch_to_host
});

if switch_to_host {
Expand Down
97 changes: 59 additions & 38 deletions lucet-runtime/lucet-runtime-tests/src/timeout.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,13 @@ macro_rules! timeout_tests {

let kill_switch = inst.kill_switch();

thread::spawn(move || {
thread::sleep(Duration::from_millis(100));
assert_eq!(kill_switch.terminate(), Ok(KillSuccess::Signalled));
});
thread::Builder::new()
.name("helper".to_owned())
.spawn(move || {
thread::sleep(Duration::from_millis(100));
assert_eq!(kill_switch.terminate(), Ok(KillSuccess::Signalled));
})
.expect("can spawn a thread");

match inst.run("infinite_loop", &[]) {
Err(Error::RuntimeTerminated(TerminationDetails::Remote)) => {
Expand Down Expand Up @@ -197,10 +200,13 @@ macro_rules! timeout_tests {

let kill_switch = inst.kill_switch();

thread::spawn(move || {
thread::sleep(Duration::from_millis(100));
assert_eq!(kill_switch.terminate(), Ok(KillSuccess::Pending));
});
thread::Builder::new()
.name("helper".to_owned())
.spawn(move || {
thread::sleep(Duration::from_millis(100));
assert_eq!(kill_switch.terminate(), Ok(KillSuccess::Pending));
})
.expect("can spawn a thread");

match inst.run("run_slow_hostcall", &[]) {
Err(Error::RuntimeTerminated(TerminationDetails::Remote)) => {}
Expand All @@ -223,10 +229,13 @@ macro_rules! timeout_tests {

let kill_switch = inst.kill_switch();

thread::spawn(move || {
thread::sleep(Duration::from_millis(100));
assert_eq!(kill_switch.terminate(), Ok(KillSuccess::Pending));
});
thread::Builder::new()
.name("helper".to_owned())
.spawn(move || {
thread::sleep(Duration::from_millis(100));
assert_eq!(kill_switch.terminate(), Ok(KillSuccess::Pending));
})
.expect("can spawn a thread");

match inst.run("run_yielding_hostcall", &[]) {
Ok(RunResult::Yielded(EmptyYieldVal)) => {}
Expand Down Expand Up @@ -260,23 +269,27 @@ macro_rules! timeout_tests {

let kill_switch = inst.kill_switch();

let t = thread::spawn(move || {
assert!(kill_switch.terminate().is_ok()); // works
thread::sleep(Duration::from_millis(100));
assert!(kill_switch.terminate().is_err()); // fails
});
let helper = thread::Builder::new()
.name("helper".to_owned())
.spawn(move || {
// This fires first, and will work properly, terminating the instance.
assert_eq!(kill_switch.terminate(), Ok(KillSuccess::Cancelled));
// Next, we will delay for 100ms and try to terminate the instance again.
// This should fail, because we have already terminated the instance.
thread::sleep(Duration::from_millis(100));
assert_eq!(kill_switch.terminate(), Err(KillError::NotTerminable));
})
.expect("can spawn a thread");

thread::sleep(Duration::from_millis(10));

match inst.run("main", &[0u32.into(), 0u32.into()]) {
// the result we're expecting - the guest has been terminated!
Err(Error::RuntimeTerminated(TerminationDetails::Remote)) => {}
res => {
panic!("unexpected result: {:?}", res);
}
res => panic!("unexpected result: {:?}", res),
};

t.join().unwrap();
helper.join().unwrap();
}

/// This test ensures that we see a more informative kill error than `NotTerminable` when
Expand Down Expand Up @@ -305,29 +318,37 @@ macro_rules! timeout_tests {
let kill_switch = inst.kill_switch();
let second_kill_switch = inst.kill_switch();

thread::spawn(move || {
thread::sleep(Duration::from_millis(100));
assert_eq!(kill_switch.terminate(), Ok(KillSuccess::Signalled));
});

thread::spawn(move || {
thread::sleep(Duration::from_millis(200));
assert_eq!(
second_kill_switch.terminate(),
Err(KillError::NotTerminable)
);
});
let helper_1 = thread::Builder::new()
.name("helper-1".to_owned())
.spawn(move || {
thread::sleep(Duration::from_millis(100));
assert_eq!(kill_switch.terminate(), Ok(KillSuccess::Signalled));
})
.expect("can spawn a thread");

let helper_2 = thread::Builder::new()
.name("helper-2".to_owned())
.spawn(move || {
thread::sleep(Duration::from_millis(200));
assert_eq!(
second_kill_switch.terminate(),
Err(KillError::NotTerminable)
);
})
.expect("can spawn a thread");

match inst.run("infinite_loop", &[]) {
Err(Error::RuntimeTerminated(TerminationDetails::Remote)) => {
// this is what we want to see
}
// the result we're expecting - the guest has been terminated!
Err(Error::RuntimeTerminated(TerminationDetails::Remote)) => {}
res => panic!("unexpected result: {:?}", res),
}

// after a timeout, can reset and run a normal function
inst.reset().expect("instance resets");
// Check that neither helper thread panicked.
helper_1.join().expect("helper_1 did not panic");
helper_2.join().expect("helper_2 did not panic");

// Check that we can reset the instance and run a function.
inst.reset().expect("instance resets");
run_onetwothree(&mut inst);
}
};
Expand Down

0 comments on commit b566730

Please sign in to comment.