delta/mlua
1
0
Fork 0
Code Issues Pull requests Packages Projects Releases Wiki Activity

Fix some bad potential unsafety on inner callback calls.

Browse source 
Since we now optionally use stack spaces for handle values, we have to be
mindful of whether our stack handle points to the stack in an outer level of
Lua "stack protection".  We now keep track of the "recursion level" of Lua
instances, and do not allow ref manipulation on "outer" Lua instances until the
inner callback has returned.  Also, update the documentation to reflect the
additional panic behavior.
This commit is contained in:
kyren 2018-03-12 22:25:45 -04:00
parent c1e1ac432c
commit 985636267c
4 changed files with 137 additions and 32 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
README.md
View file

@ -65,7 +65,8 @@ Caveats to the panic / abort guarantee:
* `rlua` reserves the right to panic on API usage errors. Currently, the only
time this will happen is when passed a handle type from a mismatched `Lua`
instance.
instance, or when accessing outer level `Lua` instances and handles inside
an inner callback.
* Currently, there are no memory or execution limits on scripts, so untrusted
scripts can always at minimum infinite loop or allocate arbitrary amounts of
memory.

117
src/lua.rs
View file

@ -27,20 +27,10 @@ use scope::Scope;
/// Top level Lua struct which holds the Lua state itself.
pub struct Lua {
pub(crate) state: *mut ffi::lua_State,
ephemeral: bool,
recursion_level: usize,
ref_stack_slots: [Cell<usize>; REF_STACK_SIZE as usize],
}
// Data associated with the main lua_State via lua_getextraspace.
struct ExtraData {
registered_userdata: HashMap<TypeId, c_int>,
registry_unref_list: Arc<Mutex<Option<Vec<c_int>>>>,
}
const REF_STACK_SIZE: c_int = 16;
static FUNCTION_METATABLE_REGISTRY_KEY: u8 = 0;
unsafe impl Send for Lua {}
impl Drop for Lua {
@ -51,7 +41,7 @@ impl Drop for Lua {
rlua_assert!(use_count.get() == 0, "live stack reference detected");
}
if !self.ephemeral {
if self.recursion_level == 0 {
let top = ffi::lua_gettop(self.state);
rlua_assert!(
top == REF_STACK_SIZE,
@ -61,8 +51,14 @@ impl Drop for Lua {
}
}
if !self.ephemeral {
if self.recursion_level == 0 {
let extra_data = *(ffi::lua_getextraspace(self.state) as *mut *mut ExtraData);
rlua_debug_assert!(
(*extra_data).recursion_level == 0,
"Lua recursion level nonzero on Lua drop"
);
*(*extra_data).registry_unref_list.lock().unwrap() = None;
Box::from_raw(extra_data);
@ -545,7 +541,7 @@ impl Lua {
Ok(RegistryKey {
registry_id,
unref_list: (*self.extra()).registry_unref_list.clone(),
unref_list: (*ExtraData::get(self.state)).registry_unref_list.clone(),
})
}
}
@ -601,7 +597,12 @@ 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.unref_list, &(*self.extra()).registry_unref_list) }
unsafe {
Arc::ptr_eq(
&key.unref_list,
&(*ExtraData::get(self.state)).registry_unref_list,
)
}
}
/// Remove any registry values whose `RegistryKey`s have all been dropped.
@ -612,7 +613,10 @@ impl Lua {
pub fn expire_registry_values(&self) {
unsafe {
let unref_list = mem::replace(
&mut *(*self.extra()).registry_unref_list.lock().unwrap(),
&mut *(*ExtraData::get(self.state))
.registry_unref_list
.lock()
.unwrap(),
Some(Vec::new()),
);
for id in unref_list.unwrap() {
@ -727,6 +731,10 @@ impl Lua {
// Used 1 stack space, does not call checkstack
pub(crate) unsafe fn push_ref(&self, lref: &LuaRef) {
assert!(
self.is_active(),
"parent Lua instance accessed inside callback"
);
assert!(
lref.lua as *const Lua == self as *const Lua,
"Lua instance passed Value created from a different Lua"
@ -755,6 +763,11 @@ impl Lua {
//
// pop_ref uses 1 extra stack space and does not call checkstack
pub(crate) unsafe fn pop_ref(&self) -> LuaRef {
assert!(
self.is_active(),
"parent Lua instance accessed inside callback"
);
for i in 0..REF_STACK_SIZE {
let ref_slot = &self.ref_stack_slots[i as usize];
if ref_slot.get() == 0 {
@ -786,6 +799,11 @@ impl Lua {
}
pub(crate) fn clone_ref(&self, lref: &LuaRef) -> LuaRef {
assert!(
self.is_active(),
"parent Lua instance accessed inside callback"
);
unsafe {
match lref.ref_type {
RefType::Nil => LuaRef {
@ -822,6 +840,11 @@ impl Lua {
}
pub(crate) fn drop_ref(&self, lref: &mut LuaRef) {
assert!(
self.is_active(),
"parent Lua instance accessed inside callback"
);
unsafe {
match lref.ref_type {
RefType::Nil => {}
@ -863,7 +886,10 @@ impl Lua {
}
}
if let Some(table_id) = (*self.extra()).registered_userdata.get(&TypeId::of::<T>()) {
if let Some(table_id) = (*ExtraData::get(self.state))
.registered_userdata
.get(&TypeId::of::<T>())
{
return Ok(*table_id);
}
@ -964,7 +990,7 @@ impl Lua {
let id = gc_guard(self.state, || {
ffi::luaL_ref(self.state, ffi::LUA_REGISTRYINDEX)
});
(*self.extra())
(*ExtraData::get(self.state))
.registered_userdata
.insert(TypeId::of::<T>(), id);
Ok(id)
@ -974,17 +1000,19 @@ impl Lua {
&'lua self,
func: Callback<'callback, 'static>,
) -> Result<Function<'lua>> {
unsafe extern "C" fn callback_call_impl(state: *mut ffi::lua_State) -> c_int {
unsafe extern "C" fn call_callback(state: *mut ffi::lua_State) -> c_int {
callback_error(state, || {
if ffi::lua_type(state, ffi::lua_upvalueindex(1)) == ffi::LUA_TNIL {
return Err(Error::CallbackDestructed);
}
let recursion_guard = RecursionGuard::new(ExtraData::get(state));
let lua = Lua {
state: state,
ephemeral: true,
recursion_level: recursion_guard.recursion_level(),
ref_stack_slots: Default::default(),
};
let args = lua.setup_callback_stack_slots();
let func = get_userdata::<Callback>(state, ffi::lua_upvalueindex(1));
@ -1016,7 +1044,7 @@ impl Lua {
ffi::lua_setmetatable(self.state, -2);
protect_lua_closure(self.state, 1, 1, |state| {
ffi::lua_pushcclosure(state, callback_call_impl, 1);
ffi::lua_pushcclosure(state, call_callback, 1);
})?;
Ok(Function(self.pop_ref()))
@ -1129,6 +1157,7 @@ impl Lua {
// Create ExtraData, and place it in the lua_State "extra space"
let extra_data = Box::into_raw(Box::new(ExtraData {
recursion_level: 0,
registered_userdata: HashMap::new(),
registry_unref_list: Arc::new(Mutex::new(Some(Vec::new()))),
}));
@ -1140,7 +1169,7 @@ impl Lua {
Lua {
state,
ephemeral: false,
recursion_level: 0,
ref_stack_slots: Default::default(),
}
}
@ -1238,7 +1267,47 @@ impl Lua {
}
}
unsafe fn extra(&self) -> *mut ExtraData {
*(ffi::lua_getextraspace(self.state) as *mut *mut ExtraData)
// Returns true if this is the "top" Lua instance. If this returns false, we are a level deeper
// into a callback and `ref_stack_slots` points to an area of the stack that is not currently
// accessible.
fn is_active(&self) -> bool {
unsafe { (*ExtraData::get(self.state)).recursion_level == self.recursion_level }
}
}
// Data associated with the main lua_State via lua_getextraspace.
struct ExtraData {
recursion_level: usize,
registered_userdata: HashMap<TypeId, c_int>,
registry_unref_list: Arc<Mutex<Option<Vec<c_int>>>>,
}
impl ExtraData {
unsafe fn get(state: *mut ffi::lua_State) -> *mut ExtraData {
*(ffi::lua_getextraspace(state) as *mut *mut ExtraData)
}
}
struct RecursionGuard(*mut ExtraData);
impl RecursionGuard {
unsafe fn new(ed: *mut ExtraData) -> RecursionGuard {
(*ed).recursion_level += 1;
RecursionGuard(ed)
}
fn recursion_level(&self) -> usize {
unsafe { (*self.0).recursion_level }
}
}
impl Drop for RecursionGuard {
fn drop(&mut self) {
unsafe {
(*self.0).recursion_level -= 1;
}
}
}
const REF_STACK_SIZE: c_int = 16;
static FUNCTION_METATABLE_REGISTRY_KEY: u8 = 0;

21
src/scope.rs
View file

@ -39,8 +39,23 @@ impl<'scope> Scope<'scope> {
/// This is a version of [`Lua::create_function`] that creates a callback which expires on scope
/// drop. See [`Lua::scope`] for more details.
///
/// Since the provided function does not have to be 'static, it is easy to capture outer
/// variables in the provided callback. However, you must *not* use Lua handle values (`Table`,
/// `Function` etc) or a `Lua` instance that you have captured from an outer level inside such a
/// callback. It is *always* a logic error to access a `Lua` instance or handle value from an
/// "outer" callback level inside an "inner" callback level, Lua does stack protection during
/// callbacks that makes the outer instances unusable until the callback returns. This is true
/// regardless of the use of `Lua::scope`, but it is very difficult (though not impossible!) to
/// run into unless you can create callbacks that are non-'static.
///
/// If you do access outer `Lua` instances or handles inside an inner callback, this will result
/// in a panic. You can instead use either [`RegistryKey`] values or [`Function::bind`] to pass
/// values to callbacks without error.
///
/// [`Lua::create_function`]: struct.Lua.html#method.create_function
/// [`Lua::scope`]: struct.Lua.html#method.scope
/// [`RegistryKey`]: struct.RegistryKey.html
/// [`Function::bind`]: struct.Function.html#method.bind
pub fn create_function<'callback, 'lua, A, R, F>(&'lua self, func: F) -> Result<Function<'lua>>
where
A: FromLuaMulti<'callback>,
@ -78,10 +93,11 @@ impl<'scope> Scope<'scope> {
/// Wraps a Rust mutable closure, creating a callable Lua function handle to it.
///
/// This is a version of [`Lua::create_function_mut`] that creates a callback which expires on
/// scope drop. See [`Lua::scope`] for more details.
/// scope drop. See [`Lua::scope`] and [`Scope::create_function`] for more details.
///
/// [`Lua::create_function_mut`]: struct.Lua.html#method.create_function_mut
/// [`Lua::scope`]: struct.Lua.html#method.scope
/// [`Scope::create_function`]: #method.create_function
pub fn create_function_mut<'callback, 'lua, A, R, F>(
&'lua self,
func: F,
@ -101,7 +117,8 @@ impl<'scope> Scope<'scope> {
/// Create a Lua userdata object from a custom userdata type.
///
/// This is a version of [`Lua::create_userdata`] that creates a userdata which expires on scope
/// drop. See [`Lua::scope`] for more details.
/// drop, and does not require that the userdata type be Send. See [`Lua::scope`] for more
/// details.
///
/// [`Lua::create_userdata`]: struct.Lua.html#method.create_userdata
/// [`Lua::scope`]: struct.Lua.html#method.scope

28
src/tests/mod.rs
View file

@ -610,9 +610,9 @@ fn test_mismatched_registry_key() {
#[test]
fn scope_func() {
let rc = Rc::new(Cell::new(0));
let lua = Lua::new();
let rc = Rc::new(Cell::new(0));
lua.scope(|scope| {
let r = rc.clone();
let f = scope
@ -640,6 +640,8 @@ fn scope_func() {
#[test]
fn scope_drop() {
let lua = Lua::new();
struct MyUserdata(Rc<()>);
impl UserData for MyUserdata {
fn add_methods(methods: &mut UserDataMethods<Self>) {
@ -649,7 +651,6 @@ fn scope_drop() {
let rc = Rc::new(());
let lua = Lua::new();
lua.scope(|scope| {
lua.globals()
.set(
@ -669,9 +670,9 @@ fn scope_drop() {
#[test]
fn scope_capture() {
let mut i = 0;
let lua = Lua::new();
let mut i = 0;
lua.scope(|scope| {
scope
.create_function_mut(|_, ()| {
@ -685,6 +686,23 @@ fn scope_capture() {
assert_eq!(i, 42);
}
#[test]
#[should_panic]
fn outer_lua_access() {
let lua = Lua::new();
let table = lua.create_table().unwrap();
lua.scope(|scope| {
scope
.create_function_mut(|_, ()| {
table.set("a", "b").unwrap();
Ok(())
})
.unwrap()
.call::<_, ()>(())
.unwrap();
});
}
#[test]
fn too_many_returns() {
let lua = Lua::new();