Skip to content

Commit 2f489ee

Browse files
khvzakkhvzak
committed
Prevent userdata destructor panics from corrupting Lua state
Resume explicit destruction panics after Lua restores its call frames, and abort on GC destructor panics.
1 parent e9be788 commit 2f489ee

3 files changed

Lines changed: 108 additions & 8 deletions

File tree

‎src/userdata.rs‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -816,6 +816,9 @@ impl AnyUserData {
816816
/// This is similar to [`AnyUserData::take`], but it doesn't require a type.
817817
///
818818
/// This method works for non-scoped userdata only.
819+
///
820+
/// Panics from the value's destructor propagate to the caller. During garbage collection,
821+
/// destructor panics abort the process instead.
819822
pub fn destroy(&self) -> Result<()> {
820823
let lua = self.0.lua.lock();
821824
let state = lua.state();
@@ -825,7 +828,12 @@ impl AnyUserData {
825828

826829
lua.push_userdata_ref(&self.0)?;
827830
protect_lua!(state, 1, 1, fn(state) {
828-
if ffi::luaL_callmeta(state, -1, cstr!("__gc")) == 0 {
831+
if ffi::luaL_getmetafield(state, 1, cstr!("__gc")) != ffi::LUA_TNIL {
832+
ffi::lua_pushvalue(state, 1);
833+
// Only explicit destruction may propagate panics
834+
ffi::lua_pushboolean(state, 1);
835+
ffi::lua_call(state, 2, 1);
836+
} else {
829837
ffi::lua_pushboolean(state, 0);
830838
}
831839
})?;

‎src/userdata/util.rs‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use std::any::TypeId;
22
use std::os::raw::c_int;
3+
use std::panic::{AssertUnwindSafe, catch_unwind};
34
use std::ptr;
45

56
use rustc_hash::FxHashMap;
@@ -443,7 +444,8 @@ unsafe fn push_userdata_metatable_namecall(
443444
#[cfg(not(feature = "luau"))]
444445
pub(crate) unsafe extern "C-unwind" fn collect_userdata<T>(state: *mut ffi::lua_State) -> c_int {
445446
let ud = get_userdata::<T>(state, -1);
446-
ptr::drop_in_place(ud);
447+
// A GC finalizer must neither unwind through Lua nor raise a Lua error
448+
catch_unwind(AssertUnwindSafe(|| ptr::drop_in_place(ud))).unwrap_or_else(|_| std::process::abort());
447449
0
448450
}
449451

@@ -473,14 +475,20 @@ pub(crate) unsafe extern "C" fn collect_userdata<T>(
473475
// It checks if the userdata is safe to destroy and sets the "destroyed" metatable
474476
// to prevent further GC collection.
475477
pub(super) unsafe extern "C-unwind" fn destroy_userdata_storage<T>(state: *mut ffi::lua_State) -> c_int {
476-
let ud = get_userdata::<UserDataStorage<T>>(state, 1);
477-
if (*ud).is_safe_to_destroy() {
478-
take_userdata::<UserDataStorage<T>>(state, 1);
479-
ffi::lua_pushboolean(state, 1);
478+
let destroy = |index| {
479+
let ud = get_userdata::<UserDataStorage<T>>(state, index);
480+
let safe_to_destroy = (*ud).is_safe_to_destroy();
481+
if safe_to_destroy {
482+
drop(take_userdata::<UserDataStorage<T>>(state, index));
483+
}
484+
ffi::lua_pushboolean(state, safe_to_destroy as c_int);
485+
1
486+
};
487+
if ffi::lua_toboolean(state, 2) != 0 {
488+
crate::state::callback_error_ext(state, ptr::null_mut(), false, |_, nargs| Ok(destroy(-nargs)))
480489
} else {
481-
ffi::lua_pushboolean(state, 0);
490+
catch_unwind(AssertUnwindSafe(|| destroy(1))).unwrap_or_else(|_| std::process::abort())
482491
}
483-
1
484492
}
485493

486494
static USERDATA_METATABLE_INDEX: u8 = 0;

‎tests/userdata.rs‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -446,6 +446,90 @@ fn test_userdata_destroy() -> Result<()> {
446446
Ok(())
447447
}
448448

449+
#[test]
450+
#[cfg(panic = "unwind")]
451+
fn test_userdata_destroy_panic() -> Result<()> {
452+
use std::panic::{AssertUnwindSafe, catch_unwind};
453+
454+
struct Panicking;
455+
impl Drop for Panicking {
456+
fn drop(&mut self) {
457+
panic!("userdata drop");
458+
}
459+
}
460+
461+
let lua = Lua::new();
462+
// Repeat beyond Lua's C-call limit
463+
for _ in 0..256 {
464+
let ud = lua.create_any_userdata(Panicking)?;
465+
let panic = catch_unwind(AssertUnwindSafe(|| ud.destroy())).unwrap_err();
466+
assert_eq!(panic.downcast_ref::<&str>(), Some(&"userdata drop"));
467+
assert!(matches!(ud.destroy(), Err(Error::UserDataDestructed)));
468+
assert!(lua.inspect_stack(0, |_| ()).is_none());
469+
assert_eq!(lua.load("return 42").eval::<i32>()?, 42);
470+
}
471+
lua.gc_collect()?;
472+
473+
Ok(())
474+
}
475+
476+
#[cfg(not(target_family = "wasm"))]
477+
#[test]
478+
fn test_userdata_gc_panic() -> Result<()> {
479+
use std::panic::{AssertUnwindSafe, catch_unwind};
480+
481+
struct Panicking;
482+
impl Drop for Panicking {
483+
fn drop(&mut self) {
484+
panic!("userdata drop");
485+
}
486+
}
487+
488+
if let Ok(mode) = std::env::var("MLUA_TEST_GC_PANIC") {
489+
let _ = catch_unwind(AssertUnwindSafe(|| -> Result<()> {
490+
let lua = Lua::new();
491+
lua.gc_stop();
492+
if mode.starts_with("callback") {
493+
let value = Panicking;
494+
lua.create_function(move |_, ()| {
495+
let _ = &value;
496+
Ok(())
497+
})?;
498+
} else {
499+
lua.create_any_userdata(Panicking)?;
500+
}
501+
if mode.ends_with("collect") {
502+
lua.gc_collect()?;
503+
lua.gc_collect()?;
504+
// Collection must abort before reaching here
505+
std::process::exit(0);
506+
}
507+
drop(lua);
508+
Ok(())
509+
}));
510+
return Ok(());
511+
}
512+
513+
for mode in [
514+
"userdata_collect",
515+
"userdata_close",
516+
"callback_collect",
517+
"callback_close",
518+
] {
519+
let output = std::process::Command::new(std::env::current_exe()?)
520+
.args(["--exact", "test_userdata_gc_panic"])
521+
.env("MLUA_TEST_GC_PANIC", mode)
522+
.output()?;
523+
assert!(!output.status.success(), "{mode}");
524+
#[cfg(unix)]
525+
{
526+
use std::os::unix::process::ExitStatusExt;
527+
assert_eq!(output.status.signal(), Some(libc::SIGABRT), "{mode}");
528+
}
529+
}
530+
Ok(())
531+
}
532+
449533
#[test]
450534
fn test_userdata_method_once() -> Result<()> {
451535
struct MyUserdata(Arc<i64>);

0 commit comments

Comments
 (0)