From ab99ff3c31619acd9232a6c33cd02771cb0ef8be Mon Sep 17 00:00:00 2001 From: SeanOMik Date: Sat, 10 Feb 2024 00:17:26 -0500 Subject: [PATCH] fix some significant undefined behavior caused by double dropping userdata --- src/error.rs | 2 +- src/function.rs | 7 +- src/main.rs | 211 +++++++++++++++++++++++++++++--------------- src/state.rs | 62 +++++++++---- src/table.rs | 4 +- src/userdata/mod.rs | 20 ++--- 6 files changed, 201 insertions(+), 105 deletions(-) diff --git a/src/error.rs b/src/error.rs index 117df41..c6a0260 100755 --- a/src/error.rs +++ b/src/error.rs @@ -10,7 +10,7 @@ pub enum Error { #[error("Lua runtime error: {0}")] Runtime(String), #[error("Error when running a __gc metamethod")] - GcFailure(String), + UserdataGc(String), /// Ran into a not enough memory error when trying to grow the lua stack. #[error("Failed to allocate memory")] MemoryAlloc, diff --git a/src/function.rs b/src/function.rs index af74075..143aa84 100755 --- a/src/function.rs +++ b/src/function.rs @@ -59,9 +59,6 @@ impl<'a> Function<'a> { }; self.push_to_lua_stack(self.state)?; - - /* let args_val = args.into_lua_value_vec(self.state)?; - args_val.push_to_lua_stack(self.state)?; */ args_val.push_to_lua_stack(self.state)?; match lua::lua_pcall(s, args_len, lua::LUA_MULTRET, handler_idx) { @@ -84,8 +81,10 @@ impl<'a> Function<'a> { let val = if ret_count > 1 { let vals = ValueVec::from_lua_stack(self.state)?; Value::Variable(vals) - } else { + } else if ret_count == 1 { Value::from_lua_stack(self.state)? + } else { + Value::None }; R::from_lua(self.state, val) diff --git a/src/main.rs b/src/main.rs index 71c113a..2f390da 100755 --- a/src/main.rs +++ b/src/main.rs @@ -1,4 +1,6 @@ -use std::{cell::{Ref, RefCell}, marker::PhantomData, mem, sync::Arc}; +use std::{ + cell::{Ref, RefCell}, ffi::CStr, marker::PhantomData, mem, ops::Deref, sync::Arc +}; use mlua_sys as lua; @@ -35,43 +37,32 @@ fn main() -> Result<()> { let globals = lua.globals()?; - let a = |_lua: &State, (num,): (i32,)| -> Result { - println!("Rust got number from lua: {}", num); - Ok(999) - }; - - let f = lua.create_function(a)?; - globals.set("native_test", f)?; - - /* let ud = lua.create_userdata(UserdataProxy::::new())?; - globals.set("Vec2", ud)?; */ + let v1 = RefCell::new(Vec2 { x: 50.0, y: 5.0 }); + let ud = lua.create_userdata(UserdataRef::from(v1.borrow()))?; + globals.set("v1", ud)?; - let cell = RefCell::new(Vec2 { - x: 50.0, - y: 5.0 - }); + let v2 = Vec2 { x: 10.0, y: 15.0 }; + let ud = lua.create_userdata(UserdataRef::from(&v2))?; + globals.set("v2", ud)?; - { - let ud = lua.create_userdata(UserdataRef { - ud_ptr: unsafe { - mem::transmute::, Ref>(cell.borrow()) - }, - })?; - globals.set("vec2", ud)?; - } - - let chunk = lua.load("text.lua", r#" + //lua.gc_stop()?; + let chunk = lua.load( + "text.lua", + r#" require "util" - if vec2 ~= nil then - print("vec2: (" .. vec2.x .. ", " .. vec2.y .. ")") - end + --if vec2 ~= nil then + print("v1: (" .. v1.x .. ", " .. v1.y .. ")") + --print("v2: (" .. v2.x .. ", " .. v2.y .. ")") + --end --print("vec2.x = " .. vec2.x) --print("vec2.y = " .. vec2.y) - "#)?; + "#, + )?; - for _ in 0..2 { + const MAX_RUNS: i32 = 400; + for i in 0..MAX_RUNS { // I don't care about the result of this execution, so I set the result as a // Value which can be anything // @@ -83,23 +74,19 @@ fn main() -> Result<()> { if let Err(e) = res { panic!("{}", e); } + + //print_refs(&lua).unwrap(); + println!("i = {}", i); + globals.set("v1", Value::Nil)?; + lua.gc_collect()?; - let vec2 = globals.get::<_, AnyUserdata>("vec2")?; - vec2.garbage_collect()?; - globals.set("vec2", Value::Nil)?; - - let mut t = cell.borrow_mut(); + let mut t = v1.borrow_mut(); t.x += 50.0; t.y += 5.0; - drop(t); + drop(t); - let ud = lua.create_userdata(UserdataRef { - // TODO: avoid unsafe here - ud_ptr: unsafe { - mem::transmute::, Ref>(cell.borrow()) - }, - })?; - globals.raw_set("vec2", ud)?; + let ud = lua.create_userdata(UserdataRef::from(v1.borrow()))?; + globals.raw_set("v1", ud)?; } unsafe { @@ -109,21 +96,59 @@ fn main() -> Result<()> { Ok(()) } +/// Prints the types of the registry +pub fn print_refs(lua: &State) -> Result<()> { + unsafe { + let _g = StackGuard::new(&lua); + let s = lua.state_ptr(); + + Value::String(String::from("Hello World")).push_to_lua_stack(&lua)?; + let r = lua::luaL_ref(s, lua::LUA_REGISTRYINDEX); + + for i in 0..r { + let ty = lua::lua_rawgeti(s, lua::LUA_REGISTRYINDEX, i as i64); + + match ty { + lua::LUA_TUSERDATA => { + lua::lua_getmetatable(s, -1); + lua::lua_pushliteral(s, "__name"); + lua::lua_rawget(s, -2); + + let v = Value::from_lua_stack(&lua)?; + println!("{}: {}", i, v.as_string().unwrap()); + }, + _ => { + let tyname = CStr::from_ptr(lua::lua_typename(s, ty)); + let tyname = tyname.to_str().unwrap(); + println!("{}: {}", i, tyname); + } + } + + lua::lua_pop(s, 1); + } + lua::luaL_unref(s, lua::LUA_REGISTRYINDEX, r); + } + Ok(()) +} + /// A LuaRef is a a handle to something in Lua. -/// +/// /// These are created with `luaL_ref`, they can be created from anything on the top of the stack /// with [`LuaRef::from_stack`]. The references are automatically freed with `luaL_unref` when /// the inner Arc detects a single strong count. #[derive(Clone)] -pub struct LuaRef<'a>(Arc, &'a State); +pub struct LuaRef<'a> { + lref: Arc, + state: &'a State, +} impl<'a> Drop for LuaRef<'a> { fn drop(&mut self) { unsafe { - let s = self.1.state_ptr(); - - if Arc::strong_count(&self.0) == 1 { - lua::luaL_unref(s, lua::LUA_REGISTRYINDEX, *self.0); + let s = self.state.state_ptr(); + + if Arc::strong_count(&self.lref) == 1 { + lua::luaL_unref(s, lua::LUA_REGISTRYINDEX, *self.lref); } } } @@ -131,19 +156,21 @@ impl<'a> Drop for LuaRef<'a> { impl<'a> LuaRef<'a> { pub fn new(lua_ref: i32, state: &'a State) -> Self { - Self(Arc::new(lua_ref), state) + Self { + lref: Arc::new(lua_ref), + state, + } } /// Creates a reference to what is at the top of the stack. pub unsafe fn from_stack(state: &'a State) -> Result { let s = state.state_ptr(); - let r = lua::luaL_ref(s, lua::LUA_REGISTRYINDEX); if r == lua::LUA_REFNIL { Err(Error::Nil) } else { Ok(LuaRef::new(r, state)) - } + } } } @@ -153,7 +180,7 @@ impl<'a> PushToLuaStack<'a> for LuaRef<'a> { state.ensure_stack(1)?; let top = lua::lua_gettop(s); - let ty = lua::lua_rawgeti(s, lua::LUA_REGISTRYINDEX, *self.0 as i64); + let ty = lua::lua_rawgeti(s, lua::LUA_REGISTRYINDEX, *self.lref as i64); let new_top = lua::lua_gettop(s); if ty == lua::LUA_TNIL || ty == lua::LUA_TNONE || top == new_top { @@ -205,9 +232,10 @@ macro_rules! impl_as_lua_number { fn from_lua(_lua: &State, val: Value) -> crate::Result { match val { Value::Number(v) => Ok(v as $ty), - _ => { - Err(Error::UnexpectedType("Number".to_string(), val.type_name().to_string())) - }, + _ => Err(Error::UnexpectedType( + "Number".to_string(), + val.type_name().to_string(), + )), } } } @@ -218,7 +246,6 @@ macro_rules! impl_as_lua_number { v.push_to_lua_stack(state) } } - }; } @@ -260,7 +287,7 @@ impl<'a> PushToLuaStack<'a> for &str { let s = format!("{}\0", self); let cstr = s.as_ptr() as *const i8; lua::lua_pushstring(state.state_ptr(), cstr); - + Ok(()) } } @@ -292,14 +319,9 @@ impl Userdata for Vec2 { builder .field_getter("x", |_, this| Ok(this.x)) .field_getter("y", |_, this| Ok(this.y)) - .field_setter("x", |_, this, x: f32| this.x = x) .field_setter("y", |_, this, y: f32| this.y = y) - - .function("new", |lua, (x, y)| { - lua.create_userdata(Vec2 { x, y, }) - }) - + .function("new", |lua, (x, y)| lua.create_userdata(Vec2 { x, y })) // method test .method("add", |lua, lhs: &Vec2, (rhs,): (Ref,)| { let lx = lhs.x; @@ -307,18 +329,23 @@ impl Userdata for Vec2 { let rx = rhs.x; let ry = rhs.y; - - lua.create_userdata(Vec2 { x: lx + rx, y: ly + ry, }) - }) + lua.create_userdata(Vec2 { + x: lx + rx, + y: ly + ry, + }) + }) .meta_method(MetaMethod::Add, |lua, lhs: &Vec2, (rhs,): (Ref,)| { let lx = lhs.x; let ly = lhs.y; let rx = rhs.x; let ry = rhs.y; - - lua.create_userdata(Vec2 { x: lx + rx, y: ly + ry, }) + + lua.create_userdata(Vec2 { + x: lx + rx, + y: ly + ry, + }) }); Ok(()) @@ -360,8 +387,48 @@ impl Userdata for UserdataProxy { } } +pub enum Borrow<'a, T> { + Wrapped(Ref<'a, T>), + Raw(&'a T), +} + +impl<'a, T> Deref for Borrow<'a, T> { + type Target = T; + + fn deref(&self) -> &Self::Target { + match self { + Borrow::Wrapped(w) => w, + Borrow::Raw(w) => w, + } + } +} + pub struct UserdataRef<'a, T: Userdata> { - ud_ptr: Ref<'a, T> + ud_ptr: Borrow<'a, T>, +} + +impl<'a, T: Userdata> From<&'a T> for UserdataRef<'static, T> { + fn from(value: &'a T) -> Self { + let ud = Borrow::Raw(value); + + Self { + ud_ptr: unsafe { + mem::transmute(ud) + } + } + } +} + +impl<'a, T: Userdata> From> for UserdataRef<'static, T> { + fn from(value: Ref<'a, T>) -> Self { + let ud = Borrow::Wrapped(value); + + Self { + ud_ptr: unsafe { + mem::transmute(ud) + }, + } + } } impl<'a, T: Userdata + 'static> Userdata for UserdataRef<'a, T> { @@ -374,7 +441,7 @@ impl<'a, T: Userdata + 'static> Userdata for UserdataRef<'a, T> { let getter: fn(AnyUserdata<'_>) -> Result<*const ()> = move |ud: AnyUserdata| { let ud_ptr = { let ud = ud.as_ref::>()?; - + let ud_ptr: *const T = &*ud.ud_ptr; ud_ptr }; @@ -393,4 +460,4 @@ impl<'a, T: Userdata + 'static> Userdata for UserdataRef<'a, T> { let name = format!("{}Ref", T::name()); name } -} \ No newline at end of file +} diff --git a/src/state.rs b/src/state.rs index e689c23..8395f93 100755 --- a/src/state.rs +++ b/src/state.rs @@ -1,6 +1,7 @@ use core::ffi; use std::{alloc::{self, Layout}, any::TypeId, cell::RefCell, collections::HashMap, ffi::{CStr, CString}, mem, ptr::{self, NonNull}, str::Utf8Error, sync::Arc}; +use lua::lua_gc; use mlua_sys as lua; use crate::{lua_error_guard, AnyUserdata, AsLua, Chunk, Error, FromLua, FromLuaStack, FromLuaVec, Function, IntoChunkData, IntoLuaVec, LuaRef, MetaMethod, PushToLuaStack, Result, StackGuard, Table, Userdata, UserdataBuilder, Value, ValueVec}; @@ -88,8 +89,8 @@ struct ClosureData<'a> { } #[derive(Default)] -struct ExtraSpace<'a> { - userdata_metatables: HashMap>, +pub struct ExtraSpace<'a> { + pub userdata_metatables: HashMap>, } pub struct State { @@ -101,10 +102,17 @@ impl Drop for State { unsafe { let extra = self.get_extra_space_ptr(); + { + // clear the refs to anything in lua before we close it and + // attempt to drop extra after + let extra = &mut *extra; + extra.userdata_metatables.clear(); + } + lua::lua_close(self.lua.as_ptr()); extra.drop_in_place(); - // must be dealloced since it wasn't memory created from lua (like userdata) + // must be dealloced since it wasn't memory created from lua (i.e. userdata) alloc::dealloc(extra.cast(), Layout::new::()); } } @@ -152,7 +160,7 @@ impl State { } } - fn get_extra_space(&self) -> &mut ExtraSpace { + pub fn get_extra_space(&self) -> &mut ExtraSpace { unsafe { self.get_extra_space_ptr().as_mut() .expect("Somehow the Lua state's extra data got deleted!") @@ -496,23 +504,16 @@ impl State { // if a Gc metamethod was not manually defined, set it if !mt.raw_has_key(MetaMethod::Gc)? { - let gc_func = self.create_function(move |lua: &State, ud: AnyUserdata| { + let gc_func = self.create_function(|_lua, ud: AnyUserdata| { unsafe { - // if this panics, there's a weird bug. - let ud_ptr = ud.as_ptr_unchecked::().unwrap(); + let ud_ptr = ud.as_ptr_unchecked::()?; ud_ptr.drop_in_place(); - - lua::luaL_unref(lua.state_ptr(), lua::LUA_REGISTRYINDEX, *ud.unsafe_ud.lref.0); - - let extra = lua.get_extra_space(); - extra.userdata_metatables.remove(&TypeId::of::()); - //todo!() } Ok(()) })?; - - mt.set_meta(MetaMethod::Gc, gc_func)?; + + mt.set(MetaMethod::Gc, gc_func)?; } let extra = self.get_extra_space(); @@ -577,4 +578,35 @@ impl State { Ok(s) } } + + /// Returns a boolean indiciating if the Lua garbage collector is running. + pub fn is_gc_running(&self) -> Result { + unsafe { + let s = self.state_ptr(); + + Ok(lua_gc(s, lua::LUA_GCISRUNNING) != 0) + } + } + + /// Triggers a full garbage-collection cycle. + pub fn gc_collect(&self) -> Result<()> { + unsafe { + let s = self.state_ptr(); + + lua_gc(s, lua::LUA_GCCOLLECT); + } + + Ok(()) + } + + /// Disable the garbage collector + pub fn gc_stop(&self) -> Result<()> { + unsafe { + let s = self.state_ptr(); + + lua_gc(s, lua::LUA_GCSTOP); + } + + Ok(()) + } } \ No newline at end of file diff --git a/src/table.rs b/src/table.rs index ffd592d..a7f9de5 100755 --- a/src/table.rs +++ b/src/table.rs @@ -8,7 +8,7 @@ pub struct Table<'a> { state: &'a State, pub(crate) lref: LuaRef<'a>, /// a boolean indicating if this Table is a Metatable - mt_marker: bool, + pub(crate) mt_marker: bool, } impl<'a> Table<'a> { @@ -126,7 +126,7 @@ impl<'a> Table<'a> { val.push_to_lua_stack(self.state)?; lua::lua_gettable(s, -2); // table[key] is at top of stack - + let val = Value::from_lua_stack(self.state)?; let val = V::from_lua(self.state, val)?; diff --git a/src/userdata/mod.rs b/src/userdata/mod.rs index 30bbbcb..1cccaca 100755 --- a/src/userdata/mod.rs +++ b/src/userdata/mod.rs @@ -388,18 +388,22 @@ impl<'a> AnyUserdata<'a> { } /// Returns the metatable of this userdata - pub fn get_metatable(&self) -> crate::Result> { + pub fn get_metatable(&'a self) -> crate::Result> { unsafe { self.state.ensure_stack(2)?; let _g = StackGuard::new(self.state); - self.unsafe_ud.lref.push_to_lua_stack(self.state)?; + //self.unsafe_ud.lref.push_to_lua_stack(self.state)?; + self.push_to_lua_stack(self.state)?; let s = self.state.state_ptr(); if lua::lua_getmetatable(s, -1) == 0 { Err(crate::Error::MissingMetatable) } else { - Ok(Table::from_lua_stack(self.state)?) + let mut t = Table::from_lua_stack(self.state)?; + t.mt_marker = true; + + Ok(t) } } } @@ -429,26 +433,20 @@ impl<'a> AnyUserdata<'a> { /// Execute a method on this userdata. This finds a function with `name` and executes it /// with the userdata as the first argument. - pub fn execute_method(&self, name: &str, args: A) -> crate::Result + pub fn execute_method(&'a self, name: &str, args: A) -> crate::Result where A: IntoLuaVec<'a>, R: FromLua<'a>, { - //let s: &Self = unsafe { mem::transmute(self) }; let name = name.to_string(); let mt = self.get_metatable()?; let f = mt.get::<_, Function>(name)?; - + let mut args = args.into_lua_value_vec(self.state)?; args.push_front(self.clone().as_lua(self.state)?); f.exec(args) } - - /// Trigger garbage collection on the userdata. - pub fn garbage_collect(self) -> crate::Result<()> { - self.execute_method("__gc", ()).into() - } } impl<'a> FromLuaStack<'a> for AnyUserdata<'a> {