From cb25a99f70b00b2cf2b1fc0ec0000d6fd89cfe9a Mon Sep 17 00:00:00 2001 From: kyren Date: Tue, 6 Feb 2018 20:23:16 -0500 Subject: [PATCH 1/5] Lots of changes, not sure if actually safe yet. * Make Lua Send * Add Send bounds to (nearly) all instances where userdata and functions are passed to Lua * Add a "scope" method which takes a callback that accepts a `Scope`, and give `Scope` the ability to create functions and userdata that are !Send, *and also functions that are not even 'static!*. --- src/conversion.rs | 2 +- src/ffi.rs | 3 + src/lua.rs | 183 +++++++++++++++++++++++++++++++++++++--------- src/table.rs | 1 + src/tests.rs | 86 +++++++++++++++++++++- src/types.rs | 28 +++++-- src/userdata.rs | 28 +++---- src/util.rs | 48 +++++++----- 8 files changed, 300 insertions(+), 79 deletions(-) diff --git a/src/conversion.rs b/src/conversion.rs index 39baaa2..5d09f5d 100644 --- a/src/conversion.rs +++ b/src/conversion.rs @@ -112,7 +112,7 @@ impl<'lua> FromLua<'lua> for AnyUserData<'lua> { } } -impl<'lua, T: UserData> ToLua<'lua> for T { +impl<'lua, T: Send + UserData> ToLua<'lua> for T { fn to_lua(self, lua: &'lua Lua) -> Result> { Ok(Value::UserData(lua.create_userdata(self)?)) } diff --git a/src/ffi.rs b/src/ffi.rs index 9130afd..dfb2878 100644 --- a/src/ffi.rs +++ b/src/ffi.rs @@ -124,6 +124,9 @@ extern "C" { pub fn lua_setuservalue(state: *mut lua_State, index: c_int); pub fn lua_getuservalue(state: *mut lua_State, index: c_int) -> c_int; + pub fn lua_getupvalue(state: *mut lua_State, funcindex: c_int, n: c_int) -> *const c_char; + pub fn lua_setupvalue(state: *mut lua_State, funcindex: c_int, n: c_int) -> *const c_char; + pub fn lua_settable(state: *mut lua_State, index: c_int); pub fn lua_rawset(state: *mut lua_State, index: c_int); pub fn lua_setmetatable(state: *mut lua_State, index: c_int); diff --git a/src/lua.rs b/src/lua.rs index d8dd809..948d142 100644 --- a/src/lua.rs +++ b/src/lua.rs @@ -1,4 +1,4 @@ -use std::{ptr, str}; +use std::{mem, process, ptr, str}; use std::sync::{Arc, Mutex}; use std::ops::DerefMut; use std::cell::RefCell; @@ -7,8 +7,7 @@ use std::any::TypeId; use std::marker::PhantomData; use std::collections::HashMap; use std::os::raw::{c_char, c_int, c_void}; -use std::mem; -use std::process; +use std::panic::{catch_unwind, AssertUnwindSafe}; use libc; @@ -30,12 +29,21 @@ pub struct Lua { ephemeral: bool, } +/// Constructed by the `Lua::scope` method, allows temporarily passing to Lua userdata that is +/// !Send, and callbacks that are !Send and not 'static. +pub struct Scope<'lua> { + lua: &'lua Lua, + destructors: RefCell>>, +} + // Data associated with the main lua_State via lua_getextraspace. struct ExtraData { registered_userdata: HashMap, - registry_drop_list: Arc>>, + registry_unref_list: Arc>>, } +unsafe impl Send for Lua {} + impl Drop for Lua { fn drop(&mut self) { unsafe { @@ -259,7 +267,7 @@ impl Lua { where A: FromLuaMulti<'lua>, R: ToLuaMulti<'lua>, - F: 'static + FnMut(&'lua Lua, A) -> Result, + F: 'static + Send + FnMut(&'lua Lua, A) -> Result, { self.create_callback_function(Box::new(move |lua, args| { func(lua, A::from_lua_multi(args, lua)?)?.to_lua_multi(lua) @@ -286,25 +294,9 @@ impl Lua { /// Create a Lua userdata object from a custom userdata type. pub fn create_userdata(&self, data: T) -> Result where - T: UserData, + T: Send + UserData, { - unsafe { - stack_err_guard(self.state, 0, move || { - check_stack(self.state, 3); - - push_userdata::>(self.state, RefCell::new(data))?; - - ffi::lua_rawgeti( - self.state, - ffi::LUA_REGISTRYINDEX, - self.userdata_metatable::()? as ffi::lua_Integer, - ); - - ffi::lua_setmetatable(self.state, -2); - - Ok(AnyUserData(self.pop_ref(self.state))) - }) - } + self.do_create_userdata(data) } /// Returns a handle to the global environment. @@ -318,6 +310,28 @@ impl Lua { } } + /// Calls the given function with a `Scope` parameter, giving the function the ability to create + /// userdata from rust types that are !Send, and rust callbacks that are !Send and not 'static. + /// The lifetime of any function or userdata created through `Scope` lasts only until the + /// completion of this method call, on completion all such created values are automatically + /// dropped and Lua references to them are invalidated. If a script accesses a value created + /// through `Scope` outside of this method, a Lua error will result. Since we can ensure the + /// lifetime of values created through `Scope`, and we know that `Lua` cannot be sent to another + /// thread while `Scope` is live, it is safe to allow !Send datatypes and functions whose + /// lifetimes only outlive the scope lifetime. + pub fn scope<'lua, F, R>(&'lua self, f: F) -> R + where + F: FnOnce(&mut Scope<'lua>) -> R, + { + let mut scope = Scope { + lua: self, + destructors: RefCell::new(Vec::new()), + }; + let r = f(&mut scope); + drop(scope); + r + } + /// Coerces a Lua value to a string. /// /// The value must be a string (in which case this is a no-op) or a number. @@ -492,7 +506,8 @@ impl Lua { Ok(RegistryKey { registry_id, - drop_list: (*self.extra()).registry_drop_list.clone(), + unref_list: (*self.extra()).registry_unref_list.clone(), + drop_unref: true, }) }) } @@ -504,7 +519,7 @@ impl Lua { /// value previously placed by `create_registry_value`. pub fn registry_value<'lua, T: FromLua<'lua>>(&'lua self, key: &RegistryKey) -> Result { unsafe { - if !Arc::ptr_eq(&key.drop_list, &(*self.extra()).registry_drop_list) { + if !Arc::ptr_eq(&key.unref_list, &(*self.extra()).registry_unref_list) { return Err(Error::MismatchedRegistryKey); } @@ -528,13 +543,12 @@ impl Lua { /// `RegistryKey`s have been dropped. pub fn remove_registry_value(&self, mut key: RegistryKey) -> Result<()> { unsafe { - if !Arc::ptr_eq(&key.drop_list, &(*self.extra()).registry_drop_list) { + if !Arc::ptr_eq(&key.unref_list, &(*self.extra()).registry_unref_list) { return Err(Error::MismatchedRegistryKey); } ffi::luaL_unref(self.state, ffi::LUA_REGISTRYINDEX, key.registry_id); - // Don't adding to the registry drop list when dropping the key - key.registry_id = ffi::LUA_REFNIL; + key.drop_unref = false; Ok(()) } } @@ -546,7 +560,7 @@ impl Lua { /// `Error::MismatchedRegistryKey` if passed a `RegistryKey` that was not created with a /// matching `Lua` state. pub fn owns_registry_value(&self, key: &RegistryKey) -> bool { - unsafe { Arc::ptr_eq(&key.drop_list, &(*self.extra()).registry_drop_list) } + unsafe { Arc::ptr_eq(&key.unref_list, &(*self.extra()).registry_unref_list) } } /// Remove any registry values whose `RegistryKey`s have all been dropped. Unlike normal handle @@ -554,11 +568,11 @@ impl Lua { /// can call this method to remove any unreachable registry values. pub fn expire_registry_values(&self) { unsafe { - let drop_list = mem::replace( - (*self.extra()).registry_drop_list.lock().unwrap().as_mut(), + let unref_list = mem::replace( + (*self.extra()).registry_unref_list.lock().unwrap().as_mut(), Vec::new(), ); - for id in drop_list { + for id in unref_list { ffi::luaL_unref(self.state, ffi::LUA_REGISTRYINDEX, id); } } @@ -694,6 +708,7 @@ impl Lua { LuaRef { lua: self, registry_id: registry_id, + drop_unref: true, } } @@ -920,7 +935,7 @@ impl Lua { let extra_data = Box::into_raw(Box::new(ExtraData { registered_userdata: HashMap::new(), - registry_drop_list: Arc::new(Mutex::new(Vec::new())), + registry_unref_list: Arc::new(Mutex::new(Vec::new())), })); *(ffi::lua_getextraspace(state) as *mut *mut ExtraData) = extra_data; }); @@ -934,6 +949,11 @@ impl Lua { fn create_callback_function<'lua>(&'lua self, func: Callback<'lua>) -> Result> { unsafe extern "C" fn callback_call_impl(state: *mut ffi::lua_State) -> c_int { + if ffi::lua_type(state, ffi::lua_upvalueindex(1)) == ffi::LUA_TNIL { + ffi::lua_pushstring(state, cstr!("rust callback has been destructed")); + ffi::lua_error(state) + } + callback_error(state, || { let lua = Lua { state: state, @@ -976,7 +996,7 @@ impl Lua { self.state, &FUNCTION_METATABLE_REGISTRY_KEY as *const u8 as *mut c_void, ); - ffi::lua_gettable(self.state, ffi::LUA_REGISTRYINDEX); + ffi::lua_rawget(self.state, ffi::LUA_REGISTRYINDEX); ffi::lua_setmetatable(self.state, -2); protect_lua_call(self.state, 1, 1, |state| { @@ -988,9 +1008,104 @@ impl Lua { } } + fn do_create_userdata(&self, data: T) -> Result + where + T: UserData, + { + unsafe { + stack_err_guard(self.state, 0, move || { + check_stack(self.state, 3); + + push_userdata::>(self.state, RefCell::new(data))?; + + ffi::lua_rawgeti( + self.state, + ffi::LUA_REGISTRYINDEX, + self.userdata_metatable::()? as ffi::lua_Integer, + ); + + ffi::lua_setmetatable(self.state, -2); + + Ok(AnyUserData(self.pop_ref(self.state))) + }) + } + } + unsafe fn extra(&self) -> *mut ExtraData { *(ffi::lua_getextraspace(self.main_state) as *mut *mut ExtraData) } } +impl<'lua> Scope<'lua> { + pub fn create_function<'scope, A, R, F>(&'scope self, mut func: F) -> Result> + where + A: FromLuaMulti<'scope>, + R: ToLuaMulti<'scope>, + F: 'scope + FnMut(&'scope Lua, A) -> Result, + { + unsafe { + let mut f = self.lua + .create_callback_function(Box::new(move |lua, args| { + func(lua, A::from_lua_multi(args, lua)?)?.to_lua_multi(lua) + }))?; + f.0.drop_unref = false; + let mut destructors = self.destructors.borrow_mut(); + let registry_id = f.0.registry_id; + destructors.push(Box::new(move |state| { + check_stack(state, 2); + ffi::lua_rawgeti( + state, + ffi::LUA_REGISTRYINDEX, + registry_id as ffi::lua_Integer, + ); + ffi::lua_getupvalue(state, -1, 1); + destruct_userdata::>(state); + + ffi::lua_pushnil(state); + ffi::lua_setupvalue(state, -2, 1); + + ffi::lua_pop(state, 1); + })); + Ok(f) + } + } + + pub fn create_userdata(&self, data: T) -> Result + where + T: UserData, + { + unsafe { + let mut u = self.lua.do_create_userdata(data)?; + u.0.drop_unref = false; + let mut destructors = self.destructors.borrow_mut(); + let registry_id = u.0.registry_id; + destructors.push(Box::new(move |state| { + check_stack(state, 1); + ffi::lua_rawgeti( + state, + ffi::LUA_REGISTRYINDEX, + registry_id as ffi::lua_Integer, + ); + destruct_userdata::>(state); + })); + Ok(u) + } + } +} + +impl<'lua> Drop for Scope<'lua> { + fn drop(&mut self) { + let state = self.lua.state; + for mut destructor in self.destructors.get_mut().drain(..) { + match catch_unwind(AssertUnwindSafe(move || destructor(state))) { + Ok(_) => {} + Err(_) => { + eprintln!("Scope userdata Drop impl has panicked, aborting!"); + process::abort() + } + } + } + } +} + static FUNCTION_METATABLE_REGISTRY_KEY: u8 = 0; diff --git a/src/table.rs b/src/table.rs index 679849a..29af395 100644 --- a/src/table.rs +++ b/src/table.rs @@ -266,6 +266,7 @@ impl<'lua> Table<'lua> { let next_key = Some(LuaRef { lua: self.0.lua, registry_id: ffi::LUA_REFNIL, + drop_unref: true, }); TablePairs { diff --git a/src/tests.rs b/src/tests.rs index 1185b1b..bb721d8 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -1,6 +1,8 @@ use std::fmt; use std::error; use std::rc::Rc; +use std::cell::Cell; +use std::sync::Arc; use std::panic::catch_unwind; use {Error, ExternalError, Function, Lua, Nil, Result, Table, UserData, Value, Variadic}; @@ -539,16 +541,16 @@ fn test_registry_value() { #[test] fn test_drop_registry_value() { - struct MyUserdata(Rc<()>); + struct MyUserdata(Arc<()>); impl UserData for MyUserdata {} let lua = Lua::new(); - let rc = Rc::new(()); + let rc = Arc::new(()); let r = lua.create_registry_value(MyUserdata(rc.clone())).unwrap(); - assert_eq!(Rc::strong_count(&rc), 2); + assert_eq!(Arc::strong_count(&rc), 2); drop(r); lua.expire_registry_values(); @@ -556,7 +558,7 @@ fn test_drop_registry_value() { lua.exec::<()>(r#"collectgarbage("collect")"#, None) .unwrap(); - assert_eq!(Rc::strong_count(&rc), 1); + assert_eq!(Arc::strong_count(&rc), 1); } #[test] @@ -597,6 +599,72 @@ fn test_mismatched_registry_key() { }; } +#[test] +fn scope_func() { + let rc = Rc::new(Cell::new(0)); + + let lua = Lua::new(); + lua.scope(|scope| { + let r = rc.clone(); + let f = scope + .create_function(move |_, ()| { + r.set(42); + Ok(()) + }) + .unwrap(); + lua.globals().set("bad", f.clone()).unwrap(); + f.call::<_, ()>(()).unwrap(); + }); + assert_eq!(rc.get(), 42); + assert_eq!(Rc::strong_count(&rc), 1); + + assert!( + lua.globals() + .get::<_, Function>("bad") + .unwrap() + .call::<_, ()>(()) + .is_err() + ); +} + +#[test] +fn scope_drop() { + struct MyUserdata(Rc<()>); + impl UserData for MyUserdata {} + + let rc = Rc::new(()); + + let lua = Lua::new(); + lua.scope(|scope| { + lua.globals() + .set( + "test", + scope.create_userdata(MyUserdata(rc.clone())).unwrap(), + ) + .unwrap(); + assert_eq!(Rc::strong_count(&rc), 2); + }); + assert_eq!(Rc::strong_count(&rc), 1); +} + +#[test] +fn scope_capture() { + let mut i = 0; + + let lua = Lua::new(); + lua.scope(|scope| { + scope + .create_function(|_, ()| { + i = 42; + Ok(()) + }) + .unwrap() + .call::<_, ()>(()) + .unwrap(); + }); + assert_eq!(i, 42); +} + // TODO: Need to use compiletest-rs or similar to make sure these don't compile. /* #[test] @@ -619,5 +687,15 @@ fn should_not_compile() { globals.set("boom", lua.create_function(|_, _| { lua.eval::("1 + 1", None) })).unwrap(); + + // Should not allow Scope references to leak + struct MyUserdata(Rc<()>); + impl UserData for MyUserdata {} + + let lua = Lua::new(); + let mut r = None; + lua.scope(|scope| { + r = Some(scope.create_userdata(MyUserdata(Rc::new(()))).unwrap()); + }); } */ diff --git a/src/types.rs b/src/types.rs index 5aa531b..de9f4e9 100644 --- a/src/types.rs +++ b/src/types.rs @@ -25,13 +25,14 @@ pub struct LightUserData(pub *mut c_void); /// can be used in many situations where it would be impossible to store a regular handle value. pub struct RegistryKey { pub(crate) registry_id: c_int, - pub(crate) drop_list: Arc>>, + pub(crate) unref_list: Arc>>, + pub(crate) drop_unref: bool, } impl Drop for RegistryKey { fn drop(&mut self) { - if self.registry_id != ffi::LUA_REFNIL { - self.drop_list.lock().unwrap().push(self.registry_id); + if self.drop_unref { + self.unref_list.lock().unwrap().push(self.registry_id); } } } @@ -42,6 +43,7 @@ pub(crate) type Callback<'lua> = pub(crate) struct LuaRef<'lua> { pub lua: &'lua Lua, pub registry_id: c_int, + pub drop_unref: bool, } impl<'lua> fmt::Debug for LuaRef<'lua> { @@ -52,17 +54,27 @@ impl<'lua> fmt::Debug for LuaRef<'lua> { impl<'lua> Clone for LuaRef<'lua> { fn clone(&self) -> Self { - unsafe { - self.lua.push_ref(self.lua.state, self); - self.lua.pop_ref(self.lua.state) + if self.drop_unref { + unsafe { + self.lua.push_ref(self.lua.state, self); + self.lua.pop_ref(self.lua.state) + } + } else { + LuaRef { + lua: self.lua, + registry_id: self.registry_id, + drop_unref: self.drop_unref, + } } } } impl<'lua> Drop for LuaRef<'lua> { fn drop(&mut self) { - unsafe { - ffi::luaL_unref(self.lua.state, ffi::LUA_REGISTRYINDEX, self.registry_id); + if self.drop_unref { + unsafe { + ffi::luaL_unref(self.lua.state, ffi::LUA_REGISTRYINDEX, self.registry_id); + } } } } diff --git a/src/userdata.rs b/src/userdata.rs index 8c154b5..536c853 100644 --- a/src/userdata.rs +++ b/src/userdata.rs @@ -89,7 +89,7 @@ impl<'lua, T: UserData> UserDataMethods<'lua, T> { where A: FromLuaMulti<'lua>, R: ToLuaMulti<'lua>, - M: 'static + for<'a> FnMut(&'lua Lua, &'a T, A) -> Result, + M: 'static + Send + for<'a> FnMut(&'lua Lua, &'a T, A) -> Result, { self.methods .insert(name.to_owned(), Self::box_method(method)); @@ -104,7 +104,7 @@ impl<'lua, T: UserData> UserDataMethods<'lua, T> { where A: FromLuaMulti<'lua>, R: ToLuaMulti<'lua>, - M: 'static + for<'a> FnMut(&'lua Lua, &'a mut T, A) -> Result, + M: 'static + Send + for<'a> FnMut(&'lua Lua, &'a mut T, A) -> Result, { self.methods .insert(name.to_owned(), Self::box_method_mut(method)); @@ -121,7 +121,7 @@ impl<'lua, T: UserData> UserDataMethods<'lua, T> { where A: FromLuaMulti<'lua>, R: ToLuaMulti<'lua>, - F: 'static + FnMut(&'lua Lua, A) -> Result, + F: 'static + Send + FnMut(&'lua Lua, A) -> Result, { self.methods .insert(name.to_owned(), Self::box_function(function)); @@ -139,7 +139,7 @@ impl<'lua, T: UserData> UserDataMethods<'lua, T> { where A: FromLuaMulti<'lua>, R: ToLuaMulti<'lua>, - M: 'static + for<'a> FnMut(&'lua Lua, &'a T, A) -> Result, + M: 'static + Send + for<'a> FnMut(&'lua Lua, &'a T, A) -> Result, { self.meta_methods.insert(meta, Self::box_method(method)); } @@ -156,7 +156,7 @@ impl<'lua, T: UserData> UserDataMethods<'lua, T> { where A: FromLuaMulti<'lua>, R: ToLuaMulti<'lua>, - M: 'static + for<'a> FnMut(&'lua Lua, &'a mut T, A) -> Result, + M: 'static + Send + for<'a> FnMut(&'lua Lua, &'a mut T, A) -> Result, { self.meta_methods.insert(meta, Self::box_method_mut(method)); } @@ -170,7 +170,7 @@ impl<'lua, T: UserData> UserDataMethods<'lua, T> { where A: FromLuaMulti<'lua>, R: ToLuaMulti<'lua>, - F: 'static + FnMut(&'lua Lua, A) -> Result, + F: 'static + Send + FnMut(&'lua Lua, A) -> Result, { self.meta_methods.insert(meta, Self::box_function(function)); } @@ -179,7 +179,7 @@ impl<'lua, T: UserData> UserDataMethods<'lua, T> { where A: FromLuaMulti<'lua>, R: ToLuaMulti<'lua>, - F: 'static + FnMut(&'lua Lua, A) -> Result, + F: 'static + Send + FnMut(&'lua Lua, A) -> Result, { Box::new(move |lua, args| function(lua, A::from_lua_multi(args, lua)?)?.to_lua_multi(lua)) } @@ -188,7 +188,7 @@ impl<'lua, T: UserData> UserDataMethods<'lua, T> { where A: FromLuaMulti<'lua>, R: ToLuaMulti<'lua>, - M: 'static + for<'a> FnMut(&'lua Lua, &'a T, A) -> Result, + M: 'static + Send + for<'a> FnMut(&'lua Lua, &'a T, A) -> Result, { Box::new(move |lua, mut args| { if let Some(front) = args.pop_front() { @@ -209,7 +209,7 @@ impl<'lua, T: UserData> UserDataMethods<'lua, T> { where A: FromLuaMulti<'lua>, R: ToLuaMulti<'lua>, - M: 'static + for<'a> FnMut(&'lua Lua, &'a mut T, A) -> Result, + M: 'static + Send + for<'a> FnMut(&'lua Lua, &'a mut T, A) -> Result, { Box::new(move |lua, mut args| { if let Some(front) = args.pop_front() { @@ -433,7 +433,7 @@ impl<'lua> AnyUserData<'lua> { #[cfg(test)] mod tests { - use std::rc::Rc; + use std::sync::Arc; use super::{MetaMethod, UserData, UserDataMethods}; use error::ExternalError; @@ -590,11 +590,11 @@ mod tests { #[test] fn detroys_userdata() { - struct MyUserdata(Rc<()>); + struct MyUserdata(Arc<()>); impl UserData for MyUserdata {} - let rc = Rc::new(()); + let rc = Arc::new(()); let lua = Lua::new(); { @@ -602,9 +602,9 @@ mod tests { globals.set("userdata", MyUserdata(rc.clone())).unwrap(); } - assert_eq!(Rc::strong_count(&rc), 2); + assert_eq!(Arc::strong_count(&rc), 2); drop(lua); // should destroy all objects - assert_eq!(Rc::strong_count(&rc), 1); + assert_eq!(Arc::strong_count(&rc), 1); } #[test] diff --git a/src/util.rs b/src/util.rs index fa36028..1de9b9f 100644 --- a/src/util.rs +++ b/src/util.rs @@ -250,18 +250,24 @@ pub unsafe fn get_userdata(state: *mut ffi::lua_State, index: c_int) -> *mut pub unsafe extern "C" fn userdata_destructor(state: *mut ffi::lua_State) -> c_int { callback_error(state, || { - // We set the metatable of userdata on __gc to a special table with no __gc method and with - // metamethods that trigger an error on access. We do this 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. - get_gc_userdata_metatable(state); - ffi::lua_setmetatable(state, -2); - let ud = &mut *(ffi::lua_touserdata(state, 1) as *mut T); - mem::replace(ud, mem::uninitialized()); + destruct_userdata::(state); Ok(0) }) } +// Pops the userdata off of the top of the stack and drops it +pub unsafe fn destruct_userdata(state: *mut ffi::lua_State) { + // We set the metatable of userdata on __gc to a special table with no __gc method and with + // metamethods that trigger an error on access. We do this 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. + get_destructed_userdata_metatable(state); + ffi::lua_setmetatable(state, -2); + let ud = &mut *(ffi::lua_touserdata(state, -1) as *mut T); + ffi::lua_pop(state, 1); + mem::replace(ud, mem::uninitialized()); +} + // In the context of a lua callback, this will call the given function and if the given function // returns an error, *or if the given function panics*, this will result in a call to lua_error (a // longjmp). The error or panic is wrapped in such a way that when calling pop_error back on @@ -517,7 +523,7 @@ unsafe fn get_error_metatable(state: *mut ffi::lua_State) -> c_int { state, &ERROR_METATABLE_REGISTRY_KEY as *const u8 as *mut c_void, ); - let t = ffi::lua_gettable(state, ffi::LUA_REGISTRYINDEX); + let t = ffi::lua_rawget(state, ffi::LUA_REGISTRYINDEX); if t != ffi::LUA_TTABLE { ffi::lua_pop(state, 1); @@ -558,7 +564,7 @@ unsafe fn get_panic_metatable(state: *mut ffi::lua_State) -> c_int { state, &PANIC_METATABLE_REGISTRY_KEY as *const u8 as *mut c_void, ); - let t = ffi::lua_gettable(state, ffi::LUA_REGISTRYINDEX); + let t = ffi::lua_rawget(state, ffi::LUA_REGISTRYINDEX); if t != ffi::LUA_TTABLE { ffi::lua_pop(state, 1); @@ -588,16 +594,19 @@ unsafe fn get_panic_metatable(state: *mut ffi::lua_State) -> c_int { ffi::LUA_TTABLE } -unsafe fn get_gc_userdata_metatable(state: *mut ffi::lua_State) -> c_int { - static GC_USERDATA_METATABLE: u8 = 0; +unsafe fn get_destructed_userdata_metatable(state: *mut ffi::lua_State) -> c_int { + static DESTRUCTED_USERDATA_METATABLE: u8 = 0; - unsafe extern "C" fn gc_error(state: *mut ffi::lua_State) -> c_int { - ffi::lua_pushstring(state, cstr!("userdata has been garbage collected")); + unsafe extern "C" fn destructed_error(state: *mut ffi::lua_State) -> c_int { + ffi::lua_pushstring(state, cstr!("userdata has been destructed")); ffi::lua_error(state) } - ffi::lua_pushlightuserdata(state, &GC_USERDATA_METATABLE as *const u8 as *mut c_void); - let t = ffi::lua_gettable(state, ffi::LUA_REGISTRYINDEX); + ffi::lua_pushlightuserdata( + state, + &DESTRUCTED_USERDATA_METATABLE as *const u8 as *mut c_void, + ); + let t = ffi::lua_rawget(state, ffi::LUA_REGISTRYINDEX); if t != ffi::LUA_TTABLE { ffi::lua_pop(state, 1); @@ -606,7 +615,10 @@ unsafe fn get_gc_userdata_metatable(state: *mut ffi::lua_State) -> c_int { gc_guard(state, || { ffi::lua_newtable(state); - ffi::lua_pushlightuserdata(state, &GC_USERDATA_METATABLE as *const u8 as *mut c_void); + ffi::lua_pushlightuserdata( + state, + &DESTRUCTED_USERDATA_METATABLE as *const u8 as *mut c_void, + ); ffi::lua_pushvalue(state, -2); for &method in &[ @@ -637,7 +649,7 @@ unsafe fn get_gc_userdata_metatable(state: *mut ffi::lua_State) -> c_int { cstr!("__ipairs"), ] { ffi::lua_pushstring(state, method); - ffi::lua_pushcfunction(state, gc_error); + ffi::lua_pushcfunction(state, destructed_error); ffi::lua_rawset(state, -3); } From ab9841a02fb0c9ebe76e15247e68401fd13474cb Mon Sep 17 00:00:00 2001 From: kyren Date: Wed, 7 Feb 2018 11:16:22 -0500 Subject: [PATCH 2/5] Don't keep the unref list around forever after Lua is dropped --- src/lua.rs | 11 ++++++----- src/types.rs | 6 ++++-- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/lua.rs b/src/lua.rs index 948d142..0b02a14 100644 --- a/src/lua.rs +++ b/src/lua.rs @@ -39,7 +39,7 @@ pub struct Scope<'lua> { // Data associated with the main lua_State via lua_getextraspace. struct ExtraData { registered_userdata: HashMap, - registry_unref_list: Arc>>, + registry_unref_list: Arc>>>, } unsafe impl Send for Lua {} @@ -57,6 +57,7 @@ impl Drop for Lua { } let extra_data = *(ffi::lua_getextraspace(self.state) as *mut *mut ExtraData); + *(*extra_data).registry_unref_list.lock().unwrap() = None; Box::from_raw(extra_data); ffi::lua_close(self.state); @@ -569,10 +570,10 @@ impl Lua { pub fn expire_registry_values(&self) { unsafe { let unref_list = mem::replace( - (*self.extra()).registry_unref_list.lock().unwrap().as_mut(), - Vec::new(), + &mut *(*self.extra()).registry_unref_list.lock().unwrap(), + Some(Vec::new()), ); - for id in unref_list { + for id in unref_list.unwrap() { ffi::luaL_unref(self.state, ffi::LUA_REGISTRYINDEX, id); } } @@ -935,7 +936,7 @@ impl Lua { let extra_data = Box::into_raw(Box::new(ExtraData { registered_userdata: HashMap::new(), - registry_unref_list: Arc::new(Mutex::new(Vec::new())), + registry_unref_list: Arc::new(Mutex::new(Some(Vec::new()))), })); *(ffi::lua_getextraspace(state) as *mut *mut ExtraData) = extra_data; }); diff --git a/src/types.rs b/src/types.rs index de9f4e9..fb3a5d0 100644 --- a/src/types.rs +++ b/src/types.rs @@ -25,14 +25,16 @@ pub struct LightUserData(pub *mut c_void); /// can be used in many situations where it would be impossible to store a regular handle value. pub struct RegistryKey { pub(crate) registry_id: c_int, - pub(crate) unref_list: Arc>>, + pub(crate) unref_list: Arc>>>, pub(crate) drop_unref: bool, } impl Drop for RegistryKey { fn drop(&mut self) { if self.drop_unref { - self.unref_list.lock().unwrap().push(self.registry_id); + if let Some(list) = self.unref_list.lock().unwrap().as_mut() { + list.push(self.registry_id); + } } } } From 98ee4e9492b626268b7170d713686da6a258cbb9 Mon Sep 17 00:00:00 2001 From: kyren Date: Wed, 7 Feb 2018 16:42:03 -0500 Subject: [PATCH 3/5] More correct scope drop behavior now no longer aborts if a Drop impl panics --- src/lua.rs | 23 +++++++++++------------ src/util.rs | 21 +++++++++++---------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/lua.rs b/src/lua.rs index 0b02a14..588a0b2 100644 --- a/src/lua.rs +++ b/src/lua.rs @@ -3,11 +3,10 @@ use std::sync::{Arc, Mutex}; use std::ops::DerefMut; use std::cell::RefCell; use std::ffi::CString; -use std::any::TypeId; +use std::any::{Any, TypeId}; use std::marker::PhantomData; use std::collections::HashMap; use std::os::raw::{c_char, c_int, c_void}; -use std::panic::{catch_unwind, AssertUnwindSafe}; use libc; @@ -33,7 +32,7 @@ pub struct Lua { /// !Send, and callbacks that are !Send and not 'static. pub struct Scope<'lua> { lua: &'lua Lua, - destructors: RefCell>>, + destructors: RefCell Box>>>, } // Data associated with the main lua_State via lua_getextraspace. @@ -1060,12 +1059,13 @@ impl<'lua> Scope<'lua> { registry_id as ffi::lua_Integer, ); ffi::lua_getupvalue(state, -1, 1); - destruct_userdata::>(state); + let ud = take_userdata::>(state); ffi::lua_pushnil(state); ffi::lua_setupvalue(state, -2, 1); ffi::lua_pop(state, 1); + Box::new(ud) })); Ok(f) } @@ -1087,7 +1087,7 @@ impl<'lua> Scope<'lua> { ffi::LUA_REGISTRYINDEX, registry_id as ffi::lua_Integer, ); - destruct_userdata::>(state); + Box::new(take_userdata::>(state)) })); Ok(u) } @@ -1096,15 +1096,14 @@ impl<'lua> Scope<'lua> { impl<'lua> Drop for Scope<'lua> { fn drop(&mut self) { + // We separate the action of invalidating the userdata in Lua and actually dropping the + // userdata type into two phases. This is so that, in the event a userdata drop panics, we + // can be sure that all of the userdata in Lua is actually invalidated. + let state = self.lua.state; + let mut drops = Vec::new(); for mut destructor in self.destructors.get_mut().drain(..) { - match catch_unwind(AssertUnwindSafe(move || destructor(state))) { - Ok(_) => {} - Err(_) => { - eprintln!("Scope userdata Drop impl has panicked, aborting!"); - process::abort() - } - } + drops.push(destructor(state)); } } } diff --git a/src/util.rs b/src/util.rs index 1de9b9f..aba6ba6 100644 --- a/src/util.rs +++ b/src/util.rs @@ -248,15 +248,9 @@ pub unsafe fn get_userdata(state: *mut ffi::lua_State, index: c_int) -> *mut ud } -pub unsafe extern "C" fn userdata_destructor(state: *mut ffi::lua_State) -> c_int { - callback_error(state, || { - destruct_userdata::(state); - Ok(0) - }) -} - -// Pops the userdata off of the top of the stack and drops it -pub unsafe fn destruct_userdata(state: *mut ffi::lua_State) { +// Pops the userdata off of the top of the stack and returns it to rust, invalidating the lua +// userdata. +pub unsafe fn take_userdata(state: *mut ffi::lua_State) -> T { // We set the metatable of userdata on __gc to a special table with no __gc method and with // metamethods that trigger an error on access. We do this so that it will not be double // dropped, and also so that it cannot be used or identified as any particular userdata type @@ -265,7 +259,14 @@ pub unsafe fn destruct_userdata(state: *mut ffi::lua_State) { ffi::lua_setmetatable(state, -2); let ud = &mut *(ffi::lua_touserdata(state, -1) as *mut T); ffi::lua_pop(state, 1); - mem::replace(ud, mem::uninitialized()); + mem::replace(ud, mem::uninitialized()) +} + +pub unsafe extern "C" fn userdata_destructor(state: *mut ffi::lua_State) -> c_int { + callback_error(state, || { + take_userdata::(state); + Ok(0) + }) } // In the context of a lua callback, this will call the given function and if the given function From b9d9bea28a86ff8097bb52f0398143692aedb5b9 Mon Sep 17 00:00:00 2001 From: kyren Date: Wed, 7 Feb 2018 16:51:24 -0500 Subject: [PATCH 4/5] slightly faster, less obnoxious scope drop --- src/lua.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/lua.rs b/src/lua.rs index 588a0b2..a0320e6 100644 --- a/src/lua.rs +++ b/src/lua.rs @@ -1101,10 +1101,12 @@ impl<'lua> Drop for Scope<'lua> { // can be sure that all of the userdata in Lua is actually invalidated. let state = self.lua.state; - let mut drops = Vec::new(); - for mut destructor in self.destructors.get_mut().drain(..) { - drops.push(destructor(state)); - } + let to_drop = self.destructors + .get_mut() + .drain(..) + .map(|mut destructor| destructor(state)) + .collect::>(); + drop(to_drop); } } From 164250b352b5d08b94e107823ac0f53fded5d454 Mon Sep 17 00:00:00 2001 From: kyren Date: Wed, 7 Feb 2018 17:05:00 -0500 Subject: [PATCH 5/5] Don't panic with "rlua internal error" message on panics that are not internal It is part of the contract that only LuaRef types constructed from the same parent Lua state are passed into Lua, so generating a panic there is not an internal error. --- src/macros.rs | 46 ++++++++++++++++++++++++++-------------------- src/userdata.rs | 2 +- src/util.rs | 20 ++++++++++---------- 3 files changed, 37 insertions(+), 31 deletions(-) diff --git a/src/macros.rs b/src/macros.rs index 1408384..929e2d7 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -6,48 +6,54 @@ macro_rules! cstr { // A panic that clears the given lua stack before panicking macro_rules! lua_panic { - ($state:expr) => { - { - $crate::ffi::lua_settor($state, 0); - panic!("rlua internal error"); - } - }; - ($state:expr, $msg:expr) => { { $crate::ffi::lua_settop($state, 0); - panic!(concat!("rlua internal error: ", $msg)); + panic!($msg); } }; - ($state:expr, $fmt:expr, $($arg:tt)+) => { + ($state:expr, $msg:expr, $($arg:tt)+) => { { $crate::ffi::lua_settop($state, 0); - panic!(concat!("rlua internal error: ", $fmt), $($arg)+); + panic!($msg, $($arg)+); } }; } // An assert that clears the given lua stack before panicking macro_rules! lua_assert { - ($state:expr, $cond:expr) => { - if !$cond { - $crate::ffi::lua_settop($state, 0); - panic!("rlua internal error"); - } - }; - ($state:expr, $cond:expr, $msg:expr) => { if !$cond { $crate::ffi::lua_settop($state, 0); - panic!(concat!("rlua internal error: ", $msg)); + panic!($msg); } }; - ($state:expr, $cond:expr, $fmt:expr, $($arg:tt)+) => { + ($state:expr, $cond:expr, $msg:expr, $($arg:tt)+) => { if !$cond { $crate::ffi::lua_settop($state, 0); - panic!(concat!("rlua internal error: ", $fmt), $($arg)+); + panic!($msg, $($arg)+); } }; } + +macro_rules! lua_internal_panic { + ($state:expr, $msg:expr) => { + lua_panic!($state, concat!("rlua internal error: ", $msg)); + }; + + ($state:expr, $msg:expr, $($arg:tt)+) => { + lua_panic!($state, concat!("rlua internal error: ", $msg), $($arg)+); + }; +} + +macro_rules! lua_internal_assert { + ($state:expr, $cond:expr, $msg:expr) => { + lua_assert!($state, $cond, concat!("rlua internal error: ", $msg)); + }; + + ($state:expr, $cond:expr, $msg:expr, $($arg:tt)+) => { + lua_assert!($state, $cond, concat!("rlua internal error: ", $msg), $($arg)+); + }; +} diff --git a/src/userdata.rs b/src/userdata.rs index 536c853..8c7a92f 100644 --- a/src/userdata.rs +++ b/src/userdata.rs @@ -370,7 +370,7 @@ impl<'lua> AnyUserData<'lua> { lua.push_ref(lua.state, &self.0); - lua_assert!( + lua_internal_assert!( lua.state, ffi::lua_getmetatable(lua.state, -1) != 0, "AnyUserData missing metatable" diff --git a/src/util.rs b/src/util.rs index aba6ba6..9789d02 100644 --- a/src/util.rs +++ b/src/util.rs @@ -11,7 +11,7 @@ use error::{Error, Result}; // Checks that Lua has enough free stack space for future stack operations. // On failure, this will clear the stack and panic. pub unsafe fn check_stack(state: *mut ffi::lua_State, amount: c_int) { - lua_assert!( + lua_internal_assert!( state, ffi::lua_checkstack(state, amount) != 0, "out of stack space" @@ -25,7 +25,7 @@ where F: FnOnce() -> R, { let expected = ffi::lua_gettop(state) + change; - lua_assert!( + lua_internal_assert!( state, expected >= 0, "too many stack values would be popped" @@ -34,7 +34,7 @@ where let res = op(); let top = ffi::lua_gettop(state); - lua_assert!( + lua_internal_assert!( state, ffi::lua_gettop(state) == expected, "expected stack to be {}, got {}", @@ -59,7 +59,7 @@ where F: FnOnce() -> Result, { let expected = ffi::lua_gettop(state) + change; - lua_assert!( + lua_internal_assert!( state, expected >= 0, "too many stack values would be popped" @@ -69,7 +69,7 @@ where let top = ffi::lua_gettop(state); if res.is_ok() { - lua_assert!( + lua_internal_assert!( state, ffi::lua_gettop(state) == expected, "expected stack to be {}, got {}", @@ -77,7 +77,7 @@ where top ); } else { - lua_assert!( + lua_internal_assert!( state, top >= expected, "{} too many stack values popped", @@ -171,7 +171,7 @@ where // the current lua stack and continues the panic. If the error on the top of the stack is actually // a WrappedError, just returns it. Otherwise, interprets the error as the appropriate lua error. pub unsafe fn pop_error(state: *mut ffi::lua_State, err_code: c_int) -> Error { - lua_assert!( + lua_internal_assert!( state, err_code != ffi::LUA_OK && err_code != ffi::LUA_YIELD, "pop_error called with non-error return code" @@ -185,7 +185,7 @@ pub unsafe fn pop_error(state: *mut ffi::lua_State, err_code: c_int) -> Error { ffi::lua_settop(state, 0); resume_unwind(p); } else { - lua_panic!(state, "panic was resumed twice") + lua_internal_panic!(state, "panic was resumed twice") } } else { let err_string = gc_guard(state, || { @@ -221,7 +221,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, "unrecognized lua error code"), + _ => lua_internal_panic!(state, "unrecognized lua error code"), } } } @@ -244,7 +244,7 @@ pub unsafe fn push_userdata(state: *mut ffi::lua_State, t: T) -> Result<()> { // 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) -> *mut T { let ud = ffi::lua_touserdata(state, index) as *mut T; - lua_assert!(state, !ud.is_null(), "userdata pointer is null"); + lua_internal_assert!(state, !ud.is_null(), "userdata pointer is null"); ud }