From adfeaeab49431d53e5fdcb7e549cfa9aab330095 Mon Sep 17 00:00:00 2001 From: kyren Date: Thu, 8 Mar 2018 10:59:50 -0500 Subject: [PATCH] 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. --- README.md | 12 +++++ src/lua.rs | 7 ++- src/macros.rs | 62 ++++++-------------------- src/string.rs | 3 +- src/userdata.rs | 3 +- src/util.rs | 114 +++++++++++++++++++++++++----------------------- 6 files changed, 91 insertions(+), 110 deletions(-) diff --git a/README.md b/README.md index 6b40dfe..496328f 100644 --- a/README.md +++ b/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. diff --git a/src/lua.rs b/src/lua.rs index 0b5ade3..b85db14 100644 --- a/src/lua.rs +++ b/src/lua.rs @@ -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 } diff --git a/src/macros.rs b/src/macros.rs index f5342e2..fb10a07 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -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)+); } }; } diff --git a/src/string.rs b/src/string.rs index 31e937b..98b6699 100644 --- a/src/string.rs +++ b/src/string.rs @@ -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" ); diff --git a/src/userdata.rs b/src/userdata.rs index e29aa59..b20561a 100644 --- a/src/userdata.rs +++ b/src/userdata.rs @@ -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" ); diff --git a/src/util.rs b/src/util.rs index b8964b4..9295553 100644 --- a/src/util.rs +++ b/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(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(state: *mut ffi::lua_State, change: c_int, o where F: FnOnce() -> Result, { - 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::(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(state: *mut ffi::lua_State, t: T) -> Result<()> { pub unsafe fn get_userdata(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(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(state: *mut ffi::lua_State) -> c // the rust side, it will resume the panic. pub unsafe fn callback_error(state: *mut ffi::lua_State, f: F) -> R where - F: FnOnce() -> Result + UnwindSafe, + F: FnOnce() -> Result, { - 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)