Skip to content

Commit 86f73ce

Browse files
committed
fix(lance): contain Rust panics at FFI boundary
1 parent 7eedef0 commit 86f73ce

6 files changed

Lines changed: 688 additions & 326 deletions

File tree

‎crates/lance_ffi/Cargo.toml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ cbindgen = "0.29"
4444
opt-level = 3
4545
lto = "thin"
4646
codegen-units = 1
47-
panic = "abort"
47+
panic = "unwind"
4848

4949
[profile.dev]
50-
panic = "abort"
50+
panic = "unwind"

‎crates/lance_ffi/src/error.rs‎

Lines changed: 73 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,10 @@
1515
// specific language governing permissions and limitations
1616
// under the License.
1717

18+
use std::any::Any;
1819
use std::cell::RefCell;
1920
use std::ffi::{c_char, CString};
21+
use std::panic::{catch_unwind, AssertUnwindSafe};
2022

2123
thread_local! {
2224
static LAST_ERROR: RefCell<CString> =
@@ -38,7 +40,77 @@ pub(crate) fn fail(message: impl Into<String>) -> i32 {
3840
-1
3941
}
4042

43+
pub(crate) fn panic_message(payload: Box<dyn Any + Send>) -> String {
44+
let message = payload
45+
.downcast_ref::<String>()
46+
.map(String::as_str)
47+
.or_else(|| payload.downcast_ref::<&str>().copied())
48+
.unwrap_or("unknown panic");
49+
format!("Lance panicked: {message}")
50+
}
51+
52+
// Catch before returning through the C ABI, including panics during cleanup.
53+
pub(crate) fn catch_panic<T>(fallback: T, operation: impl FnOnce() -> T) -> T {
54+
match catch_unwind(AssertUnwindSafe(operation)) {
55+
Ok(result) => result,
56+
Err(payload) => {
57+
fail(panic_message(payload));
58+
fallback
59+
}
60+
}
61+
}
62+
4163
#[no_mangle]
4264
pub extern "C" fn paimon_lance_last_error() -> *const c_char {
43-
LAST_ERROR.with(|cell| cell.borrow().as_ptr())
65+
// Do not try to record another error if accessing the error slot panics.
66+
catch_unwind(|| LAST_ERROR.with(|cell| cell.borrow().as_ptr())).unwrap_or(std::ptr::null())
67+
}
68+
69+
#[cfg(test)]
70+
mod tests {
71+
use super::*;
72+
use std::ffi::CStr;
73+
74+
pub(crate) fn last_error() -> String {
75+
unsafe { CStr::from_ptr(paimon_lance_last_error()) }
76+
.to_str()
77+
.unwrap()
78+
.to_owned()
79+
}
80+
81+
#[test]
82+
fn panic_becomes_error_and_unwinds_locals() {
83+
let dropped = std::cell::Cell::new(false);
84+
struct OnDrop<'a>(&'a std::cell::Cell<bool>);
85+
impl Drop for OnDrop<'_> {
86+
fn drop(&mut self) {
87+
self.0.set(true);
88+
}
89+
}
90+
assert_eq!(
91+
catch_panic(-1, || {
92+
let _guard = OnDrop(&dropped);
93+
panic!("decode failed");
94+
}),
95+
-1
96+
);
97+
assert!(dropped.get());
98+
assert_eq!(last_error(), "Lance panicked: decode failed");
99+
assert_eq!(
100+
catch_panic(-1, || {
101+
clear_last_error();
102+
42
103+
}),
104+
42
105+
);
106+
assert_eq!(last_error(), "");
107+
}
108+
109+
#[test]
110+
fn supports_owned_and_non_string_panics() {
111+
catch_panic((), || std::panic::panic_any(String::from("owned message")));
112+
assert_eq!(last_error(), "Lance panicked: owned message");
113+
catch_panic((), || std::panic::panic_any(42));
114+
assert_eq!(last_error(), "Lance panicked: unknown panic");
115+
}
44116
}

‎crates/lance_ffi/src/lib.rs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@
1717

1818
#![deny(unsafe_op_in_unsafe_fn)]
1919

20+
#[cfg(not(panic = "unwind"))]
21+
compile_error!("paimon_lance_ffi requires panic=unwind to contain panics at the C ABI");
22+
2023
mod error;
2124
mod reader;
2225
mod runtime;

0 commit comments

Comments
 (0)