Skip to content

Commit

Permalink
Explicit error type for destructed callbacks
Browse files Browse the repository at this point in the history 
Also removes some cleverness if debug_assertions was disabled, as it really
doesn't make much of a performance difference.
  • Loading branch information
@kyren
kyren committed Feb 10, 2018
1 parent 514abd5 commit fe6e4bd
Show file tree
Hide file tree
Showing 4 changed files with 82 additions and 75 deletions.
14 changes: 12 additions & 2 deletions src/error.rs
View file
Expand Up @@ -30,7 +30,13 @@ pub enum Error {
///
/// This is an error because `rlua` callbacks are FnMut and thus can only be mutably borrowed
/// once.
RecursiveCallbackError,
RecursiveCallback,
/// Either a callback or a userdata method has been called, but the callback or userdata has
/// been destructed.
///
/// This can happen either due to to being destructed in a previous __gc, or due to being
/// destructed from exiting a `Lua::scope` call.
CallbackDestructed,
/// A Rust value could not be converted to a Lua value.
ToLuaConversionError {
/// Name of the Rust type that could not be converted.
Expand Down Expand Up @@ -117,7 +123,11 @@ impl fmt::Display for Error {
Error::GarbageCollectorError(ref msg) => {
write!(fmt, "garbage collector error: {}", msg)
}
Error::RecursiveCallbackError => write!(fmt, "callback called recursively"),
Error::RecursiveCallback => write!(fmt, "callback called recursively"),
Error::CallbackDestructed => write!(
fmt,
"a destructed callback or destructed userdata method was called"
),
Error::ToLuaConversionError {
from,
to,
Expand Down
11 changes: 5 additions & 6 deletions src/lua.rs
View file
Expand Up @@ -966,22 +966,21 @@ impl Lua {
func: Callback<'callback>,
) -> Result<Function<'lua>> {
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,
main_state: main_state(state),
ephemeral: true,
};

if ffi::lua_type(state, ffi::lua_upvalueindex(1)) == ffi::LUA_TNIL {
return Err(Error::CallbackDestructed);
}

let func = get_userdata::<RefCell<Callback>>(state, ffi::lua_upvalueindex(1));
let mut func = (*func)
.try_borrow_mut()
.map_err(|_| Error::RecursiveCallbackError)?;
.map_err(|_| Error::RecursiveCallback)?;

let nargs = ffi::lua_gettop(state);
let mut args = MultiValue::new();
Expand Down
31 changes: 21 additions & 10 deletions src/tests.rs
View file
Expand Up @@ -5,7 +5,8 @@ use std::cell::Cell;
use std::sync::Arc;
use std::panic::catch_unwind;

use {Error, ExternalError, Function, Lua, Nil, Result, Table, UserData, Value, Variadic};
use {Error, ExternalError, Function, Lua, Nil, Result, Table, UserData, UserDataMethods, Value,
Variadic};

#[test]
fn test_load() {
Expand Down Expand Up @@ -458,7 +459,7 @@ fn test_recursive_callback_error() {
{
Err(Error::CallbackError { ref cause, .. }) => match *cause.as_ref() {
Error::CallbackError { ref cause, .. } => match *cause.as_ref() {
Error::RecursiveCallbackError { .. } => {}
Error::RecursiveCallback { .. } => {}
ref other => panic!("incorrect result: {:?}", other),
},
ref other => panic!("incorrect result: {:?}", other),
Expand Down Expand Up @@ -619,19 +620,24 @@ fn scope_func() {
assert_eq!(rc.get(), 42);
assert_eq!(Rc::strong_count(&rc), 1);

assert!(
lua.globals()
.get::<_, Function>("bad")
.unwrap()
.call::<_, ()>(())
.is_err()
);
match lua.globals()
.get::<_, Function>("bad")
.unwrap()
.call::<_, ()>(())
{
Err(Error::CallbackError { .. }) => {}
r => panic!("improper return for destructed function: {:?}", r),
};
}

#[test]
fn scope_drop() {
struct MyUserdata(Rc<()>);
impl UserData for MyUserdata {}
impl UserData for MyUserdata {
fn add_methods(methods: &mut UserDataMethods<Self>) {
methods.add_method("method", |_, _, ()| Ok(()));
}
}

let rc = Rc::new(());

Expand All @@ -646,6 +652,11 @@ fn scope_drop() {
assert_eq!(Rc::strong_count(&rc), 2);
});
assert_eq!(Rc::strong_count(&rc), 1);

match lua.exec::<()>("test:method()", None) {
Err(Error::CallbackError { .. }) => {}
r => panic!("improper return for destructed userdata: {:?}", r),
};
}

#[test]
Expand Down
101 changes: 44 additions & 57 deletions src/util.rs
View file
Expand Up @@ -24,29 +24,25 @@ pub unsafe fn stack_guard<F, R>(state: *mut ffi::lua_State, change: c_int, op: F
where
F: FnOnce() -> R,
{
if cfg!(debug_assertions) {
let expected = ffi::lua_gettop(state) + change;
lua_internal_assert!(
state,
expected >= 0,
"too many stack values would be popped"
);
let expected = ffi::lua_gettop(state) + change;
lua_internal_assert!(
state,
expected >= 0,
"too many stack values would be popped"
);

let res = op();
let res = op();

let top = ffi::lua_gettop(state);
lua_internal_assert!(
state,
ffi::lua_gettop(state) == expected,
"expected stack to be {}, got {}",
expected,
top
);
let top = ffi::lua_gettop(state);
lua_internal_assert!(
state,
ffi::lua_gettop(state) == expected,
"expected stack to be {}, got {}",
expected,
top
);

res
} else {
op()
}
res
}

// Run an operation on a lua_State and automatically clean up the stack before
Expand All @@ -62,45 +58,36 @@ pub unsafe fn stack_err_guard<F, R>(state: *mut ffi::lua_State, change: c_int, o
where
F: FnOnce() -> Result<R>,
{
if cfg!(debug_assertions) {
let expected = ffi::lua_gettop(state) + change;
let expected = ffi::lua_gettop(state) + change;
lua_internal_assert!(
state,
expected >= 0,
"too many stack values would be popped"
);

let res = op();

let top = ffi::lua_gettop(state);
if res.is_ok() {
lua_internal_assert!(
state,
expected >= 0,
"too many stack values would be popped"
ffi::lua_gettop(state) == expected,
"expected stack to be {}, got {}",
expected,
top
);

let res = op();

let top = ffi::lua_gettop(state);
if res.is_ok() {
lua_internal_assert!(
state,
ffi::lua_gettop(state) == expected,
"expected stack to be {}, got {}",
expected,
top
);
} else {
lua_internal_assert!(
state,
top >= expected,
"{} too many stack values popped",
top - expected
);
if top > expected {
ffi::lua_settop(state, expected);
}
}
res
} else {
let prev = ffi::lua_gettop(state) + change;
let res = op();
if res.is_err() {
ffi::lua_settop(state, prev);
lua_internal_assert!(
state,
top >= expected,
"{} too many stack values popped",
top - expected
);
if top > expected {
ffi::lua_settop(state, expected);
}
res
}
res
}

// Call a function that calls into the Lua API and may trigger a Lua error (longjmp) in a safe way.
Expand Down Expand Up @@ -253,7 +240,6 @@ pub unsafe fn push_userdata<T>(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<T>(state: *mut ffi::lua_State, index: c_int) -> *mut T {
let ud = ffi::lua_touserdata(state, index) as *mut T;
lua_internal_assert!(state, !ud.is_null(), "userdata pointer is null");
Expand All @@ -269,9 +255,10 @@ pub unsafe fn take_userdata<T>(state: *mut ffi::lua_State) -> T {
// 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);
let ud = ffi::lua_touserdata(state, -1) as *mut T;
lua_internal_assert!(state, !ud.is_null(), "userdata pointer is null");
ffi::lua_pop(state, 1);
mem::replace(ud, mem::uninitialized())
ptr::read(ud)
}

pub unsafe extern "C" fn userdata_destructor<T>(state: *mut ffi::lua_State) -> c_int {
Expand Down Expand Up @@ -611,7 +598,7 @@ unsafe fn get_destructed_userdata_metatable(state: *mut ffi::lua_State) -> c_int
static DESTRUCTED_USERDATA_METATABLE: u8 = 0;

unsafe extern "C" fn destructed_error(state: *mut ffi::lua_State) -> c_int {
ffi::lua_pushstring(state, cstr!("userdata has been destructed"));
push_wrapped_error(state, Error::CallbackDestructed);
ffi::lua_error(state)
}

Expand Down

0 comments on commit fe6e4bd

Please sign in to comment.