Change strategies for handling the Lua stack during panics
Previously, on an internal panic, the Lua stack would be reset before panicking in an attempt to make sure that such panics would not cause stack leaks or leave the stack in an unknown state. Now, such panic handling is done in stack_guard and stack_err_guard instead, and this is for a few reasons: 1) The previous approach did NOT handle user triggered panics that were outside of `rlua`, such as a panic in a ToLua / FromLua implementation. This is especially bad since most other panics would be indicative of an internal bug anyway, so the utility of keeping `rlua` types usable after such panics was questionable. It is much more sensible to ensure that `rlua` types are usable after *user generated* panics. 2) Every entry point into `rlua` should be guarded by a stack_guard or stack_err_guard anyway, so this should restore the Lua stack on exiting back to user code in all cases. 3) The method of stack restoration no longer *clears* the stack, only resets it to what it previously was. This allows us, potentially, to keep values at the beginning of the Lua stack long term and know that panics will not clobber them. There may be a way of dramatically speeding up ref types by using a small static area at the beginning of the stack instead of only the registry, so this may be important.
This commit is contained in:
parent
6a0264169a
commit
adfeaeab49
12
README.md
12
README.md
|
@ -97,6 +97,12 @@ This is done by overriding the normal Lua 'pcall' and 'xpcall' with custom
|
|||
versions that cannot catch errors that are actually from Rust panics, and by
|
||||
handling panic errors on the receiving Rust side by resuming the panic.
|
||||
|
||||
`rlua` should also be panic safe in another way as well, which is that any `Lua`
|
||||
instances or handles should remain usable after a user triggered panic, and such
|
||||
panics should not break internal invariants or leak Lua stack space. This is
|
||||
mostly important to safely use `rlua` types in Drop impls, as you should not be
|
||||
using panics for general error handling.
|
||||
|
||||
In summary, here is a list of `rlua` behaviors that should be considered a bug.
|
||||
If you encounter them, a bug report would be very welcome:
|
||||
|
||||
|
@ -124,3 +130,9 @@ If you encounter them, a bug report would be very welcome:
|
|||
longjmp over your Rust stack frames, this is a bug!
|
||||
* If you can somehow handle a panic in a Rust callback from Lua, this is a
|
||||
bug.
|
||||
* If you detect that, after catching a panic, a `Lua` or handle method is
|
||||
triggering other bugs or there is a Lua stack space leak, this is a bug.
|
||||
`rlua` instances are supposed to remain fully usable in the face of user
|
||||
triggered panics. This guarantee does NOT extend to panics marked with
|
||||
"rlua internal error" simply because that is already indicative of a
|
||||
separate bug, but it may be true in many cases anyway.
|
||||
|
|
|
@ -55,7 +55,7 @@ impl Drop for Lua {
|
|||
if cfg!(test) {
|
||||
let top = ffi::lua_gettop(self.state);
|
||||
if top != 0 {
|
||||
lua_internal_abort!("Lua stack leak detected, stack top is {}", top);
|
||||
rlua_abort!("Lua stack leak detected, stack top is {}", top);
|
||||
}
|
||||
}
|
||||
|
||||
|
@ -727,8 +727,7 @@ impl Lua {
|
|||
|
||||
// Used 1 stack space, does not call checkstack
|
||||
pub(crate) unsafe fn push_ref(&self, state: *mut ffi::lua_State, lref: &LuaRef) {
|
||||
lua_assert!(
|
||||
state,
|
||||
rlua_assert!(
|
||||
lref.lua.main_state == self.main_state,
|
||||
"Lua instance passed Value created from a different Lua"
|
||||
);
|
||||
|
@ -912,7 +911,7 @@ impl Lua {
|
|||
// not really a huge loss. Importantly, this allows us to turn off the gc, and
|
||||
// then know that calling Lua API functions marked as 'm' will not result in a
|
||||
// 'longjmp' error while the gc is off.
|
||||
lua_abort!("out of memory in Lua allocation, aborting!");
|
||||
abort!("out of memory in Lua allocation, aborting!");
|
||||
} else {
|
||||
p as *mut c_void
|
||||
}
|
||||
|
|
|
@ -4,41 +4,7 @@ macro_rules! cstr {
|
|||
);
|
||||
}
|
||||
|
||||
// A panic that clears the given lua stack before panicking
|
||||
macro_rules! lua_panic {
|
||||
($state:expr, $msg:expr) => {
|
||||
{
|
||||
$crate::ffi::lua_settop($state, 0);
|
||||
panic!($msg);
|
||||
}
|
||||
};
|
||||
|
||||
($state:expr, $msg:expr, $($arg:tt)+) => {
|
||||
{
|
||||
$crate::ffi::lua_settop($state, 0);
|
||||
panic!($msg, $($arg)+);
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
// An assert that clears the given lua stack before panicking
|
||||
macro_rules! lua_assert {
|
||||
($state:expr, $cond:expr, $msg:expr) => {
|
||||
if !$cond {
|
||||
$crate::ffi::lua_settop($state, 0);
|
||||
panic!($msg);
|
||||
}
|
||||
};
|
||||
|
||||
($state:expr, $cond:expr, $msg:expr, $($arg:tt)+) => {
|
||||
if !$cond {
|
||||
$crate::ffi::lua_settop($state, 0);
|
||||
panic!($msg, $($arg)+);
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
macro_rules! lua_abort {
|
||||
macro_rules! abort {
|
||||
($msg:expr) => {
|
||||
{
|
||||
eprintln!($msg);
|
||||
|
@ -54,36 +20,36 @@ macro_rules! lua_abort {
|
|||
};
|
||||
}
|
||||
|
||||
macro_rules! lua_internal_panic {
|
||||
($state:expr, $msg:expr) => {
|
||||
lua_panic!($state, concat!("rlua internal error: ", $msg));
|
||||
macro_rules! rlua_panic {
|
||||
($msg:expr) => {
|
||||
panic!(concat!("rlua internal error: ", $msg));
|
||||
};
|
||||
|
||||
($state:expr, $msg:expr, $($arg:tt)+) => {
|
||||
lua_panic!($state, concat!("rlua internal error: ", $msg), $($arg)+);
|
||||
($msg:expr, $($arg:tt)+) => {
|
||||
panic!(concat!("rlua internal error: ", $msg), $($arg)+);
|
||||
};
|
||||
}
|
||||
|
||||
macro_rules! lua_internal_assert {
|
||||
($state:expr, $cond:expr, $msg:expr) => {
|
||||
lua_assert!($state, $cond, concat!("rlua internal error: ", $msg));
|
||||
macro_rules! rlua_assert {
|
||||
($cond:expr, $msg:expr) => {
|
||||
assert!($cond, concat!("rlua internal error: ", $msg));
|
||||
};
|
||||
|
||||
($state:expr, $cond:expr, $msg:expr, $($arg:tt)+) => {
|
||||
lua_assert!($state, $cond, concat!("rlua internal error: ", $msg), $($arg)+);
|
||||
($cond:expr, $msg:expr, $($arg:tt)+) => {
|
||||
assert!($cond, concat!("rlua internal error: ", $msg), $($arg)+);
|
||||
};
|
||||
}
|
||||
|
||||
macro_rules! lua_internal_abort {
|
||||
macro_rules! rlua_abort {
|
||||
($msg:expr) => {
|
||||
{
|
||||
lua_abort!(concat!("rlua internal error: ", $msg));
|
||||
abort!(concat!("rlua internal error: ", $msg));
|
||||
}
|
||||
};
|
||||
|
||||
($msg:expr, $($arg:tt)+) => {
|
||||
{
|
||||
lua_abort!(concat!("rlua internal error, aborting!: ", $msg), $($arg)+);
|
||||
abort!(concat!("rlua internal error, aborting!: ", $msg), $($arg)+);
|
||||
}
|
||||
};
|
||||
}
|
||||
|
|
|
@ -72,8 +72,7 @@ impl<'lua> String<'lua> {
|
|||
stack_guard(lua.state, 0, || {
|
||||
check_stack(lua.state, 1);
|
||||
lua.push_ref(lua.state, &self.0);
|
||||
lua_internal_assert!(
|
||||
lua.state,
|
||||
rlua_assert!(
|
||||
ffi::lua_type(lua.state, -1) == ffi::LUA_TSTRING,
|
||||
"string ref is not string type"
|
||||
);
|
||||
|
|
|
@ -420,8 +420,7 @@ impl<'lua> AnyUserData<'lua> {
|
|||
|
||||
lua.push_ref(lua.state, &self.0);
|
||||
|
||||
lua_internal_assert!(
|
||||
lua.state,
|
||||
rlua_assert!(
|
||||
ffi::lua_getmetatable(lua.state, -1) != 0,
|
||||
"AnyUserData missing metatable"
|
||||
);
|
||||
|
|
114
src/util.rs
114
src/util.rs
|
@ -3,16 +3,15 @@ use std::sync::Arc;
|
|||
use std::ffi::CStr;
|
||||
use std::any::Any;
|
||||
use std::os::raw::{c_char, c_int, c_void};
|
||||
use std::panic::{catch_unwind, resume_unwind, UnwindSafe};
|
||||
use std::panic::{catch_unwind, resume_unwind, AssertUnwindSafe};
|
||||
|
||||
use ffi;
|
||||
use error::{Error, Result};
|
||||
|
||||
// Checks that Lua has enough free stack space for future stack operations. On failure, this will
|
||||
// clear the stack and panic.
|
||||
// panic.
|
||||
pub unsafe fn check_stack(state: *mut ffi::lua_State, amount: c_int) {
|
||||
lua_internal_assert!(
|
||||
state,
|
||||
rlua_assert!(
|
||||
ffi::lua_checkstack(state, amount) != 0,
|
||||
"out of stack space"
|
||||
);
|
||||
|
@ -28,29 +27,35 @@ pub unsafe fn check_stack_err(state: *mut ffi::lua_State, amount: c_int) -> Resu
|
|||
}
|
||||
}
|
||||
|
||||
// Run an operation on a lua_State and check that the stack change is what is
|
||||
// expected. If the stack change does not match, clears the stack and panics.
|
||||
// Run an operation on a lua_State and check that the stack change is what is expected. If the
|
||||
// stack change does not match, resets the stack and panics. If the given operation panics, tries
|
||||
// to restore the stack to its previous state before resuming the panic.
|
||||
pub unsafe fn stack_guard<F, R>(state: *mut ffi::lua_State, change: c_int, op: F) -> R
|
||||
where
|
||||
F: FnOnce() -> R,
|
||||
{
|
||||
let expected = ffi::lua_gettop(state) + change;
|
||||
lua_internal_assert!(
|
||||
state,
|
||||
expected >= 0,
|
||||
"too many stack values would be popped"
|
||||
);
|
||||
let begin = ffi::lua_gettop(state);
|
||||
let expected = begin + change;
|
||||
rlua_assert!(expected >= 0, "too many stack values would be popped");
|
||||
|
||||
let res = op();
|
||||
let res = match catch_unwind(AssertUnwindSafe(op)) {
|
||||
Ok(r) => r,
|
||||
Err(p) => {
|
||||
let top = ffi::lua_gettop(state);
|
||||
if top > begin {
|
||||
ffi::lua_settop(state, begin);
|
||||
}
|
||||
resume_unwind(p);
|
||||
}
|
||||
};
|
||||
|
||||
let top = ffi::lua_gettop(state);
|
||||
lua_internal_assert!(
|
||||
state,
|
||||
ffi::lua_gettop(state) == expected,
|
||||
"expected stack to be {}, got {}",
|
||||
expected,
|
||||
top
|
||||
);
|
||||
if top != expected {
|
||||
if top > begin {
|
||||
ffi::lua_settop(state, begin);
|
||||
}
|
||||
rlua_panic!("expected stack to be {}, got {}", expected, top);
|
||||
}
|
||||
|
||||
res
|
||||
}
|
||||
|
@ -66,33 +71,34 @@ pub unsafe fn stack_err_guard<F, R>(state: *mut ffi::lua_State, change: c_int, o
|
|||
where
|
||||
F: FnOnce() -> Result<R>,
|
||||
{
|
||||
let expected = ffi::lua_gettop(state) + change;
|
||||
lua_internal_assert!(
|
||||
state,
|
||||
expected >= 0,
|
||||
"too many stack values would be popped"
|
||||
);
|
||||
let begin = ffi::lua_gettop(state);
|
||||
let expected = begin + change;
|
||||
rlua_assert!(expected >= 0, "too many stack values would be popped");
|
||||
|
||||
let res = op();
|
||||
let res = match catch_unwind(AssertUnwindSafe(op)) {
|
||||
Ok(r) => r,
|
||||
Err(p) => {
|
||||
let top = ffi::lua_gettop(state);
|
||||
if top > begin {
|
||||
ffi::lua_settop(state, begin);
|
||||
}
|
||||
resume_unwind(p);
|
||||
}
|
||||
};
|
||||
|
||||
let top = ffi::lua_gettop(state);
|
||||
if res.is_ok() {
|
||||
lua_internal_assert!(
|
||||
state,
|
||||
ffi::lua_gettop(state) == expected,
|
||||
"expected stack to be {}, got {}",
|
||||
expected,
|
||||
top
|
||||
);
|
||||
if top != expected {
|
||||
if top > begin {
|
||||
ffi::lua_settop(state, begin);
|
||||
}
|
||||
rlua_panic!("expected stack to be {}, got {}", expected, top);
|
||||
}
|
||||
} else {
|
||||
lua_internal_assert!(
|
||||
state,
|
||||
top >= expected,
|
||||
"{} too many stack values popped",
|
||||
top - expected
|
||||
);
|
||||
if top > expected {
|
||||
ffi::lua_settop(state, expected);
|
||||
} else if top < expected {
|
||||
rlua_panic!("{} too many stack values popped", top - begin - change);
|
||||
}
|
||||
}
|
||||
res
|
||||
|
@ -162,13 +168,14 @@ where
|
|||
}
|
||||
}
|
||||
|
||||
// Pops an error off of the stack and returns it. If the error is actually a WrappedPanic, clears
|
||||
// the current lua stack and continues the panic. If the error on the top of the stack is actually
|
||||
// a WrappedError, just returns it. Otherwise, interprets the error as the appropriate lua error.
|
||||
// Pops an error off of the stack and returns it. The specific behavior depends on the type of the
|
||||
// error at the top of the stack:
|
||||
// 1) If the error is actually a WrappedPanic, this will continue the panic.
|
||||
// 2) If the error on the top of the stack is actually a WrappedError, just returns it.
|
||||
// 3) Otherwise, interprets the error as the appropriate lua error.
|
||||
// Uses 2 stack spaces, does not call lua_checkstack.
|
||||
pub unsafe fn pop_error(state: *mut ffi::lua_State, err_code: c_int) -> Error {
|
||||
lua_internal_assert!(
|
||||
state,
|
||||
rlua_assert!(
|
||||
err_code != ffi::LUA_OK && err_code != ffi::LUA_YIELD,
|
||||
"pop_error called with non-error return code"
|
||||
);
|
||||
|
@ -178,10 +185,9 @@ pub unsafe fn pop_error(state: *mut ffi::lua_State, err_code: c_int) -> Error {
|
|||
} else if is_wrapped_panic(state, -1) {
|
||||
let panic = get_userdata::<WrappedPanic>(state, -1);
|
||||
if let Some(p) = (*panic).0.take() {
|
||||
ffi::lua_settop(state, 0);
|
||||
resume_unwind(p);
|
||||
} else {
|
||||
lua_internal_panic!(state, "panic was resumed twice")
|
||||
rlua_panic!("panic was resumed twice")
|
||||
}
|
||||
} else {
|
||||
let err_string = gc_guard(state, || {
|
||||
|
@ -213,10 +219,10 @@ pub unsafe fn pop_error(state: *mut ffi::lua_State, err_code: c_int) -> Error {
|
|||
ffi::LUA_ERRMEM => {
|
||||
// This should be impossible, as we set the lua allocator to one that aborts
|
||||
// instead of failing.
|
||||
lua_internal_abort!("impossible Lua allocation error, aborting!")
|
||||
rlua_abort!("impossible Lua allocation error, aborting!")
|
||||
}
|
||||
ffi::LUA_ERRGCMM => Error::GarbageCollectorError(err_string),
|
||||
_ => lua_internal_panic!(state, "unrecognized lua error code"),
|
||||
_ => rlua_panic!("unrecognized lua error code"),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
@ -239,7 +245,7 @@ pub unsafe fn push_userdata<T>(state: *mut ffi::lua_State, t: T) -> Result<()> {
|
|||
|
||||
pub unsafe fn get_userdata<T>(state: *mut ffi::lua_State, index: c_int) -> *mut T {
|
||||
let ud = ffi::lua_touserdata(state, index) as *mut T;
|
||||
lua_internal_assert!(state, !ud.is_null(), "userdata pointer is null");
|
||||
rlua_assert!(!ud.is_null(), "userdata pointer is null");
|
||||
ud
|
||||
}
|
||||
|
||||
|
@ -253,7 +259,7 @@ pub unsafe fn take_userdata<T>(state: *mut ffi::lua_State) -> T {
|
|||
get_destructed_userdata_metatable(state);
|
||||
ffi::lua_setmetatable(state, -2);
|
||||
let ud = ffi::lua_touserdata(state, -1) as *mut T;
|
||||
lua_internal_assert!(state, !ud.is_null(), "userdata pointer is null");
|
||||
rlua_assert!(!ud.is_null(), "userdata pointer is null");
|
||||
ffi::lua_pop(state, 1);
|
||||
ptr::read(ud)
|
||||
}
|
||||
|
@ -271,9 +277,9 @@ pub unsafe extern "C" fn userdata_destructor<T>(state: *mut ffi::lua_State) -> c
|
|||
// the rust side, it will resume the panic.
|
||||
pub unsafe fn callback_error<R, F>(state: *mut ffi::lua_State, f: F) -> R
|
||||
where
|
||||
F: FnOnce() -> Result<R> + UnwindSafe,
|
||||
F: FnOnce() -> Result<R>,
|
||||
{
|
||||
match catch_unwind(f) {
|
||||
match catch_unwind(AssertUnwindSafe(f)) {
|
||||
Ok(Ok(r)) => r,
|
||||
Ok(Err(err)) => {
|
||||
ffi::lua_settop(state, 0);
|
||||
|
@ -284,7 +290,7 @@ where
|
|||
Err(p) => {
|
||||
ffi::lua_settop(state, 0);
|
||||
if ffi::lua_checkstack(state, 2) == 0 {
|
||||
lua_internal_abort!("not enough stack space to propagate panic");
|
||||
rlua_abort!("not enough stack space to propagate panic");
|
||||
}
|
||||
push_wrapped_panic(state, p);
|
||||
ffi::lua_error(state)
|
||||
|
|
Loading…
Reference in New Issue