Fix FFI panic guard: use panic=unwind in release profile
ober
88927365680050010fa448ceeb410dfe3533fe64
--- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -67,9 +67,10 @@ fn guard<F: FnOnce() -> i32>(f: F) -> i32 { ``` A Rust panic must never unwind across the FFI boundary (that's undefined -behaviour). `guard` traps it and returns `JPGP_E_INTERNAL`. Combined -with `panic = "abort"` in the release profile, panic paths are -essentially impossible in production builds. +behaviour). `guard` traps it and returns `JPGP_E_INTERNAL`. For `catch_unwind` +to actually catch a panic, the release profile uses `panic = "unwind"`; +`panic = "abort"` would turn every panic into a process abort and make the +guard a no-op. ### The release profile @@ -79,7 +80,7 @@ opt-level = "z" # size, not speed lto = true codegen-units = 1 strip = true -panic = "abort" +panic = "unwind" # required so the catch_unwind FFI guard can trap panics ``` The Rust crate's final size matters more than its raw speed — most CPU --- a/docs/FFI.md +++ b/docs/FFI.md @@ -81,9 +81,10 @@ fn guard<F: FnOnce() -> i32>(f: F) -> i32 { } ``` -Combined with `panic = "abort"` in the release profile, panic paths are -effectively unreachable. The guard exists so even debug builds can't -unwind across `extern "C"` (UB). +The release profile uses `panic = "unwind"` so `catch_unwind` can actually +trap a panic and return `JPGP_E_INTERNAL`. (`panic = "abort"` would abort the +whole CLI on any panic and make this guard a no-op.) The guard ensures no +panic ever unwinds across `extern "C"` (UB). ## Functions --- a/docs/THREAT-MODEL.md +++ b/docs/THREAT-MODEL.md @@ -66,8 +66,9 @@ through the same process. The interesting trust boundaries are: [`pgp/prompt.ss`](../pgp/prompt.ss). 2. **FFI boundary.** All inputs are length-checked or NUL-terminated; all outputs go through the `(buf, buf_len, *out_len)` pattern. - Every entry point is wrapped in `catch_unwind`. Combined with - `panic = "abort"`, panic paths cannot unwind across the boundary. + Every entry point is wrapped in `catch_unwind`. The release profile uses + `panic = "unwind"` so the guard actually traps a panic and returns + `JPGP_E_INTERNAL`; nothing unwinds across the boundary. Code: [`pgp-native/src/lib.rs`](../pgp-native/src/lib.rs), [`pgp-native/src/util.rs`](../pgp-native/src/util.rs). 3. **File parsing.** Recipient files, identity files, signature blobs, --- a/pgp-native/Cargo.toml +++ b/pgp-native/Cargo.toml @@ -28,4 +28,4 @@ opt-level = "z" lto = true codegen-units = 1 strip = true -panic = "abort" +panic = "unwind" --- a/pgp-native/src/lib.rs +++ b/pgp-native/src/lib.rs @@ -363,11 +363,66 @@ pub extern "C" fn jpgp_abi_version() -> u32 { 2 } +/// Test-only FFI entry point whose body always panics inside `guard`. +/// Compiled only under `cfg(test)` so no panic path ships in release builds. +/// Used to prove `catch_unwind` traps a panic at the `extern "C"` boundary and +/// maps it to `JPGP_E_INTERNAL` instead of aborting/unwinding (which requires +/// `panic = "unwind"`; with `panic = "abort"` this would kill the process). +#[cfg(test)] +#[no_mangle] +pub unsafe extern "C" fn jpgp_test_force_panic() -> i32 { + guard(|| -> i32 { + panic!("forced panic for FFI guard regression test"); + }) +} + #[cfg(test)] mod tests { use super::*; #[test] + fn guard_maps_panic_to_internal_error() { + let code = guard(|| -> i32 { + panic!("forced panic inside guard"); + }); + assert_eq!(code, JPGP_E_INTERNAL); + } + + #[test] + fn ffi_entry_point_traps_panic_returns_internal_error() { + let code = unsafe { jpgp_test_force_panic() }; + assert_eq!(code, JPGP_E_INTERNAL); + } + + #[test] + fn release_profile_keeps_unwind_so_guard_can_catch() { + let manifest = + std::fs::read_to_string(concat!(env!("CARGO_MANIFEST_DIR"), "/Cargo.toml")) + .expect("read Cargo.toml"); + let mut in_release = false; + let mut panic_value: Option<String> = None; + for line in manifest.lines() { + let trimmed = line.trim(); + if trimmed.starts_with('[') { + in_release = trimmed == "[profile.release]"; + continue; + } + if in_release { + if let Some(rest) = trimmed.strip_prefix("panic") { + if let Some(rest) = rest.trim_start().strip_prefix('=') { + panic_value = Some(rest.trim().trim_matches('"').to_string()); + } + } + } + } + assert_eq!( + panic_value.as_deref(), + Some("unwind"), + "[profile.release] must set panic = \"unwind\" so the catch_unwind FFI guard can trap panics; panic = \"abort\" makes the guard a no-op and aborts the CLI" + ); + } + + #[test] fn ed25519_ffi_rejects_wrong_fixed_lengths() { let mut sk = [0u8; 32]; let mut pk = [0u8; 32];