From e41c72d64259ade8e66e45a65e1af22694b643ff Mon Sep 17 00:00:00 2001 From: kyren Date: Sun, 3 Dec 2017 18:15:31 -0500 Subject: [PATCH] more refactoring trying to find a workable path to 'm' error safety --- src/lua.rs | 79 ++++++++++++++++++++++++------------------- src/util.rs | 96 ++++++++++++++++++++++++++--------------------------- 2 files changed, 92 insertions(+), 83 deletions(-) diff --git a/src/lua.rs b/src/lua.rs index b9cdfe9..f11de04 100644 --- a/src/lua.rs +++ b/src/lua.rs @@ -205,21 +205,23 @@ impl<'lua> Function<'lua> { let nargs = args.len() as c_int; check_stack(lua.state, nargs + 3); + ffi::lua_pushcfunction(lua.state, error_traceback); let stack_start = ffi::lua_gettop(lua.state); lua.push_ref(lua.state, &self.0); for arg in args { lua.push_value(lua.state, arg); } - handle_error( - lua.state, - pcall_with_traceback(lua.state, nargs, ffi::LUA_MULTRET), - )?; + let ret = ffi::lua_pcall(lua.state, nargs, ffi::LUA_MULTRET, stack_start); + if ret != ffi::LUA_OK { + return Err(pop_error(lua.state, ret)); + } let nresults = ffi::lua_gettop(lua.state) - stack_start; let mut results = MultiValue::new(); check_stack(lua.state, 1); for _ in 0..nresults { results.push_front(lua.pop_value(lua.state)); } + ffi::lua_pop(lua.state, 1); R::from_lua_multi(results, lua) }) } @@ -391,10 +393,11 @@ impl<'lua> Thread<'lua> { lua.push_value(thread_state, arg); } - handle_error( - thread_state, - resume_with_traceback(thread_state, lua.state, nargs), - )?; + let ret = ffi::lua_resume(thread_state, lua.state, nargs); + if ret != ffi::LUA_OK && ret != ffi::LUA_YIELD { + error_traceback(thread_state); + return Err(pop_error(thread_state, ret)); + } let nresults = ffi::lua_gettop(thread_state); let mut results = MultiValue::new(); @@ -442,6 +445,14 @@ impl Drop for Lua { fn drop(&mut self) { unsafe { if !self.ephemeral { + if cfg!(test) { + let top = ffi::lua_gettop(self.state); + if top != 0 { + eprintln!("Lua stack leak detected, stack top is {}", top); + ::std::process::abort() + } + } + ffi::lua_close(self.state); } } @@ -578,33 +589,31 @@ impl Lua { stack_err_guard(self.state, 0, || { check_stack(self.state, 1); - handle_error( - self.state, - if let Some(name) = name { - let name = CString::new(name.to_owned()).map_err(|e| { - Error::ToLuaConversionError { - from: "&str", - to: "string", - message: Some(e.to_string()), - } - })?; - ffi::luaL_loadbuffer( - self.state, - source.as_ptr() as *const c_char, - source.len(), - name.as_ptr(), - ) - } else { - ffi::luaL_loadbuffer( - self.state, - source.as_ptr() as *const c_char, - source.len(), - ptr::null(), - ) - }, - )?; - - Ok(Function(self.pop_ref(self.state))) + match if let Some(name) = name { + let name = CString::new(name.to_owned()).map_err(|e| { + Error::ToLuaConversionError { + from: "&str", + to: "string", + message: Some(e.to_string()), + } + })?; + ffi::luaL_loadbuffer( + self.state, + source.as_ptr() as *const c_char, + source.len(), + name.as_ptr(), + ) + } else { + ffi::luaL_loadbuffer( + self.state, + source.as_ptr() as *const c_char, + source.len(), + ptr::null(), + ) + } { + ffi::LUA_OK => Ok(Function(self.pop_ref(self.state))), + err => Err(pop_error(self.state, err)), + } }) } } diff --git a/src/util.rs b/src/util.rs index 5f2dc84..335ebac 100644 --- a/src/util.rs +++ b/src/util.rs @@ -107,8 +107,10 @@ pub unsafe fn pgettable(state: *mut ffi::lua_State, index: c_int) -> Result Ok(ffi::lua_type(state, -1)), + err => Err(pop_error(state, err)), + } } // Protected version of lua_settable, uses 4 stack spaces, does not call checkstack. @@ -127,8 +129,10 @@ pub unsafe fn psettable(state: *mut ffi::lua_State, index: c_int) -> Result<()> ffi::lua_remove(state, -5); ffi::lua_remove(state, -5); - handle_error(state, pcall_with_traceback(state, 3, 0))?; - Ok(()) + match pcall_with_traceback(state, 3, 0) { + ffi::LUA_OK => Ok(()), + err => Err(pop_error(state, err)), + } } // Protected version of luaL_len, uses 2 stack spaces, does not call checkstack. @@ -143,10 +147,14 @@ pub unsafe fn plen(state: *mut ffi::lua_State, index: c_int) -> Result { + let len = ffi::lua_tointeger(state, -1); + ffi::lua_pop(state, 1); + Ok(len) + } + err => Err(pop_error(state, err)), + } } // Protected version of lua_geti, uses 3 stack spaces, does not call checkstack. @@ -167,8 +175,10 @@ pub unsafe fn pgeti( ffi::lua_pushvalue(state, table_index); ffi::lua_pushinteger(state, i); - handle_error(state, pcall_with_traceback(state, 2, 1))?; - Ok(ffi::lua_type(state, -1)) + match pcall_with_traceback(state, 2, 1) { + ffi::LUA_OK => Ok(ffi::lua_type(state, -1)), + err => Err(pop_error(state, err)), + } } // Protected version of lua_next, uses 3 stack spaces, does not call checkstack. @@ -189,25 +199,31 @@ pub unsafe fn pnext(state: *mut ffi::lua_State, index: c_int) -> Result { ffi::lua_remove(state, -4); let stack_start = ffi::lua_gettop(state) - 3; - handle_error(state, pcall_with_traceback(state, 2, ffi::LUA_MULTRET))?; - let nresults = ffi::lua_gettop(state) - stack_start; - if nresults == 0 { - Ok(0) - } else { - Ok(1) + match pcall_with_traceback(state, 2, ffi::LUA_MULTRET) { + ffi::LUA_OK => { + let nresults = ffi::lua_gettop(state) - stack_start; + if nresults == 0 { + Ok(0) + } else { + Ok(1) + } + } + err => Err(pop_error(state, err)), } } -// If the return code indicates an error, pops the error off of the stack and -// returns Err. If the error is actually a WrappedPanic, clears 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 handle_error(state: *mut ffi::lua_State, err: c_int) -> Result<()> { - if err == ffi::LUA_OK || err == ffi::LUA_YIELD { - Ok(()) - } else if let Some(err) = pop_wrapped_error(state) { - Err(err) +// Pops an error off of the stack and returns it. If the error is actually a WrappedPanic, clears +// 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!( + state, + err_code != ffi::LUA_OK && err_code != ffi::LUA_YIELD, + "pop_error called with non-error return code" + ); + + if let Some(err) = pop_wrapped_error(state) { + err } else if is_wrapped_panic(state, -1) { let panic = get_userdata::(state, -1); if let Some(p) = (*panic).0.take() { @@ -227,7 +243,7 @@ pub unsafe fn handle_error(state: *mut ffi::lua_State, err: c_int) -> Result<()> }; ffi::lua_pop(state, 1); - Err(match err { + match err_code { ffi::LUA_ERRRUN => Error::RuntimeError(err_string), ffi::LUA_ERRSYNTAX => { Error::SyntaxError { @@ -258,7 +274,7 @@ pub unsafe fn handle_error(state: *mut ffi::lua_State, err: c_int) -> Result<()> process::abort() } _ => lua_panic!(state, "internal error: unrecognized lua error code"), - }) + } } } @@ -293,7 +309,7 @@ pub unsafe extern "C" fn userdata_destructor(state: *mut ffi::lua_State) -> c // 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 handle_error back on +// longjmp). The error or panic is wrapped in such a way that when calling pop_error back on // the rust side, it will resume the panic. pub unsafe fn callback_error(state: *mut ffi::lua_State, f: F) -> R where @@ -313,8 +329,8 @@ where } // Takes an error at the top of the stack, and if it is a WrappedError, converts it to an -// Error::CallbackError with a traceback. If it is a lua error, creates a new string error with a -// printed traceback, and if it is a WrappedPanic, does not modify it. +// Error::CallbackError with a traceback, if it is some lua type, prints the error along with a +// traceback, and if it is a WrappedPanic, does not modify it. pub unsafe extern "C" fn error_traceback(state: *mut ffi::lua_State) -> c_int { if let Some(error) = pop_wrapped_error(state) { ffi::luaL_traceback(state, state, ptr::null(), 0); @@ -345,11 +361,7 @@ pub unsafe extern "C" fn error_traceback(state: *mut ffi::lua_State) -> c_int { // ffi::lua_pcall with a message handler that gives a nice traceback. If the // caught error is actually a Error, will simply pass the error along. Does // not call checkstack, and uses 2 extra stack spaces. -pub unsafe fn pcall_with_traceback( - state: *mut ffi::lua_State, - nargs: c_int, - nresults: c_int, -) -> c_int { +unsafe fn pcall_with_traceback(state: *mut ffi::lua_State, nargs: c_int, nresults: c_int) -> c_int { let msgh_position = ffi::lua_gettop(state) - nargs; ffi::lua_pushcfunction(state, error_traceback); ffi::lua_insert(state, msgh_position); @@ -358,18 +370,6 @@ pub unsafe fn pcall_with_traceback( ret } -pub unsafe fn resume_with_traceback( - state: *mut ffi::lua_State, - from: *mut ffi::lua_State, - nargs: c_int, -) -> c_int { - let res = ffi::lua_resume(state, from, nargs); - if res != ffi::LUA_OK && res != ffi::LUA_YIELD { - error_traceback(state); - } - res -} - // A variant of pcall that does not allow lua to catch panic errors from callback_error pub unsafe extern "C" fn safe_pcall(state: *mut ffi::lua_State) -> c_int { let top = ffi::lua_gettop(state);