Improve panic handling (check for twice resumed panics)

Author: khvzak

src/error.rs

@@ -139,6 +139,11 @@ pub enum Error {
         /// Original error returned by the Rust code.
         cause: Arc<Error>,
     },
+    /// A Rust panic that was previosly resumed, returned again.
+    ///
+    /// This error can occur only when a Rust panic resumed previously was recovered
+    /// and returned again.
+    PreviouslyResumedPanic,
     /// Serialization error.
     #[cfg(feature = "serialize")]
     #[cfg_attr(docsrs, doc(cfg(feature = "serialize")))]
@@ -230,6 +235,9 @@ impl fmt::Display for Error {
             Error::CallbackError { ref traceback, .. } => {
                 write!(fmt, "callback error: {}", traceback)
             }
+            Error::PreviouslyResumedPanic => {
+                write!(fmt, "previously resumed panic returned again")
+            }
             #[cfg(feature = "serialize")]
             Error::SerializeError(ref err) => {
                 write!(fmt, "serialize error: {}", err)

src/lua.rs

@@ -4,6 +4,7 @@ use std::collections::HashMap;
 use std::ffi::CString;
 use std::marker::PhantomData;
 use std::os::raw::{c_char, c_int, c_void};
+use std::panic::resume_unwind;
 use std::sync::{Arc, Mutex, Weak};
 use std::{mem, ptr, str};
 
@@ -25,7 +26,7 @@ use crate::util::{
     assert_stack, callback_error, check_stack, get_gc_userdata, get_main_state, get_userdata,
     get_wrapped_error, init_error_registry, init_gc_metatable_for, init_userdata_metatable,
     pop_error, protect_lua, protect_lua_closure, push_gc_userdata, push_meta_gc_userdata,
-    push_string, push_userdata, push_wrapped_error, StackGuard,
+    push_string, push_userdata, push_wrapped_error, StackGuard, WrappedPanic,
 };
 use crate::value::{FromLua, FromLuaMulti, MultiValue, Nil, ToLua, ToLuaMulti, Value};
 
@@ -1402,32 +1403,33 @@ impl Lua {
 
     // Uses 2 stack spaces, does not call checkstack
     pub(crate) unsafe fn pop_value(&self) -> Value {
-        match ffi::lua_type(self.state, -1) {
+        let state = self.state;
+        match ffi::lua_type(state, -1) {
             ffi::LUA_TNIL => {
-                ffi::lua_pop(self.state, 1);
+                ffi::lua_pop(state, 1);
                 Nil
             }
 
             ffi::LUA_TBOOLEAN => {
-                let b = Value::Boolean(ffi::lua_toboolean(self.state, -1) != 0);
-                ffi::lua_pop(self.state, 1);
+                let b = Value::Boolean(ffi::lua_toboolean(state, -1) != 0);
+                ffi::lua_pop(state, 1);
                 b
             }
 
             ffi::LUA_TLIGHTUSERDATA => {
-                let ud = Value::LightUserData(LightUserData(ffi::lua_touserdata(self.state, -1)));
-                ffi::lua_pop(self.state, 1);
+                let ud = Value::LightUserData(LightUserData(ffi::lua_touserdata(state, -1)));
+                ffi::lua_pop(state, 1);
                 ud
             }
 
             ffi::LUA_TNUMBER => {
-                if ffi::lua_isinteger(self.state, -1) != 0 {
-                    let i = Value::Integer(ffi::lua_tointeger(self.state, -1));
-                    ffi::lua_pop(self.state, 1);
+                if ffi::lua_isinteger(state, -1) != 0 {
+                    let i = Value::Integer(ffi::lua_tointeger(state, -1));
+                    ffi::lua_pop(state, 1);
                     i
                 } else {
-                    let n = Value::Number(ffi::lua_tonumber(self.state, -1));
-                    ffi::lua_pop(self.state, 1);
+                    let n = Value::Number(ffi::lua_tonumber(state, -1));
+                    ffi::lua_pop(state, 1);
                     n
                 }
             }
@@ -1439,12 +1441,20 @@ impl Lua {
             ffi::LUA_TFUNCTION => Value::Function(Function(self.pop_ref())),
 
             ffi::LUA_TUSERDATA => {
-                // It should not be possible to interact with userdata types other than custom
-                // UserData types OR a WrappedError.  WrappedPanic should not be here.
-                if let Some(err) = get_wrapped_error(self.state, -1).as_ref() {
+                // We must prevent interaction with userdata types other than UserData OR a WrappedError.
+                // WrappedPanics are automatically resumed.
+                if let Some(err) = get_wrapped_error(state, -1).as_ref() {
                     let err = err.clone();
-                    ffi::lua_pop(self.state, 1);
+                    ffi::lua_pop(state, 1);
                     Value::Error(err)
+                } else if let Some(panic) = get_gc_userdata::<WrappedPanic>(state, -1).as_mut() {
+                    if let Some(panic) = (*panic).0.take() {
+                        ffi::lua_pop(state, 1);
+                        resume_unwind(panic);
+                    }
+                    // Previously resumed panic?
+                    ffi::lua_pop(state, 1);
+                    Nil
                 } else {
                     Value::UserData(AnyUserData(self.pop_ref()))
                 }

src/util.rs

@@ -186,7 +186,7 @@ pub unsafe fn pop_error(state: *mut ffi::lua_State, err_code: c_int) -> Error {
         if let Some(p) = (*panic).0.take() {
             resume_unwind(p);
         } else {
-            mlua_panic!("error during panic handling, panic was resumed twice")
+            Error::PreviouslyResumedPanic
         }
     } else {
         let err_string = to_string(state, -1).into_owned();
@@ -587,27 +587,22 @@ pub unsafe fn init_error_registry(state: *mut ffi::lua_State) {
                 Ok(err_buf)
             } else if let Some(panic) = get_gc_userdata::<WrappedPanic>(state, -1).as_ref() {
                 if let Some(ref p) = (*panic).0 {
-                    ffi::lua_pushlightuserdata(
-                        state,
-                        &ERROR_PRINT_BUFFER_KEY as *const u8 as *mut c_void,
-                    );
-                    ffi::lua_rawget(state, ffi::LUA_REGISTRYINDEX);
+                    let err_buf_key = &ERROR_PRINT_BUFFER_KEY as *const u8 as *const c_void;
+                    ffi::lua_rawgetp(state, ffi::LUA_REGISTRYINDEX, err_buf_key);
                     let err_buf = ffi::lua_touserdata(state, -1) as *mut String;
+                    (*err_buf).clear();
                     ffi::lua_pop(state, 2);
 
-                    let error = if let Some(x) = p.downcast_ref::<&str>() {
-                        x.to_string()
-                    } else if let Some(x) = p.downcast_ref::<String>() {
-                        x.to_string()
+                    if let Some(msg) = p.downcast_ref::<&str>() {
+                        let _ = write!(&mut (*err_buf), "{}", msg);
+                    } else if let Some(msg) = p.downcast_ref::<String>() {
+                        let _ = write!(&mut (*err_buf), "{}", msg);
                     } else {
-                        "panic".to_string()
+                        let _ = write!(&mut (*err_buf), "<panic>");
                     };
-
-                    (*err_buf).clear();
-                    let _ = write!(&mut (*err_buf), "{}", error);
                     Ok(err_buf)
                 } else {
-                    mlua_panic!("error during panic handling, panic was resumed")
+                    Err(Error::PreviouslyResumedPanic)
                 }
             } else {
                 // I'm not sure whether this is possible to trigger without bugs in mlua?
@@ -721,7 +716,7 @@ pub unsafe fn init_error_registry(state: *mut ffi::lua_State) {
 }
 
 struct WrappedError(pub Error);
-struct WrappedPanic(pub Option<Box<dyn Any + Send + 'static>>);
+pub(crate) struct WrappedPanic(pub Option<Box<dyn Any + Send + 'static>>);
 
 // Converts the given lua value to a string in a reasonable format without causing a Lua error or
 // panicking.

tests/tests.rs

@@ -11,7 +11,7 @@
 extern "system" {}
 
 use std::iter::FromIterator;
-use std::panic::catch_unwind;
+use std::panic::{catch_unwind, AssertUnwindSafe};
 use std::sync::Arc;
 use std::{error, f32, f64, fmt};
 
@@ -384,62 +384,92 @@ fn test_error() -> Result<()> {
 
     assert!(understand_recursion.call::<_, ()>(()).is_err());
 
-    match catch_unwind(|| -> Result<()> {
-        let lua = Lua::new();
-        let globals = lua.globals();
+    Ok(())
+}
 
-        lua.load(
-            r#"
-            function rust_panic()
-                local _, err = pcall(function () rust_panic_function() end)
-                if err ~= nil then
-                    error(err)
-                end
-            end
-        "#,
-        )
-        .exec()?;
+#[test]
+fn test_panic() -> Result<()> {
+    fn make_lua() -> Result<Lua> {
+        let lua = Lua::new();
         let rust_panic_function =
-            lua.create_function(|_, ()| -> Result<()> { panic!("test_panic") })?;
-        globals.set("rust_panic_function", rust_panic_function)?;
+            lua.create_function(|_, ()| -> Result<()> { panic!("rust panic") })?;
+        lua.globals()
+            .set("rust_panic_function", rust_panic_function)?;
+        Ok(lua)
+    }
 
-        let rust_panic = globals.get::<_, Function>("rust_panic")?;
+    // Test triggerting Lua error passing Rust panic (must be resumed)
+    {
+        let lua = make_lua()?;
 
-        rust_panic.call::<_, ()>(())
-    }) {
-        Ok(Ok(_)) => panic!("no panic was detected"),
-        Ok(Err(e)) => panic!("error during panic test {:?}", e),
-        Err(p) => assert!(*p.downcast::<&str>().unwrap() == "test_panic"),
-    };
+        match catch_unwind(AssertUnwindSafe(|| -> Result<()> {
+            lua.load(
+                r#"
+                _, err = pcall(rust_panic_function)
+                error(err)
+            "#,
+            )
+            .exec()
+        })) {
+            Ok(Ok(_)) => panic!("no panic was detected"),
+            Ok(Err(e)) => panic!("error during panic test {:?}", e),
+            Err(p) => assert!(*p.downcast::<&str>().unwrap() == "rust panic"),
+        };
+
+        // Trigger same panic again
+        match lua.load("error(err)").exec() {
+            Ok(_) => panic!("no error was detected"),
+            Err(Error::PreviouslyResumedPanic) => {}
+            Err(e) => panic!("expected PreviouslyResumedPanic, got {:?}", e),
+        }
+    }
 
-    match catch_unwind(|| -> Result<()> {
-        let lua = Lua::new();
-        let globals = lua.globals();
+    // Test returning Rust panic (must be resumed)
+    {
+        let lua = make_lua()?;
+        match catch_unwind(AssertUnwindSafe(|| -> Result<()> {
+            let _catched_panic = lua
+                .load(
+                    r#"
+                    -- Set global
+                    _, err = pcall(rust_panic_function)
+                    return err
+                "#,
+                )
+                .eval::<Value>()?;
+            Ok(())
+        })) {
+            Ok(_) => panic!("no panic was detected"),
+            Err(_) => {}
+        };
+
+        assert!(lua.globals().get::<_, Value>("err")? == Value::Nil);
+        match lua.load("tostring(err)").exec() {
+            Ok(_) => panic!("no error was detected"),
+            Err(Error::CallbackError { ref cause, .. }) => match cause.as_ref() {
+                Error::PreviouslyResumedPanic => {}
+                e => panic!("expected PreviouslyResumedPanic, got {:?}", e),
+            },
+            Err(e) => panic!("expected CallbackError, got {:?}", e),
+        }
+    }
 
+    // Test representing rust panic as a string
+    match catch_unwind(|| -> Result<()> {
+        let lua = make_lua()?;
         lua.load(
             r#"
-            function rust_panic()
-                local _, err = pcall(function () rust_panic_function() end)
-                if err ~= nil then
-                    error(tostring(err))
-                end
-            end
+            local _, err = pcall(rust_panic_function)
+            error(tostring(err))
         "#,
         )
-        .exec()?;
-        let rust_panic_function =
-            lua.create_function(|_, ()| -> Result<()> { panic!("test_panic") })?;
-        globals.set("rust_panic_function", rust_panic_function)?;
-
-        let rust_panic = globals.get::<_, Function>("rust_panic")?;
-
-        rust_panic.call::<_, ()>(())
+        .exec()
     }) {
         Ok(Ok(_)) => panic!("no error was detected"),
         Ok(Err(Error::RuntimeError(_))) => {}
-        Ok(Err(e)) => panic!("unexpected error during panic test {:?}", e),
+        Ok(Err(e)) => panic!("expected RuntimeError, got {:?}", e),
         Err(_) => panic!("panic was detected"),
-    };
+    }
 
     Ok(())
 }