Simplify handling of userdata __gc and resurrected userdata.

kyren · kyren · commit 77eb73a50c2f · 2018-01-27T18:27:01.000-05:00
Now, simply remove the userdata table immediately before dropping the userdata.
This does two things, it prevents __gc from double dropping the userdata, and
after the first call to __gc, it prevents the userdata from being identified as
any particular userdata type, so it cannot be misused after being finalized.

This change thus removes the userdata invalidation error, and simplifies a lot
of userdata handling code.

It also fixes a panic bug.  Because there is no predictable order for
finalizers, it is possible to run a userdata finalizer that does not resurrect
itself before a lua table finalizer that accesses that userdata, and this means
that there were several asserts that were possible to trigger in normal Lua code
in util.rs related to `WrappedError`.

Now, finalized userdata is simply a userdata with no methods, so any use of
finalized userdata becomes a normal script runtime error (though, with a
potentially confusing error message).  As a future improvement, we could set
a metatable on finalized userdata that provides a better error message.
diff --git a/src/error.rs b/src/error.rs
@@ -31,13 +31,6 @@ pub enum Error {
     /// This is an error because `rlua` callbacks are FnMut and thus 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.
@@ -123,10 +116,6 @@ 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,
diff --git a/src/lua.rs b/src/lua.rs
@@ -957,7 +957,7 @@ impl Lua {
                     ephemeral: true,
                 };
 
-                let func = get_userdata::<RefCell<Callback>>(state, ffi::lua_upvalueindex(1))?;
+                let func = get_userdata::<RefCell<Callback>>(state, ffi::lua_upvalueindex(1));
                 let mut func = (*func)
                     .try_borrow_mut()
                     .map_err(|_| Error::RecursiveCallbackError)?;
diff --git a/src/userdata.rs b/src/userdata.rs
@@ -386,7 +386,7 @@ impl<'lua> AnyUserData<'lua> {
                     ffi::lua_pop(lua.state, 3);
                     Err(Error::UserDataTypeMismatch)
                 } else {
-                    let res = func(&*get_userdata::<RefCell<T>>(lua.state, -3)?);
+                    let res = func(&*get_userdata::<RefCell<T>>(lua.state, -3));
                     ffi::lua_pop(lua.state, 3);
                     res
                 }
@@ -434,7 +434,7 @@ impl<'lua> AnyUserData<'lua> {
 #[cfg(test)]
 mod tests {
     use super::{MetaMethod, UserData, UserDataMethods};
-    use error::{Error, ExternalError};
+    use error::ExternalError;
     use string::String;
     use function::Function;
     use lua::Lua;
@@ -568,7 +568,7 @@ mod tests {
             globals.set("userdata", MyUserdata { id: 123 }).unwrap();
         }
 
-        match lua.eval::<()>(
+        assert!(lua.eval::<()>(
             r#"
                 local tbl = setmetatable({
                     userdata = userdata
@@ -582,14 +582,8 @@ mod tests {
                 collectgarbage("collect")
                 hatch:access()
             "#,
-            None,
-        ) {
-            Err(Error::CallbackError { cause, .. }) => match *cause {
-                Error::ExpiredUserData { .. } => {}
-                ref other => panic!("incorrect result: {}", other),
-            },
-            other => panic!("incorrect result: {:?}", other),
-        }
+            None
+        ).is_err());
     }
 
     #[test]
diff --git a/src/util.rs b/src/util.rs
@@ -104,21 +104,25 @@ pub unsafe fn protect_lua_call<F, R>(
     f: F,
 ) -> Result<R>
 where
-    F: FnMut(*mut ffi::lua_State) -> R,
+    F: FnOnce(*mut ffi::lua_State) -> R,
 {
     struct Params<F, R> {
         function: F,
-        ret: Option<R>,
+        result: R,
         nresults: c_int,
     }
 
     unsafe extern "C" fn do_call<F, R>(state: *mut ffi::lua_State) -> c_int
     where
-        F: FnMut(*mut ffi::lua_State) -> R,
+        F: FnOnce(*mut ffi::lua_State) -> R,
     {
         let params = ffi::lua_touserdata(state, -1) as *mut Params<F, R>;
         ffi::lua_pop(state, 1);
-        (*params).ret = Some(((*params).function)(state));
+
+        let function = mem::replace(&mut (*params).function, mem::uninitialized());
+        mem::replace(&mut (*params).result, function(state));
+        // params now has function uninitialied and result initialized
+
         if (*params).nresults == ffi::LUA_MULTRET {
             ffi::lua_gettop(state)
         } else {
@@ -132,20 +136,32 @@ where
     ffi::lua_pushcfunction(state, do_call::<F, R>);
     ffi::lua_rotate(state, stack_start + 1, 2);
 
+    // We are about to do some really scary stuff with the Params structure, both because
+    // protect_lua_call is very hot, and becuase we would like to allow the function type to be
+    // FnOnce rather than FnMut.  We are using Params here both to pass data to the callback and
+    // return data from the callback.
+    //
+    // params starts out with function initialized and result uninitialized, nresults is Copy so we
+    // don't care about it.
     let mut params = Params {
         function: f,
-        ret: None,
+        result: mem::uninitialized(),
         nresults,
     };
 
     ffi::lua_pushlightuserdata(state, &mut params as *mut Params<F, R> as *mut c_void);
 
     let ret = ffi::lua_pcall(state, nargs + 1, nresults, stack_start + 1);
+    let result = mem::replace(&mut params.result, mem::uninitialized());
+
+    // params now has both function and result uninitialized, so we need to forget it so Drop isn't
+    // run.
+    mem::forget(params);
 
     ffi::lua_remove(state, stack_start + 1);
 
     if ret == ffi::LUA_OK {
-        Ok(params.ret.unwrap())
+        Ok(result)
     } else {
         Err(pop_error(state, ret))
     }
@@ -164,8 +180,7 @@ 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::<WrappedPanic>(state, -1)
-            .expect("WrappedPanic was somehow resurrected after garbage collection");
+        let panic = get_userdata::<WrappedPanic>(state, -1);
         if let Some(p) = (*panic).0.take() {
             ffi::lua_settop(state, 0);
             resume_unwind(p);
@@ -220,26 +235,30 @@ pub unsafe fn push_string(state: *mut ffi::lua_State, s: &str) -> Result<()> {
 
 // Internally uses 4 stack spaces, does not call checkstack
 pub unsafe fn push_userdata<T>(state: *mut ffi::lua_State, t: T) -> Result<()> {
-    let mut t = Some(t);
-    protect_lua_call(state, 0, 1, |state| {
-        let ud = ffi::lua_newuserdata(state, mem::size_of::<Option<T>>()) as *mut Option<T>;
-        ptr::write(ud, t.take());
+    protect_lua_call(state, 0, 1, move |state| {
+        let ud = ffi::lua_newuserdata(state, mem::size_of::<T>()) as *mut T;
+        ptr::write(ud, t);
     })
 }
 
 // Returns None in the case that the userdata has already been garbage collected.
-pub unsafe fn get_userdata<T>(state: *mut ffi::lua_State, index: c_int) -> Result<*mut T> {
-    let ud = ffi::lua_touserdata(state, index) as *mut Option<T>;
+pub unsafe fn get_userdata<T>(state: *mut ffi::lua_State, index: c_int) -> *mut T {
+    let ud = ffi::lua_touserdata(state, index) as *mut T;
     lua_assert!(state, !ud.is_null(), "userdata pointer is null");
-    (*ud)
-        .as_mut()
-        .map(|v| v as *mut T)
-        .ok_or(Error::ExpiredUserData)
+    ud
 }
 
 pub unsafe extern "C" fn userdata_destructor<T>(state: *mut ffi::lua_State) -> c_int {
     callback_error(state, || {
-        *(ffi::lua_touserdata(state, 1) as *mut Option<T>) = None;
+        // We clear the metatable of userdata on __gc so that it will not be double dropped, and
+        // also so that it cannot be used or identified as any particular userdata type after the
+        // first call to __gc.
+        gc_guard(state, || {
+            ffi::lua_newtable(state);
+            ffi::lua_setmetatable(state, -2);
+        });
+        let ud = &mut *(ffi::lua_touserdata(state, 1) as *mut T);
+        mem::replace(ud, mem::uninitialized());
         Ok(0)
     })
 }
@@ -367,9 +386,8 @@ pub unsafe fn push_wrapped_error(state: *mut ffi::lua_State, err: Error) {
     ffi::luaL_checkstack(state, 2, ptr::null());
 
     gc_guard(state, || {
-        let ud = ffi::lua_newuserdata(state, mem::size_of::<Option<WrappedError>>())
-            as *mut Option<WrappedError>;
-        ptr::write(ud, Some(WrappedError(err)))
+        let ud = ffi::lua_newuserdata(state, mem::size_of::<WrappedError>()) as *mut WrappedError;
+        ptr::write(ud, WrappedError(err))
     });
 
     get_error_metatable(state);
@@ -382,8 +400,7 @@ pub unsafe fn pop_wrapped_error(state: *mut ffi::lua_State) -> Option<Error> {
     if ffi::lua_gettop(state) == 0 || !is_wrapped_error(state, -1) {
         None
     } else {
-        let err = &*get_userdata::<WrappedError>(state, -1)
-            .expect("WrappedError was somehow resurrected after garbage collection");
+        let err = &*get_userdata::<WrappedError>(state, -1);
         let err = err.0.clone();
         ffi::lua_pop(state, 1);
         Some(err)
@@ -415,9 +432,8 @@ unsafe fn push_wrapped_panic(state: *mut ffi::lua_State, panic: Box<Any + Send>)
     ffi::luaL_checkstack(state, 2, ptr::null());
 
     gc_guard(state, || {
-        let ud = ffi::lua_newuserdata(state, mem::size_of::<Option<WrappedPanic>>())
-            as *mut Option<WrappedPanic>;
-        ptr::write(ud, Some(WrappedPanic(Some(panic))))
+        let ud = ffi::lua_newuserdata(state, mem::size_of::<WrappedPanic>()) as *mut WrappedPanic;
+        ptr::write(ud, WrappedPanic(Some(panic)))
     });
 
     get_panic_metatable(state);
@@ -480,8 +496,7 @@ 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::<WrappedError>(state, -1)
-                    .expect("WrappedError was somehow resurrected after garbage collection");
+                let error = get_userdata::<WrappedError>(state, -1);
                 let error_str = (*error).0.to_string();
                 gc_guard(state, || {
                     ffi::lua_pushlstring(
