From 5db3afca48fabd5d0b09cfa78370cc81163cb488 Mon Sep 17 00:00:00 2001 From: Matt Hess Date: Thu, 26 Feb 2026 15:46:25 +0000 Subject: [PATCH] Add handle lifecycle safety, wallet_close and daemon_close now wait for active sync to finish before dropping, preventing use-after-free on wallet switch --- crates/salvium-ffi/src/daemon.rs | 71 ++++++++++++++++++++++++------ crates/salvium-ffi/src/transfer.rs | 32 +++++++------- crates/salvium-ffi/src/wallet.rs | 53 ++++++++++++++++++++-- 3 files changed, 122 insertions(+), 34 deletions(-) diff --git a/crates/salvium-ffi/src/daemon.rs b/crates/salvium-ffi/src/daemon.rs index dd18568..522d46f 100644 --- a/crates/salvium-ffi/src/daemon.rs +++ b/crates/salvium-ffi/src/daemon.rs @@ -1,6 +1,7 @@ //! Daemon RPC handle and query functions. use std::ffi::{c_char, c_void}; +use std::sync::atomic::{AtomicUsize, Ordering}; use crate::error::{ffi_try_ptr, ffi_try_string}; use crate::handles::{borrow_handle, drop_handle}; @@ -8,6 +9,35 @@ use crate::strings::c_str_to_str; use salvium_rpc::DaemonRpc; +/// Wrapper that pairs a DaemonRpc with a usage counter. +/// +/// `in_use` tracks how many long-running operations (sync) currently hold a +/// reference. `salvium_daemon_close` waits for `in_use == 0` before dropping, +/// preventing use-after-free when the app closes the daemon while a sync is +/// still running on another thread. +pub(crate) struct DaemonHandle { + pub daemon: DaemonRpc, + pub in_use: AtomicUsize, +} + +impl DaemonHandle { + fn new(daemon: DaemonRpc) -> Self { + Self { + daemon, + in_use: AtomicUsize::new(0), + } + } +} + +/// RAII guard that decrements `in_use` on drop. +pub(crate) struct DaemonUseGuard<'a>(pub &'a AtomicUsize); + +impl Drop for DaemonUseGuard<'_> { + fn drop(&mut self) { + self.0.fetch_sub(1, Ordering::Release); + } +} + /// Connect to a daemon RPC endpoint. /// /// - `url`: null-terminated URL (e.g. "http://127.0.0.1:19081") @@ -17,14 +47,27 @@ use salvium_rpc::DaemonRpc; pub unsafe extern "C" fn salvium_daemon_connect(url: *const c_char) -> *mut c_void { ffi_try_ptr(|| { let url_str = unsafe { c_str_to_str(url) }?; - Ok(DaemonRpc::new(url_str)) + Ok(DaemonHandle::new(DaemonRpc::new(url_str))) }) } /// Close a daemon handle. +/// +/// If the daemon is in use by a sync operation, this blocks until the sync +/// finishes before dropping. Safe to call from any thread. #[no_mangle] pub unsafe extern "C" fn salvium_daemon_close(handle: *mut c_void) { - drop_handle::(handle); + if handle.is_null() { + return; + } + let dh = unsafe { &*(handle as *const DaemonHandle) }; + + // Wait for outstanding users (sync) to finish. + while dh.in_use.load(Ordering::Acquire) > 0 { + std::thread::sleep(std::time::Duration::from_millis(50)); + } + + drop_handle::(handle); } /// Get daemon info as JSON. @@ -34,9 +77,9 @@ pub unsafe extern "C" fn salvium_daemon_close(handle: *mut c_void) { #[no_mangle] pub unsafe extern "C" fn salvium_daemon_get_info(handle: *mut c_void) -> *mut c_char { ffi_try_string(|| { - let daemon = unsafe { borrow_handle::(handle) }?; + let dh = unsafe { borrow_handle::(handle) }?; let rt = crate::runtime(); - let info = rt.block_on(daemon.get_info()).map_err(|e| e.to_string())?; + let info = rt.block_on(dh.daemon.get_info()).map_err(|e| e.to_string())?; serde_json::to_string(&info).map_err(|e| e.to_string()) }) } @@ -47,9 +90,9 @@ pub unsafe extern "C" fn salvium_daemon_get_info(handle: *mut c_void) -> *mut c_ #[no_mangle] pub unsafe extern "C" fn salvium_daemon_get_height(handle: *mut c_void) -> u64 { let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - let daemon = unsafe { borrow_handle::(handle) }.ok()?; + let dh = unsafe { borrow_handle::(handle) }.ok()?; let rt = crate::runtime(); - rt.block_on(daemon.get_height()).ok() + rt.block_on(dh.daemon.get_height()).ok() })); match result { Ok(Some(h)) => h, @@ -63,9 +106,9 @@ pub unsafe extern "C" fn salvium_daemon_get_height(handle: *mut c_void) -> u64 { #[no_mangle] pub unsafe extern "C" fn salvium_daemon_is_synchronized(handle: *mut c_void) -> i32 { let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - let daemon = unsafe { borrow_handle::(handle) }.ok()?; + let dh = unsafe { borrow_handle::(handle) }.ok()?; let rt = crate::runtime(); - rt.block_on(daemon.is_synchronized()).ok() + rt.block_on(dh.daemon.is_synchronized()).ok() })); match result { Ok(Some(true)) => 1, @@ -81,10 +124,10 @@ pub unsafe extern "C" fn salvium_daemon_is_synchronized(handle: *mut c_void) -> #[no_mangle] pub unsafe extern "C" fn salvium_daemon_get_fee_estimate(handle: *mut c_void) -> *mut c_char { ffi_try_string(|| { - let daemon = unsafe { borrow_handle::(handle) }?; + let dh = unsafe { borrow_handle::(handle) }?; let rt = crate::runtime(); let fee = rt - .block_on(daemon.get_fee_estimate(0)) + .block_on(dh.daemon.get_fee_estimate(0)) .map_err(|e| e.to_string())?; serde_json::to_string(&fee).map_err(|e| e.to_string()) }) @@ -96,10 +139,10 @@ pub unsafe extern "C" fn salvium_daemon_get_fee_estimate(handle: *mut c_void) -> #[no_mangle] pub unsafe extern "C" fn salvium_daemon_get_supply_info(handle: *mut c_void) -> *mut c_char { ffi_try_string(|| { - let daemon = unsafe { borrow_handle::(handle) }?; + let dh = unsafe { borrow_handle::(handle) }?; let rt = crate::runtime(); let info = rt - .block_on(daemon.get_supply_info()) + .block_on(dh.daemon.get_supply_info()) .map_err(|e| e.to_string())?; serde_json::to_string(&info).map_err(|e| e.to_string()) }) @@ -111,10 +154,10 @@ pub unsafe extern "C" fn salvium_daemon_get_supply_info(handle: *mut c_void) -> #[no_mangle] pub unsafe extern "C" fn salvium_daemon_get_yield_info(handle: *mut c_void) -> *mut c_char { ffi_try_string(|| { - let daemon = unsafe { borrow_handle::(handle) }?; + let dh = unsafe { borrow_handle::(handle) }?; let rt = crate::runtime(); let info = rt - .block_on(daemon.get_yield_info()) + .block_on(dh.daemon.get_yield_info()) .map_err(|e| e.to_string())?; serde_json::to_string(&info).map_err(|e| e.to_string()) }) diff --git a/crates/salvium-ffi/src/transfer.rs b/crates/salvium-ffi/src/transfer.rs index c7f0a11..a28f324 100644 --- a/crates/salvium-ffi/src/transfer.rs +++ b/crates/salvium-ffi/src/transfer.rs @@ -114,8 +114,8 @@ pub unsafe extern "C" fn salvium_wallet_transfer( params_json: *const c_char, ) -> *mut c_char { ffi_try_string(|| { - let wallet_ref = unsafe { borrow_handle::(wallet) }?; - let daemon_ref = unsafe { borrow_handle::(daemon) }?; + let wh = unsafe { borrow_handle::(wallet) }?; + let dh = unsafe { borrow_handle::(daemon) }?; let json_str = unsafe { c_str_to_str(params_json) }?; let params: TransferParams = @@ -125,14 +125,14 @@ pub unsafe extern "C" fn salvium_wallet_transfer( return Err("no destinations specified".into()); } - if !wallet_ref.can_spend() { + if !wh.wallet.can_spend() { return Err("wallet is view-only, cannot sign transactions".into()); } let priority = parse_priority(¶ms.priority); let rt = crate::runtime(); - rt.block_on(async { do_transfer(wallet_ref, daemon_ref, ¶ms, priority).await }) + rt.block_on(async { do_transfer(&wh.wallet, &dh.daemon, ¶ms, priority).await }) }) } @@ -158,21 +158,21 @@ pub unsafe extern "C" fn salvium_wallet_stake( params_json: *const c_char, ) -> *mut c_char { ffi_try_string(|| { - let wallet_ref = unsafe { borrow_handle::(wallet) }?; - let daemon_ref = unsafe { borrow_handle::(daemon) }?; + let wh = unsafe { borrow_handle::(wallet) }?; + let dh = unsafe { borrow_handle::(daemon) }?; let json_str = unsafe { c_str_to_str(params_json) }?; let params: StakeParams = serde_json::from_str(json_str).map_err(|e| format!("invalid stake params: {e}"))?; - if !wallet_ref.can_spend() { + if !wh.wallet.can_spend() { return Err("wallet is view-only, cannot sign transactions".into()); } let priority = parse_priority(¶ms.priority); let rt = crate::runtime(); - rt.block_on(async { do_stake(wallet_ref, daemon_ref, ¶ms, priority).await }) + rt.block_on(async { do_stake(&wh.wallet, &dh.daemon, ¶ms, priority).await }) }) } @@ -200,21 +200,21 @@ pub unsafe extern "C" fn salvium_wallet_sweep( params_json: *const c_char, ) -> *mut c_char { ffi_try_string(|| { - let wallet_ref = unsafe { borrow_handle::(wallet) }?; - let daemon_ref = unsafe { borrow_handle::(daemon) }?; + let wh = unsafe { borrow_handle::(wallet) }?; + let dh = unsafe { borrow_handle::(daemon) }?; let json_str = unsafe { c_str_to_str(params_json) }?; let params: SweepParams = serde_json::from_str(json_str).map_err(|e| format!("invalid sweep params: {e}"))?; - if !wallet_ref.can_spend() { + if !wh.wallet.can_spend() { return Err("wallet is view-only, cannot sign transactions".into()); } let priority = parse_priority(¶ms.priority); let rt = crate::runtime(); - rt.block_on(async { do_sweep(wallet_ref, daemon_ref, ¶ms, priority).await }) + rt.block_on(async { do_sweep(&wh.wallet, &dh.daemon, ¶ms, priority).await }) }) } @@ -233,8 +233,8 @@ pub unsafe extern "C" fn salvium_wallet_transfer_dry_run( params_json: *const c_char, ) -> *mut c_char { ffi_try_string(|| { - let wallet_ref = unsafe { borrow_handle::(wallet) }?; - let daemon_ref = unsafe { borrow_handle::(daemon) }?; + let wh = unsafe { borrow_handle::(wallet) }?; + let dh = unsafe { borrow_handle::(daemon) }?; let json_str = unsafe { c_str_to_str(params_json) }?; let mut params: TransferParams = @@ -244,14 +244,14 @@ pub unsafe extern "C" fn salvium_wallet_transfer_dry_run( if params.destinations.is_empty() { return Err("no destinations specified".into()); } - if !wallet_ref.can_spend() { + if !wh.wallet.can_spend() { return Err("wallet is view-only, cannot sign transactions".into()); } let priority = parse_priority(¶ms.priority); let rt = crate::runtime(); - rt.block_on(async { do_transfer(wallet_ref, daemon_ref, ¶ms, priority).await }) + rt.block_on(async { do_transfer(&wh.wallet, &dh.daemon, ¶ms, priority).await }) }) } diff --git a/crates/salvium-ffi/src/wallet.rs b/crates/salvium-ffi/src/wallet.rs index 739a078..23eeb40 100644 --- a/crates/salvium-ffi/src/wallet.rs +++ b/crates/salvium-ffi/src/wallet.rs @@ -10,10 +10,18 @@ use crate::strings::c_str_to_str; use salvium_wallet::Wallet; -/// Wrapper that pairs a Wallet with its cancellation flag. +/// Wrapper that pairs a Wallet with its cancellation and lifecycle flags. +/// +/// `sync_running` is set to `true` while `salvium_wallet_sync` is executing +/// and back to `false` when it returns (even on error / cancel). +/// `salvium_wallet_close` checks this flag and, if sync is active, sets +/// `sync_cancel` and spins until sync finishes before dropping the handle. +/// This prevents use-after-free when the app closes a wallet while sync is +/// still running on another thread. pub(crate) struct WalletHandle { pub wallet: Wallet, pub sync_cancel: Arc, + pub sync_running: AtomicBool, } impl WalletHandle { @@ -21,10 +29,21 @@ impl WalletHandle { Self { wallet, sync_cancel: Arc::new(AtomicBool::new(false)), + sync_running: AtomicBool::new(false), } } } +/// RAII guard that sets `sync_running` to `false` on drop, ensuring +/// the flag is cleared even if sync panics or returns early. +struct SyncRunningGuard<'a>(&'a AtomicBool); + +impl Drop for SyncRunningGuard<'_> { + fn drop(&mut self) { + self.0.store(false, Ordering::Release); + } +} + // ============================================================================= // Wallet Lifecycle // ============================================================================= @@ -107,8 +126,26 @@ pub unsafe extern "C" fn salvium_wallet_open( } /// Close a wallet handle, releasing all resources. +/// +/// If a sync is in progress, this cancels it and blocks until the sync +/// loop has exited before dropping the handle. Safe to call from any thread. #[no_mangle] pub unsafe extern "C" fn salvium_wallet_close(handle: *mut c_void) { + if handle.is_null() { + return; + } + // Safety: pointer is non-null and was created by into_handle. + // We read the atomic flags through a shared ref before drop_handle takes ownership. + let wh = unsafe { &*(handle as *const WalletHandle) }; + + // Signal any running sync to stop. + wh.sync_cancel.store(true, Ordering::Relaxed); + + // Wait for sync to finish before dropping. + while wh.sync_running.load(Ordering::Acquire) { + std::thread::sleep(std::time::Duration::from_millis(50)); + } + drop_handle::(handle); } @@ -354,12 +391,20 @@ pub unsafe extern "C" fn salvium_wallet_sync( ) -> i32 { ffi_try(|| { let handle = unsafe { borrow_handle_mut::(wallet) }?; - let daemon_ref = unsafe { borrow_handle::(daemon) }?; + let dh = unsafe { borrow_handle::(daemon) }?; let rt = crate::runtime(); // Reset cancel flag before starting. handle.sync_cancel.store(false, Ordering::Relaxed); + // Mark sync as active; the guard clears this on drop (even on panic/early return). + handle.sync_running.store(true, Ordering::Release); + let _guard = SyncRunningGuard(&handle.sync_running); + + // Register daemon usage so daemon_close waits for us. + dh.in_use.fetch_add(1, Ordering::Release); + let _daemon_guard = crate::daemon::DaemonUseGuard(&dh.in_use); + rt.block_on(async { if let Some(cb) = callback { let (tx, mut rx) = tokio::sync::mpsc::channel::(256); @@ -373,7 +418,7 @@ pub unsafe extern "C" fn salvium_wallet_sync( let result = handle .wallet - .sync(daemon_ref, Some(&tx), &handle.sync_cancel) + .sync(&dh.daemon, Some(&tx), &handle.sync_cancel) .await; drop(tx); // Close channel so forwarder exits. let _ = forwarder.await; @@ -382,7 +427,7 @@ pub unsafe extern "C" fn salvium_wallet_sync( } else { handle .wallet - .sync(daemon_ref, None, &handle.sync_cancel) + .sync(&dh.daemon, None, &handle.sync_cancel) .await .map(|_| ()) .map_err(|e| e.to_string())