Skip to content

Commit b958949

Browse files
khvzakkhvzak
committed
Improve panic handling (check for twice resumed panics)
1 parent 58cb371 commit b958949

File tree

4 files changed

+118
-75
lines changed

4 files changed

+118
-75
lines changed

‎src/error.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,11 @@ pub enum Error {
139139
/// Original error returned by the Rust code.
140140
cause: Arc<Error>,
141141
},
142+
/// A Rust panic that was previosly resumed, returned again.
143+
///
144+
/// This error can occur only when a Rust panic resumed previously was recovered
145+
/// and returned again.
146+
PreviouslyResumedPanic,
142147
/// Serialization error.
143148
#[cfg(feature = "serialize")]
144149
#[cfg_attr(docsrs, doc(cfg(feature = "serialize")))]
@@ -230,6 +235,9 @@ impl fmt::Display for Error {
230235
Error::CallbackError { ref traceback, .. } => {
231236
write!(fmt, "callback error: {}", traceback)
232237
}
238+
Error::PreviouslyResumedPanic => {
239+
write!(fmt, "previously resumed panic returned again")
240+
}
233241
#[cfg(feature = "serialize")]
234242
Error::SerializeError(ref err) => {
235243
write!(fmt, "serialize error: {}", err)

‎src/lua.rs

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ use std::collections::HashMap;
44
use std::ffi::CString;
55
use std::marker::PhantomData;
66
use std::os::raw::{c_char, c_int, c_void};
7+
use std::panic::resume_unwind;
78
use std::sync::{Arc, Mutex, Weak};
89
use std::{mem, ptr, str};
910

@@ -25,7 +26,7 @@ use crate::util::{
2526
assert_stack, callback_error, check_stack, get_gc_userdata, get_main_state, get_userdata,
2627
get_wrapped_error, init_error_registry, init_gc_metatable_for, init_userdata_metatable,
2728
pop_error, protect_lua, protect_lua_closure, push_gc_userdata, push_meta_gc_userdata,
28-
push_string, push_userdata, push_wrapped_error, StackGuard,
29+
push_string, push_userdata, push_wrapped_error, StackGuard, WrappedPanic,
2930
};
3031
use crate::value::{FromLua, FromLuaMulti, MultiValue, Nil, ToLua, ToLuaMulti, Value};
3132

@@ -1402,32 +1403,33 @@ impl Lua {
14021403

14031404
// Uses 2 stack spaces, does not call checkstack
14041405
pub(crate) unsafe fn pop_value(&self) -> Value {
1405-
match ffi::lua_type(self.state, -1) {
1406+
let state = self.state;
1407+
match ffi::lua_type(state, -1) {
14061408
ffi::LUA_TNIL => {
1407-
ffi::lua_pop(self.state, 1);
1409+
ffi::lua_pop(state, 1);
14081410
Nil
14091411
}
14101412

14111413
ffi::LUA_TBOOLEAN => {
1412-
let b = Value::Boolean(ffi::lua_toboolean(self.state, -1) != 0);
1413-
ffi::lua_pop(self.state, 1);
1414+
let b = Value::Boolean(ffi::lua_toboolean(state, -1) != 0);
1415+
ffi::lua_pop(state, 1);
14141416
b
14151417
}
14161418

14171419
ffi::LUA_TLIGHTUSERDATA => {
1418-
let ud = Value::LightUserData(LightUserData(ffi::lua_touserdata(self.state, -1)));
1419-
ffi::lua_pop(self.state, 1);
1420+
let ud = Value::LightUserData(LightUserData(ffi::lua_touserdata(state, -1)));
1421+
ffi::lua_pop(state, 1);
14201422
ud
14211423
}
14221424

14231425
ffi::LUA_TNUMBER => {
1424-
if ffi::lua_isinteger(self.state, -1) != 0 {
1425-
let i = Value::Integer(ffi::lua_tointeger(self.state, -1));
1426-
ffi::lua_pop(self.state, 1);
1426+
if ffi::lua_isinteger(state, -1) != 0 {
1427+
let i = Value::Integer(ffi::lua_tointeger(state, -1));
1428+
ffi::lua_pop(state, 1);
14271429
i
14281430
} else {
1429-
let n = Value::Number(ffi::lua_tonumber(self.state, -1));
1430-
ffi::lua_pop(self.state, 1);
1431+
let n = Value::Number(ffi::lua_tonumber(state, -1));
1432+
ffi::lua_pop(state, 1);
14311433
n
14321434
}
14331435
}
@@ -1439,12 +1441,20 @@ impl Lua {
14391441
ffi::LUA_TFUNCTION => Value::Function(Function(self.pop_ref())),
14401442

14411443
ffi::LUA_TUSERDATA => {
1442-
// It should not be possible to interact with userdata types other than custom
1443-
// UserData types OR a WrappedError. WrappedPanic should not be here.
1444-
if let Some(err) = get_wrapped_error(self.state, -1).as_ref() {
1444+
// We must prevent interaction with userdata types other than UserData OR a WrappedError.
1445+
// WrappedPanics are automatically resumed.
1446+
if let Some(err) = get_wrapped_error(state, -1).as_ref() {
14451447
let err = err.clone();
1446-
ffi::lua_pop(self.state, 1);
1448+
ffi::lua_pop(state, 1);
14471449
Value::Error(err)
1450+
} else if let Some(panic) = get_gc_userdata::<WrappedPanic>(state, -1).as_mut() {
1451+
if let Some(panic) = (*panic).0.take() {
1452+
ffi::lua_pop(state, 1);
1453+
resume_unwind(panic);
1454+
}
1455+
// Previously resumed panic?
1456+
ffi::lua_pop(state, 1);
1457+
Nil
14481458
} else {
14491459
Value::UserData(AnyUserData(self.pop_ref()))
14501460
}

‎src/util.rs

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ pub unsafe fn pop_error(state: *mut ffi::lua_State, err_code: c_int) -> Error {
186186
if let Some(p) = (*panic).0.take() {
187187
resume_unwind(p);
188188
} else {
189-
mlua_panic!("error during panic handling, panic was resumed twice")
189+
Error::PreviouslyResumedPanic
190190
}
191191
} else {
192192
let err_string = to_string(state, -1).into_owned();
@@ -587,27 +587,22 @@ pub unsafe fn init_error_registry(state: *mut ffi::lua_State) {
587587
Ok(err_buf)
588588
} else if let Some(panic) = get_gc_userdata::<WrappedPanic>(state, -1).as_ref() {
589589
if let Some(ref p) = (*panic).0 {
590-
ffi::lua_pushlightuserdata(
591-
state,
592-
&ERROR_PRINT_BUFFER_KEY as *const u8 as *mut c_void,
593-
);
594-
ffi::lua_rawget(state, ffi::LUA_REGISTRYINDEX);
590+
let err_buf_key = &ERROR_PRINT_BUFFER_KEY as *const u8 as *const c_void;
591+
ffi::lua_rawgetp(state, ffi::LUA_REGISTRYINDEX, err_buf_key);
595592
let err_buf = ffi::lua_touserdata(state, -1) as *mut String;
593+
(*err_buf).clear();
596594
ffi::lua_pop(state, 2);
597595

598-
let error = if let Some(x) = p.downcast_ref::<&str>() {
599-
x.to_string()
600-
} else if let Some(x) = p.downcast_ref::<String>() {
601-
x.to_string()
596+
if let Some(msg) = p.downcast_ref::<&str>() {
597+
let _ = write!(&mut (*err_buf), "{}", msg);
598+
} else if let Some(msg) = p.downcast_ref::<String>() {
599+
let _ = write!(&mut (*err_buf), "{}", msg);
602600
} else {
603-
"panic".to_string()
601+
let _ = write!(&mut (*err_buf), "<panic>");
604602
};
605-
606-
(*err_buf).clear();
607-
let _ = write!(&mut (*err_buf), "{}", error);
608603
Ok(err_buf)
609604
} else {
610-
mlua_panic!("error during panic handling, panic was resumed")
605+
Err(Error::PreviouslyResumedPanic)
611606
}
612607
} else {
613608
// 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) {
721716
}
722717

723718
struct WrappedError(pub Error);
724-
struct WrappedPanic(pub Option<Box<dyn Any + Send + 'static>>);
719+
pub(crate) struct WrappedPanic(pub Option<Box<dyn Any + Send + 'static>>);
725720

726721
// Converts the given lua value to a string in a reasonable format without causing a Lua error or
727722
// panicking.

‎tests/tests.rs

Lines changed: 73 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
extern "system" {}
1212

1313
use std::iter::FromIterator;
14-
use std::panic::catch_unwind;
14+
use std::panic::{catch_unwind, AssertUnwindSafe};
1515
use std::sync::Arc;
1616
use std::{error, f32, f64, fmt};
1717

@@ -384,62 +384,92 @@ fn test_error() -> Result<()> {
384384

385385
assert!(understand_recursion.call::<_, ()>(()).is_err());
386386

387-
match catch_unwind(|| -> Result<()> {
388-
let lua = Lua::new();
389-
let globals = lua.globals();
387+
Ok(())
388+
}
390389

391-
lua.load(
392-
r#"
393-
function rust_panic()
394-
local _, err = pcall(function () rust_panic_function() end)
395-
if err ~= nil then
396-
error(err)
397-
end
398-
end
399-
"#,
400-
)
401-
.exec()?;
390+
#[test]
391+
fn test_panic() -> Result<()> {
392+
fn make_lua() -> Result<Lua> {
393+
let lua = Lua::new();
402394
let rust_panic_function =
403-
lua.create_function(|_, ()| -> Result<()> { panic!("test_panic") })?;
404-
globals.set("rust_panic_function", rust_panic_function)?;
395+
lua.create_function(|_, ()| -> Result<()> { panic!("rust panic") })?;
396+
lua.globals()
397+
.set("rust_panic_function", rust_panic_function)?;
398+
Ok(lua)
399+
}
405400

406-
let rust_panic = globals.get::<_, Function>("rust_panic")?;
401+
// Test triggerting Lua error passing Rust panic (must be resumed)
402+
{
403+
let lua = make_lua()?;
407404

408-
rust_panic.call::<_, ()>(())
409-
}) {
410-
Ok(Ok(_)) => panic!("no panic was detected"),
411-
Ok(Err(e)) => panic!("error during panic test {:?}", e),
412-
Err(p) => assert!(*p.downcast::<&str>().unwrap() == "test_panic"),
413-
};
405+
match catch_unwind(AssertUnwindSafe(|| -> Result<()> {
406+
lua.load(
407+
r#"
408+
_, err = pcall(rust_panic_function)
409+
error(err)
410+
"#,
411+
)
412+
.exec()
413+
})) {
414+
Ok(Ok(_)) => panic!("no panic was detected"),
415+
Ok(Err(e)) => panic!("error during panic test {:?}", e),
416+
Err(p) => assert!(*p.downcast::<&str>().unwrap() == "rust panic"),
417+
};
418+
419+
// Trigger same panic again
420+
match lua.load("error(err)").exec() {
421+
Ok(_) => panic!("no error was detected"),
422+
Err(Error::PreviouslyResumedPanic) => {}
423+
Err(e) => panic!("expected PreviouslyResumedPanic, got {:?}", e),
424+
}
425+
}
414426

415-
match catch_unwind(|| -> Result<()> {
416-
let lua = Lua::new();
417-
let globals = lua.globals();
427+
// Test returning Rust panic (must be resumed)
428+
{
429+
let lua = make_lua()?;
430+
match catch_unwind(AssertUnwindSafe(|| -> Result<()> {
431+
let _catched_panic = lua
432+
.load(
433+
r#"
434+
-- Set global
435+
_, err = pcall(rust_panic_function)
436+
return err
437+
"#,
438+
)
439+
.eval::<Value>()?;
440+
Ok(())
441+
})) {
442+
Ok(_) => panic!("no panic was detected"),
443+
Err(_) => {}
444+
};
445+
446+
assert!(lua.globals().get::<_, Value>("err")? == Value::Nil);
447+
match lua.load("tostring(err)").exec() {
448+
Ok(_) => panic!("no error was detected"),
449+
Err(Error::CallbackError { ref cause, .. }) => match cause.as_ref() {
450+
Error::PreviouslyResumedPanic => {}
451+
e => panic!("expected PreviouslyResumedPanic, got {:?}", e),
452+
},
453+
Err(e) => panic!("expected CallbackError, got {:?}", e),
454+
}
455+
}
418456

457+
// Test representing rust panic as a string
458+
match catch_unwind(|| -> Result<()> {
459+
let lua = make_lua()?;
419460
lua.load(
420461
r#"
421-
function rust_panic()
422-
local _, err = pcall(function () rust_panic_function() end)
423-
if err ~= nil then
424-
error(tostring(err))
425-
end
426-
end
462+
local _, err = pcall(rust_panic_function)
463+
error(tostring(err))
427464
"#,
428465
)
429-
.exec()?;
430-
let rust_panic_function =
431-
lua.create_function(|_, ()| -> Result<()> { panic!("test_panic") })?;
432-
globals.set("rust_panic_function", rust_panic_function)?;
433-
434-
let rust_panic = globals.get::<_, Function>("rust_panic")?;
435-
436-
rust_panic.call::<_, ()>(())
466+
.exec()
437467
}) {
438468
Ok(Ok(_)) => panic!("no error was detected"),
439469
Ok(Err(Error::RuntimeError(_))) => {}
440-
Ok(Err(e)) => panic!("unexpected error during panic test {:?}", e),
470+
Ok(Err(e)) => panic!("expected RuntimeError, got {:?}", e),
441471
Err(_) => panic!("panic was detected"),
442-
};
472+
}
443473

444474
Ok(())
445475
}

0 commit comments

Comments
 (0)