Include garbage collector error type, remove unnecessary setmetatable wrapper

5 files changed, 44 insertions, 85 deletions

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<StdError + Send + Sync>),
 }
 
-/// A specialized `Result` type used by rlua's API.
+/// A specialized `Result` type used by `rlua`'s API.
 pub type Result<T> = StdResult<T, Error>;
 
 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]
@@ -559,33 +559,6 @@ mod tests {
     }
 
     #[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, F: FnOnce() -> 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);