diff --git a/src/error.rs b/src/error.rs index 04e909a..a509432 100644 --- a/src/error.rs +++ b/src/error.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use std::error::Error as StdError; use std::result::Result as StdResult; -/// Error type returned by rlua methods. +/// Error type returned by `rlua` methods. #[derive(Debug, Clone)] pub enum Error { /// Syntax error while parsing Lua source code. @@ -22,6 +22,10 @@ pub enum Error { /// Among other things, this includes invoking operators on wrong types (such as calling or /// indexing a `nil` value). RuntimeError(String), + /// Lua garbage collector error, aka `LUA_ERRGCMM`. + /// + /// The Lua VM returns this error when there is an error running a `__gc` metamethod. + GarbageCollectorError(String), /// A Rust value could not be converted to a Lua value. ToLuaConversionError { /// Name of the Rust type that could not be converted. @@ -95,7 +99,7 @@ pub enum Error { ExternalError(Arc), } -/// A specialized `Result` type used by rlua's API. +/// A specialized `Result` type used by `rlua`'s API. pub type Result = StdResult; impl fmt::Display for Error { @@ -103,6 +107,7 @@ impl fmt::Display for Error { match *self { Error::SyntaxError { ref message, .. } => write!(fmt, "syntax error: {}", message), Error::RuntimeError(ref msg) => write!(fmt, "runtime error: {}", msg), + Error::GarbageCollectorError(ref msg) => write!(fmt, "garbage collector error: {}", msg), Error::ToLuaConversionError { from, to, @@ -142,6 +147,7 @@ impl StdError for Error { match *self { Error::SyntaxError { .. } => "syntax error", Error::RuntimeError(_) => "runtime error", + Error::GarbageCollectorError(_) => "garbage collector error", 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 5e5c048..471f717 100644 --- a/src/lua.rs +++ b/src/lua.rs @@ -1110,8 +1110,11 @@ impl Lua { } else { let p = libc::realloc(ptr as *mut libc::c_void, nsize); if p.is_null() { - // We must abort on OOM, because otherwise this will result in an unsafe - // longjmp. + // We require that OOM results in an abort, and that the lua allocator function + // never errors. Since this is what rust itself normally does on OOM, this is + // 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. eprintln!("Out of memory in Lua allocation, aborting!"); process::abort() } else { @@ -1193,10 +1196,6 @@ impl Lua { ffi::lua_pushcfunction(state, safe_xpcall); ffi::lua_rawset(state, -3); - push_string(state, "setmetatable").unwrap(); - ffi::lua_pushcfunction(state, safe_setmetatable); - ffi::lua_rawset(state, -3); - ffi::lua_pop(state, 1); }); diff --git a/src/table.rs b/src/table.rs index 7347b7f..fa2d506 100644 --- a/src/table.rs +++ b/src/table.rs @@ -435,7 +435,7 @@ where #[cfg(test)] mod tests { use super::Table; - use error::Result; + use error::{Result}; use lua::{Lua, Nil, Value}; #[test] @@ -558,33 +558,6 @@ mod tests { assert_eq!(tin.get::<_, i64>(3).unwrap(), 3); } - #[test] - fn test_setmetatable_gc() { - let lua = Lua::new(); - lua.exec::<()>( - r#" - val = nil - table = {} - setmetatable(table, { - __gc = function() - val = "gcwascalled" - end - }) - table_badgc = {} - setmetatable(table_badgc, { - __gc = "what's a gc" - }) - table = nil - table_badgc = nil - collectgarbage("collect") - "#, - None, - ).unwrap(); - - let globals = lua.globals(); - assert_eq!(globals.get::<_, String>("val").unwrap(), "gcwascalled"); - } - #[test] fn test_metatable() { let lua = Lua::new(); diff --git a/src/tests.rs b/src/tests.rs index 56b8040..a1c82df 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -646,6 +646,28 @@ fn test_set_metatable_nil() { ).unwrap(); } +#[test] +fn test_gc_error() { + let lua = Lua::new(); + match lua.exec::<()>( + r#" + val = nil + table = {} + setmetatable(table, { + __gc = function() + error("gcwascalled") + end + }) + table = nil + collectgarbage("collect") + "#, + None, + ) { + Err(Error::GarbageCollectorError(_)) => {} + _ => panic!("__gc error did not result in correct type"), + } +} + // TODO: Need to use compiletest-rs or similar to make sure these don't compile. /* #[test] diff --git a/src/util.rs b/src/util.rs index 2f4a363..3507cc4 100644 --- a/src/util.rs +++ b/src/util.rs @@ -201,15 +201,11 @@ 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. - eprintln!("Lua memory error, aborting!"); + eprintln!("impossible Lua allocation error, aborting!"); process::abort() } ffi::LUA_ERRGCMM => { - // This should be impossible, since we wrap setmetatable to protect __gc - // metamethods, but if we do end up here then the same logic as setmetatable - // applies and we must abort. - eprintln!("Lua error during __gc, aborting!"); - process::abort() + Error::GarbageCollectorError(err_string) } _ => lua_panic!(state, "internal error: unrecognized lua error code"), } @@ -354,43 +350,6 @@ pub unsafe extern "C" fn safe_xpcall(state: *mut ffi::lua_State) -> c_int { } } -// Safely call setmetatable, if a __gc function is given, will wrap it in pcall, and panic on error. -pub unsafe extern "C" fn safe_setmetatable(state: *mut ffi::lua_State) -> c_int { - if ffi::lua_gettop(state) < 2 { - ffi::lua_pushstring(state, cstr!("not enough arguments to setmetatable")); - ffi::lua_error(state); - } - - // Wrapping the __gc method in setmetatable ONLY works because Lua 5.3 only honors the __gc - // method when it exists upon calling setmetatable, and ignores it if it is set later. - ffi::lua_pushstring(state, cstr!("__gc")); - if ffi::lua_istable(state, -2) == 1 && ffi::lua_rawget(state, -2) == ffi::LUA_TFUNCTION { - unsafe extern "C" fn safe_gc(state: *mut ffi::lua_State) -> c_int { - ffi::lua_pushvalue(state, ffi::lua_upvalueindex(1)); - ffi::lua_insert(state, 1); - if ffi::lua_pcall(state, 1, 0, 0) != ffi::LUA_OK { - // If a user supplied __gc metamethod causes an error, we must always abort. We may - // be inside a protected context due to being in a callback, but inside an - // unprotected ffi call that can cause memory errors, so may be at risk of - // longjmping over arbitrary rust. - eprintln!("Lua error during __gc, aborting!"); - process::abort() - } else { - ffi::lua_gettop(state) - } - } - - ffi::lua_pushcclosure(state, safe_gc, 1); - ffi::lua_pushstring(state, cstr!("__gc")); - ffi::lua_insert(state, -2); - ffi::lua_rawset(state, -3); - } else { - ffi::lua_pop(state, 1); - } - ffi::lua_setmetatable(state, -2); - 1 -} - // Does not call checkstack, uses 1 stack space pub unsafe fn main_state(state: *mut ffi::lua_State) -> *mut ffi::lua_State { ffi::lua_rawgeti(state, ffi::LUA_REGISTRYINDEX, ffi::LUA_RIDX_MAINTHREAD); @@ -593,12 +552,12 @@ unsafe fn get_panic_metatable(state: *mut ffi::lua_State) -> c_int { ffi::LUA_TTABLE } -// Runs the given function with the Lua garbage collector disabled. `rlua` assumes that all memory -// errors are aborts, so in this way, 'm' functions that may also cause a `__gc` metamethod error -// are guaranteed not to cause a Lua error (longjmp). The given function should never panic or -// longjmp, because this could inadverntently disable the gc. This is useful when error handling -// must allocate, and `__gc` errors at that time would shadow more important errors, or be extremely -// difficult to handle safely. +// Runs the given function with the Lua garbage collector disabled. `rlua` assumes that all +// allocation failures are aborts, so when the garbage collector is disabled, 'm' functions that can +// cause either an allocation error or a a `__gc` metamethod error are prevented from causing errors +// at all. The given function should never panic or longjmp, because this could inadverntently +// disable the gc. This is useful when error handling must allocate, and `__gc` errors at that time +// would shadow more important errors, or be extremely difficult to handle safely. unsafe fn gc_guard R>(state: *mut ffi::lua_State, f: F) -> R { if ffi::lua_gc(state, ffi::LUA_GCISRUNNING, 0) != 0 { ffi::lua_gc(state, ffi::LUA_GCSTOP, 0);