From b9589491e420db5a7a50165cbac1e887911c2203 Mon Sep 17 00:00:00 2001 From: Alex Orlenko Date: Thu, 15 Apr 2021 23:04:36 +0100 Subject: [PATCH] Improve panic handling (check for twice resumed panics) --- src/error.rs | 8 ++++ src/lua.rs | 42 ++++++++++------- src/util.rs | 27 +++++------ tests/tests.rs | 126 ++++++++++++++++++++++++++++++------------------- 4 files changed, 123 insertions(+), 80 deletions(-) diff --git a/src/error.rs b/src/error.rs index ad58307..029d629 100644 --- a/src/error.rs +++ b/src/error.rs @@ -139,6 +139,11 @@ pub enum Error { /// Original error returned by the Rust code. cause: Arc, }, + /// A Rust panic that was previosly resumed, returned again. + /// + /// This error can occur only when a Rust panic resumed previously was recovered + /// and returned again. + PreviouslyResumedPanic, /// Serialization error. #[cfg(feature = "serialize")] #[cfg_attr(docsrs, doc(cfg(feature = "serialize")))] @@ -230,6 +235,9 @@ impl fmt::Display for Error { Error::CallbackError { ref traceback, .. } => { write!(fmt, "callback error: {}", traceback) } + Error::PreviouslyResumedPanic => { + write!(fmt, "previously resumed panic returned again") + } #[cfg(feature = "serialize")] Error::SerializeError(ref err) => { write!(fmt, "serialize error: {}", err) diff --git a/src/lua.rs b/src/lua.rs index 9e94e19..e215201 100644 --- a/src/lua.rs +++ b/src/lua.rs @@ -4,6 +4,7 @@ use std::collections::HashMap; use std::ffi::CString; use std::marker::PhantomData; use std::os::raw::{c_char, c_int, c_void}; +use std::panic::resume_unwind; use std::sync::{Arc, Mutex, Weak}; use std::{mem, ptr, str}; @@ -25,7 +26,7 @@ use crate::util::{ assert_stack, callback_error, check_stack, get_gc_userdata, get_main_state, get_userdata, get_wrapped_error, init_error_registry, init_gc_metatable_for, init_userdata_metatable, pop_error, protect_lua, protect_lua_closure, push_gc_userdata, push_meta_gc_userdata, - push_string, push_userdata, push_wrapped_error, StackGuard, + push_string, push_userdata, push_wrapped_error, StackGuard, WrappedPanic, }; use crate::value::{FromLua, FromLuaMulti, MultiValue, Nil, ToLua, ToLuaMulti, Value}; @@ -1402,32 +1403,33 @@ impl Lua { // Uses 2 stack spaces, does not call checkstack pub(crate) unsafe fn pop_value(&self) -> Value { - match ffi::lua_type(self.state, -1) { + let state = self.state; + match ffi::lua_type(state, -1) { ffi::LUA_TNIL => { - ffi::lua_pop(self.state, 1); + ffi::lua_pop(state, 1); Nil } ffi::LUA_TBOOLEAN => { - let b = Value::Boolean(ffi::lua_toboolean(self.state, -1) != 0); - ffi::lua_pop(self.state, 1); + let b = Value::Boolean(ffi::lua_toboolean(state, -1) != 0); + ffi::lua_pop(state, 1); b } ffi::LUA_TLIGHTUSERDATA => { - let ud = Value::LightUserData(LightUserData(ffi::lua_touserdata(self.state, -1))); - ffi::lua_pop(self.state, 1); + let ud = Value::LightUserData(LightUserData(ffi::lua_touserdata(state, -1))); + ffi::lua_pop(state, 1); ud } ffi::LUA_TNUMBER => { - if ffi::lua_isinteger(self.state, -1) != 0 { - let i = Value::Integer(ffi::lua_tointeger(self.state, -1)); - ffi::lua_pop(self.state, 1); + if ffi::lua_isinteger(state, -1) != 0 { + let i = Value::Integer(ffi::lua_tointeger(state, -1)); + ffi::lua_pop(state, 1); i } else { - let n = Value::Number(ffi::lua_tonumber(self.state, -1)); - ffi::lua_pop(self.state, 1); + let n = Value::Number(ffi::lua_tonumber(state, -1)); + ffi::lua_pop(state, 1); n } } @@ -1439,12 +1441,20 @@ impl Lua { ffi::LUA_TFUNCTION => Value::Function(Function(self.pop_ref())), ffi::LUA_TUSERDATA => { - // It should not be possible to interact with userdata types other than custom - // UserData types OR a WrappedError. WrappedPanic should not be here. - if let Some(err) = get_wrapped_error(self.state, -1).as_ref() { + // We must prevent interaction with userdata types other than UserData OR a WrappedError. + // WrappedPanics are automatically resumed. + if let Some(err) = get_wrapped_error(state, -1).as_ref() { let err = err.clone(); - ffi::lua_pop(self.state, 1); + ffi::lua_pop(state, 1); Value::Error(err) + } else if let Some(panic) = get_gc_userdata::(state, -1).as_mut() { + if let Some(panic) = (*panic).0.take() { + ffi::lua_pop(state, 1); + resume_unwind(panic); + } + // Previously resumed panic? + ffi::lua_pop(state, 1); + Nil } else { Value::UserData(AnyUserData(self.pop_ref())) } diff --git a/src/util.rs b/src/util.rs index ae668fc..b2c2b61 100644 --- a/src/util.rs +++ b/src/util.rs @@ -186,7 +186,7 @@ pub unsafe fn pop_error(state: *mut ffi::lua_State, err_code: c_int) -> Error { if let Some(p) = (*panic).0.take() { resume_unwind(p); } else { - mlua_panic!("error during panic handling, panic was resumed twice") + Error::PreviouslyResumedPanic } } else { let err_string = to_string(state, -1).into_owned(); @@ -587,27 +587,22 @@ pub unsafe fn init_error_registry(state: *mut ffi::lua_State) { Ok(err_buf) } else if let Some(panic) = get_gc_userdata::(state, -1).as_ref() { if let Some(ref p) = (*panic).0 { - ffi::lua_pushlightuserdata( - state, - &ERROR_PRINT_BUFFER_KEY as *const u8 as *mut c_void, - ); - ffi::lua_rawget(state, ffi::LUA_REGISTRYINDEX); + let err_buf_key = &ERROR_PRINT_BUFFER_KEY as *const u8 as *const c_void; + ffi::lua_rawgetp(state, ffi::LUA_REGISTRYINDEX, err_buf_key); let err_buf = ffi::lua_touserdata(state, -1) as *mut String; + (*err_buf).clear(); ffi::lua_pop(state, 2); - let error = if let Some(x) = p.downcast_ref::<&str>() { - x.to_string() - } else if let Some(x) = p.downcast_ref::() { - x.to_string() + if let Some(msg) = p.downcast_ref::<&str>() { + let _ = write!(&mut (*err_buf), "{}", msg); + } else if let Some(msg) = p.downcast_ref::() { + let _ = write!(&mut (*err_buf), "{}", msg); } else { - "panic".to_string() + let _ = write!(&mut (*err_buf), ""); }; - - (*err_buf).clear(); - let _ = write!(&mut (*err_buf), "{}", error); Ok(err_buf) } else { - mlua_panic!("error during panic handling, panic was resumed") + Err(Error::PreviouslyResumedPanic) } } else { // I'm not sure whether this is possible to trigger without bugs in mlua? @@ -721,7 +716,7 @@ pub unsafe fn init_error_registry(state: *mut ffi::lua_State) { } struct WrappedError(pub Error); -struct WrappedPanic(pub Option>); +pub(crate) struct WrappedPanic(pub Option>); // Converts the given lua value to a string in a reasonable format without causing a Lua error or // panicking. diff --git a/tests/tests.rs b/tests/tests.rs index 813ea1d..4f07843 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -11,7 +11,7 @@ extern "system" {} use std::iter::FromIterator; -use std::panic::catch_unwind; +use std::panic::{catch_unwind, AssertUnwindSafe}; use std::sync::Arc; use std::{error, f32, f64, fmt}; @@ -384,62 +384,92 @@ fn test_error() -> Result<()> { assert!(understand_recursion.call::<_, ()>(()).is_err()); - match catch_unwind(|| -> Result<()> { - let lua = Lua::new(); - let globals = lua.globals(); + Ok(()) +} +#[test] +fn test_panic() -> Result<()> { + fn make_lua() -> Result { + let lua = Lua::new(); + let rust_panic_function = + lua.create_function(|_, ()| -> Result<()> { panic!("rust panic") })?; + lua.globals() + .set("rust_panic_function", rust_panic_function)?; + Ok(lua) + } + + // Test triggerting Lua error passing Rust panic (must be resumed) + { + let lua = make_lua()?; + + match catch_unwind(AssertUnwindSafe(|| -> Result<()> { + lua.load( + r#" + _, err = pcall(rust_panic_function) + error(err) + "#, + ) + .exec() + })) { + Ok(Ok(_)) => panic!("no panic was detected"), + Ok(Err(e)) => panic!("error during panic test {:?}", e), + Err(p) => assert!(*p.downcast::<&str>().unwrap() == "rust panic"), + }; + + // Trigger same panic again + match lua.load("error(err)").exec() { + Ok(_) => panic!("no error was detected"), + Err(Error::PreviouslyResumedPanic) => {} + Err(e) => panic!("expected PreviouslyResumedPanic, got {:?}", e), + } + } + + // Test returning Rust panic (must be resumed) + { + let lua = make_lua()?; + match catch_unwind(AssertUnwindSafe(|| -> Result<()> { + let _catched_panic = lua + .load( + r#" + -- Set global + _, err = pcall(rust_panic_function) + return err + "#, + ) + .eval::()?; + Ok(()) + })) { + Ok(_) => panic!("no panic was detected"), + Err(_) => {} + }; + + assert!(lua.globals().get::<_, Value>("err")? == Value::Nil); + match lua.load("tostring(err)").exec() { + Ok(_) => panic!("no error was detected"), + Err(Error::CallbackError { ref cause, .. }) => match cause.as_ref() { + Error::PreviouslyResumedPanic => {} + e => panic!("expected PreviouslyResumedPanic, got {:?}", e), + }, + Err(e) => panic!("expected CallbackError, got {:?}", e), + } + } + + // Test representing rust panic as a string + match catch_unwind(|| -> Result<()> { + let lua = make_lua()?; lua.load( r#" - function rust_panic() - local _, err = pcall(function () rust_panic_function() end) - if err ~= nil then - error(err) - end - end + local _, err = pcall(rust_panic_function) + error(tostring(err)) "#, ) - .exec()?; - let rust_panic_function = - lua.create_function(|_, ()| -> Result<()> { panic!("test_panic") })?; - globals.set("rust_panic_function", rust_panic_function)?; - - let rust_panic = globals.get::<_, Function>("rust_panic")?; - - rust_panic.call::<_, ()>(()) - }) { - Ok(Ok(_)) => panic!("no panic was detected"), - Ok(Err(e)) => panic!("error during panic test {:?}", e), - Err(p) => assert!(*p.downcast::<&str>().unwrap() == "test_panic"), - }; - - match catch_unwind(|| -> Result<()> { - let lua = Lua::new(); - let globals = lua.globals(); - - lua.load( - r#" - function rust_panic() - local _, err = pcall(function () rust_panic_function() end) - if err ~= nil then - error(tostring(err)) - end - end - "#, - ) - .exec()?; - let rust_panic_function = - lua.create_function(|_, ()| -> Result<()> { panic!("test_panic") })?; - globals.set("rust_panic_function", rust_panic_function)?; - - let rust_panic = globals.get::<_, Function>("rust_panic")?; - - rust_panic.call::<_, ()>(()) + .exec() }) { Ok(Ok(_)) => panic!("no error was detected"), Ok(Err(Error::RuntimeError(_))) => {} - Ok(Err(e)) => panic!("unexpected error during panic test {:?}", e), + Ok(Err(e)) => panic!("expected RuntimeError, got {:?}", e), Err(_) => panic!("panic was detected"), - }; + } Ok(()) }