From 66a4e9a8e73b9526eecb74d6e4dcbc06d6a53c60 Mon Sep 17 00:00:00 2001 From: kyren Date: Mon, 4 Dec 2017 02:46:57 -0500 Subject: [PATCH] Add `ExpiredUserData` error and avoid what was previously a panic Also make sure that panic messages clearly state that they are internal errors, so people report them as a bug. Since the only panics left are all internal errors, just move the internal error message into the panic / assert macros. --- src/error.rs | 11 +++++++++++ src/lua.rs | 4 ++-- src/macros.rs | 8 ++++---- src/tests.rs | 21 ++++++++++++++------- src/thread.rs | 10 ++++++++-- src/userdata.rs | 17 +++++++++++------ src/util.rs | 36 +++++++++++++++++++++--------------- 7 files changed, 71 insertions(+), 36 deletions(-) diff --git a/src/error.rs b/src/error.rs index b3e4586..d3f2c74 100644 --- a/src/error.rs +++ b/src/error.rs @@ -31,6 +31,12 @@ pub enum Error { /// This is an error, since `rlua` callbacks are FnMut an the functions can only be mutably /// borrowed once. RecursiveCallbackError, + /// Lua code has accessed a [`UserData`] value that was already garbage collected + /// + /// This can happen when a [`UserData`] has a custom `__gc` metamethod, this method resurrects + /// the [`UserData`], and then the [`UserData`] is subsequently accessed. + /// [`UserData`]: trait.UserData.html + ExpiredUserData, /// A Rust value could not be converted to a Lua value. ToLuaConversionError { /// Name of the Rust type that could not be converted. @@ -116,6 +122,10 @@ impl fmt::Display for Error { write!(fmt, "garbage collector error: {}", msg) } Error::RecursiveCallbackError => write!(fmt, "callback called recursively"), + Error::ExpiredUserData => write!( + fmt, + "access of userdata which has already been garbage collected" + ), Error::ToLuaConversionError { from, to, @@ -157,6 +167,7 @@ impl StdError for Error { Error::RuntimeError(_) => "runtime error", Error::GarbageCollectorError(_) => "garbage collector error", Error::RecursiveCallbackError => "callback called recursively", + Error::ExpiredUserData => "access of userdata which has already been garbage collected", Error::ToLuaConversionError { .. } => "conversion error to lua", Error::FromLuaConversionError { .. } => "conversion error from lua", Error::CoroutineInactive => "attempt to resume inactive coroutine", diff --git a/src/lua.rs b/src/lua.rs index df705f0..1a9d8ad 100644 --- a/src/lua.rs +++ b/src/lua.rs @@ -567,7 +567,7 @@ impl Lua { &LUA_USERDATA_REGISTRY_KEY as *const u8 as *mut c_void, ); ffi::lua_gettable(self.state, ffi::LUA_REGISTRYINDEX); - let registered_userdata = get_userdata::>(self.state, -1); + let registered_userdata = get_userdata::>(self.state, -1)?; ffi::lua_pop(self.state, 1); if let Some(table_id) = (*registered_userdata).get(&TypeId::of::()) { @@ -798,7 +798,7 @@ impl Lua { ephemeral: true, }; - let func = get_userdata::>(state, ffi::lua_upvalueindex(1)); + let func = get_userdata::>(state, ffi::lua_upvalueindex(1))?; let mut func = (*func) .try_borrow_mut() .map_err(|_| Error::RecursiveCallbackError)?; diff --git a/src/macros.rs b/src/macros.rs index 517db21..1408384 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -16,14 +16,14 @@ macro_rules! lua_panic { ($state:expr, $msg:expr) => { { $crate::ffi::lua_settop($state, 0); - panic!(concat!("rlua: ", $msg)); + panic!(concat!("rlua internal error: ", $msg)); } }; ($state:expr, $fmt:expr, $($arg:tt)+) => { { $crate::ffi::lua_settop($state, 0); - panic!(concat!("rlua: ", $fmt), $($arg)+); + panic!(concat!("rlua internal error: ", $fmt), $($arg)+); } }; } @@ -40,14 +40,14 @@ macro_rules! lua_assert { ($state:expr, $cond:expr, $msg:expr) => { if !$cond { $crate::ffi::lua_settop($state, 0); - panic!(concat!("rlua: ", $msg)); + panic!(concat!("rlua internal error: ", $msg)); } }; ($state:expr, $cond:expr, $fmt:expr, $($arg:tt)+) => { if !$cond { $crate::ffi::lua_settop($state, 0); - panic!(concat!("rlua: ", $fmt), $($arg)+); + panic!(concat!("rlua internal error: ", $fmt), $($arg)+); } }; } diff --git a/src/tests.rs b/src/tests.rs index ae1f344..418c3d1 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -448,13 +448,20 @@ fn test_recursive_callback_error() { Ok(()) }).unwrap(); lua.globals().set("f", f).unwrap(); - assert!( - lua.globals() - .get::<_, Function>("f") - .unwrap() - .call::<_, ()>(false) - .is_err() - ) + match lua.globals() + .get::<_, Function>("f") + .unwrap() + .call::<_, ()>(false) + { + Err(Error::CallbackError { ref cause, .. }) => match *cause.as_ref() { + Error::CallbackError { ref cause, .. } => match *cause.as_ref() { + Error::RecursiveCallbackError { .. } => {} + ref other => panic!("incorrect result: {:?}", other), + }, + ref other => panic!("incorrect result: {:?}", other), + }, + other => panic!("incorrect result: {:?}", other), + }; } #[test] diff --git a/src/thread.rs b/src/thread.rs index 4f31019..83eb994 100644 --- a/src/thread.rs +++ b/src/thread.rs @@ -142,6 +142,8 @@ impl<'lua> Thread<'lua> { #[cfg(test)] mod tests { + use std::panic::catch_unwind; + use super::{Thread, ThreadStatus}; use error::Error; use function::Function; @@ -243,8 +245,8 @@ mod tests { } #[test] - #[should_panic] fn coroutine_panic() { + // check that coroutines propagate panics correctly let lua = Lua::new(); let thrd_main = lua.create_function(|lua, ()| { // whoops, 'main' has a wrong type @@ -253,6 +255,10 @@ mod tests { }).unwrap(); lua.globals().set("main", thrd_main.clone()).unwrap(); let thrd: Thread = lua.create_thread(thrd_main).unwrap(); - thrd.resume::<_, ()>(()).unwrap(); + + match catch_unwind(|| thrd.resume::<_, ()>(())) { + Ok(r) => panic!("coroutine panic not propagated, instead returned {:?}", r), + Err(_) => {} + } } } diff --git a/src/userdata.rs b/src/userdata.rs index f3bbf12..08ea583 100644 --- a/src/userdata.rs +++ b/src/userdata.rs @@ -358,7 +358,7 @@ impl<'lua> AnyUserData<'lua> { { unsafe { let lua = self.0.lua; - stack_guard(lua.state, 0, move || { + stack_err_guard(lua.state, 0, move || { check_stack(lua.state, 3); lua.push_ref(lua.state, &self.0); @@ -379,7 +379,7 @@ impl<'lua> AnyUserData<'lua> { ffi::lua_pop(lua.state, 3); Err(Error::UserDataTypeMismatch) } else { - let res = func(&*get_userdata::>(lua.state, -3)); + let res = func(&*get_userdata::>(lua.state, -3)?); ffi::lua_pop(lua.state, 3); res } @@ -391,7 +391,7 @@ impl<'lua> AnyUserData<'lua> { #[cfg(test)] mod tests { use super::{MetaMethod, UserData, UserDataMethods}; - use error::ExternalError; + use error::{Error, ExternalError}; use string::String; use function::Function; use lua::Lua; @@ -505,7 +505,6 @@ mod tests { } #[test] - #[should_panic] fn test_expired_userdata() { struct MyUserdata { id: u8, @@ -526,7 +525,7 @@ mod tests { globals.set("userdata", MyUserdata { id: 123 }).unwrap(); } - lua.eval::<()>( + match lua.eval::<()>( r#" local tbl = setmetatable({ userdata = userdata @@ -541,7 +540,13 @@ mod tests { hatch:access() "#, None, - ).unwrap(); + ) { + Err(Error::CallbackError { cause, .. }) => match *cause { + Error::ExpiredUserData { .. } => {} + ref other => panic!("incorrect result: {}", other), + }, + other => panic!("incorrect result: {:?}", other), + } } #[test] diff --git a/src/util.rs b/src/util.rs index 852155b..036d709 100644 --- a/src/util.rs +++ b/src/util.rs @@ -28,7 +28,7 @@ where lua_assert!( state, expected >= 0, - "internal stack error: too many values would be popped" + "too many stack values would be popped" ); let res = op(); @@ -37,7 +37,7 @@ where lua_assert!( state, ffi::lua_gettop(state) == expected, - "internal stack error: expected stack to be {}, got {}", + "expected stack to be {}, got {}", expected, top ); @@ -62,7 +62,7 @@ where lua_assert!( state, expected >= 0, - "internal stack error: too many values would be popped" + "too many stack values would be popped" ); let res = op(); @@ -72,7 +72,7 @@ where lua_assert!( state, ffi::lua_gettop(state) == expected, - "internal stack error: expected stack to be {}, got {}", + "expected stack to be {}, got {}", expected, top ); @@ -80,7 +80,7 @@ where lua_assert!( state, top >= expected, - "internal stack error: {} too many values popped", + "{} too many stack values popped", top - expected ); if top > expected { @@ -164,12 +164,13 @@ pub unsafe fn pop_error(state: *mut ffi::lua_State, err_code: c_int) -> Error { if let Some(err) = pop_wrapped_error(state) { err } else if is_wrapped_panic(state, -1) { - let panic = get_userdata::(state, -1); + let panic = get_userdata::(state, -1) + .expect("WrappedPanic was somehow resurrected after garbage collection"); if let Some(p) = (*panic).0.take() { ffi::lua_settop(state, 0); resume_unwind(p); } else { - lua_panic!(state, "internal error: panic was resumed twice") + lua_panic!(state, "panic was resumed twice") } } else { let err_string = gc_guard(state, || { @@ -205,7 +206,7 @@ pub unsafe fn pop_error(state: *mut ffi::lua_State, err_code: c_int) -> Error { process::abort() } ffi::LUA_ERRGCMM => Error::GarbageCollectorError(err_string), - _ => lua_panic!(state, "internal error: unrecognized lua error code"), + _ => lua_panic!(state, "unrecognized lua error code"), } } } @@ -224,11 +225,14 @@ 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 { +// Returns None in the case that the userdata has already been garbage collected. +pub unsafe fn get_userdata(state: *mut ffi::lua_State, index: c_int) -> Result<*mut T> { let ud = ffi::lua_touserdata(state, index) as *mut Option; - lua_assert!(state, !ud.is_null()); - lua_assert!(state, (*ud).is_some(), "access of expired userdata"); - (*ud).as_mut().unwrap() + lua_assert!(state, !ud.is_null(), "userdata pointer is null"); + (*ud) + .as_mut() + .map(|v| v as *mut T) + .ok_or(Error::ExpiredUserData) } pub unsafe extern "C" fn userdata_destructor(state: *mut ffi::lua_State) -> c_int { @@ -377,7 +381,8 @@ pub unsafe fn pop_wrapped_error(state: *mut ffi::lua_State) -> Option { if ffi::lua_gettop(state) == 0 || !is_wrapped_error(state, -1) { None } else { - let err = &*get_userdata::(state, -1); + let err = &*get_userdata::(state, -1) + .expect("WrappedError was somehow resurrected after garbage collection"); let err = err.0.clone(); ffi::lua_pop(state, 1); Some(err) @@ -457,7 +462,8 @@ unsafe fn get_error_metatable(state: *mut ffi::lua_State) -> c_int { unsafe extern "C" fn error_tostring(state: *mut ffi::lua_State) -> c_int { callback_error(state, || { if is_wrapped_error(state, -1) { - let error = get_userdata::(state, -1); + let error = get_userdata::(state, -1) + .expect("WrappedError was somehow resurrected after garbage collection"); let error_str = (*error).0.to_string(); gc_guard(state, || { ffi::lua_pushlstring( @@ -470,7 +476,7 @@ unsafe fn get_error_metatable(state: *mut ffi::lua_State) -> c_int { Ok(1) } else { - panic!("internal error: userdata mismatch in Error metamethod"); + panic!("userdata mismatch in Error metamethod"); } }) }