refactor(key-wallet-ffi): new more ergonomic FFIError implementation#670
Open
refactor(key-wallet-ffi): new more ergonomic FFIError implementation#670
Conversation
Contributor
📝 WalkthroughWalkthroughRefactored FFI error handling and pointer validation across key-wallet-ffi and dash-spv-ffi: introduced in-place Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ZocoLini
force-pushed
the
chore/ffi-error-handling
branch
7 times, most recently
from
6a5bf81 to
5521e4b
Compare
ZocoLini
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
key-wallet-ffi/src/account_collection.rs (1)
85-92:⚠️ Potential issue | 🔴 CriticalNull
errorpointer now causes UB.
deref_ptr!(wallet, error)unconditionally does(*error).clean()before any null check, but the# Safetydoc on line 82 still allowserrorto be null ("a valid pointer to an FFIError structure or null"). Callers passing a null error (the documented contract) will dereference a null pointer. Either the macro must null-checkerroror the safety contract must be tightened to require non-null. Same root cause as flagged onaccount.rs(seewallet_get_account_count).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/account_collection.rs` around lines 85 - 92, The function wallet_get_account_collection calls deref_ptr!(wallet, error) which unconditionally uses (*error).clean() and therefore UB when callers pass error = null as the public API allows; fix by handling a nullable error pointer instead of using the macro that assumes non-null: in wallet_get_account_collection, check if error.is_null() before invoking any error-clean/assign behavior and only call deref_ptr!-style validation for wallet when a non-null error is present, or update the deref_ptr! macro to perform a null-check on the error pointer before touching (*error).clean(); ensure references to FFIError, deref_ptr!, wallet_get_account_collection and FFIAccountCollection::new are preserved and that the function still returns the newly boxed FFIAccountCollection on success.key-wallet-ffi/src/account.rs (1)
546-566:⚠️ Potential issue | 🔴 Critical
deref_ptr!dereferenceserrorunconditionally before null-checking — violates documented safety contract.The
# Safetydocumentation forwallet_get_account_count,wallet_get_account_collection, and many other FFI functions states thaterror"must be a valid pointer to an FFIError structure or null". However, thederef_ptr!macro (andderef_ptr_mut!) calls(*$error).clean()as the first statement before any null-check. A caller passingerror = null— which the contract explicitly permits — will trigger undefined behavior.Fix: Null-check
errorbefore dereferencing in both macro arms:Macro fix in `key-wallet-ffi/src/error.rs`
macro_rules! deref_ptr { ($ptr:expr, $error:expr, $return_value:expr) => {{ - (*$error).clean(); - - if $ptr.is_null() { + if !$error.is_null() { + (*$error).clean(); + } + + if $ptr.is_null() { return { - (*$error).set( - $crate::error::FFIErrorCode::InvalidInput, - &format!("{} ptr is null", stringify!($ptr)), - ); + if !$error.is_null() { + (*$error).set( + $crate::error::FFIErrorCode::InvalidInput, + &format!("{} ptr is null", stringify!($ptr)), + ); + } $return_value }; } unsafe { &*$ptr } }}; // Apply same pattern to 2-arg arm }Apply the same pattern to
deref_ptr_mut!and the 2-argument variants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/account.rs` around lines 546 - 566, The deref_ptr! / deref_ptr_mut! macros currently call (*$error).clean() before checking whether error is null, violating the FFI safety contract used by functions like wallet_get_account_count and wallet_get_account_collection; update the macros in error.rs so they first check if $error.is_null() and only call (*$error).clean() when non-null (apply same pattern to both mutable and 2-argument variants), ensuring all callers that pass error = null do not cause an unconditional dereference of FFIError.key-wallet-ffi/src/managed_account.rs (2)
204-218:⚠️ Potential issue | 🟠 MajorFree the temporary
FFIErrormessage after copying it.On these failure paths,
wallet_manager_get_managed_wallet_infocan allocateerror.message; the code copies it into a new result error string but never frees the original localFFIErrormessage. That leaks on every upstream failure.🛠️ Suggested pattern
- return FFIManagedCoreAccountResult::error( - error.code, - if error.message.is_null() { - "Failed to get managed wallet info".to_string() - } else { - let c_str = std::ffi::CStr::from_ptr(error.message); - c_str.to_string_lossy().to_string() - }, - ); + let message = if error.message.is_null() { + "Failed to get managed wallet info".to_string() + } else { + let message = std::ffi::CStr::from_ptr(error.message).to_string_lossy().into_owned(); + error.free_message(); + message + }; + return FFIManagedCoreAccountResult::error(error.code, message);Also applies to: 322-336, 394-406, 455-467, 1211-1225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` around lines 204 - 218, The failure branch after calling wallet_manager_get_managed_wallet_info currently copies error.message into the FFIManagedCoreAccountResult but never frees the original FFIError::message, leaking memory; update the branch so that after you convert error.message (via CStr::from_ptr and to_string_lossy) you free the original C string (e.g., call the crate's FFI string free helper or libc::free on error.message) and null out error.message before returning; apply the same change to the other failure paths mentioned (the analogous blocks around the calls producing FFIError at the other locations) so each temporary FFIError.message is freed after copying.
924-933:⚠️ Potential issue | 🔴 CriticalDo not pass a nullable
errorpointer intoderef_ptr!.The
deref_ptr!macro immediately calls(*$error).clean()before validating any pointers. If a caller passes null forerror(which the function documents as permissible), this dereferences null, causing undefined behavior. Either guarderrorwith a null-check before invoking the macro, or change the ABI to require a non-nullFFIError*pointer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` around lines 924 - 933, The function managed_wallet_get_account_count currently calls the deref_ptr! macro with the error pointer even though the API documents error may be null; because deref_ptr! calls (*error).clean() it will dereference null and UB. Fix by null-checking the error pointer before any deref_ptr! invocation (e.g., validate error != std::ptr::null_mut() and only call deref_ptr! when non-null), or alternatively change the ABI/signature to require a non-null *mut FFIError and update callers; ensure references to deref_ptr!, FFIError, and managed_wallet_get_account_count are updated consistently.key-wallet-ffi/src/managed_account_collection.rs (1)
92-101:⚠️ Potential issue | 🔴 CriticalKeep
errornull-safe before usingderef_ptr!.Line 92 documents
erroras "valid pointer to an FFIError structure or null", butderef_ptr!unconditionally executes(*error).clean()at its first operation before validatingmanagerorwallet_id. A caller that omits the optional error out-param (passing NULL) will hit undefined behavior even with otherwise valid inputs. Either requireerrorto be non-null in the contract, or update the macro to only dereference and cleanerrorwhen it is non-null.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account_collection.rs` around lines 92 - 101, The function managed_wallet_get_account_collection currently calls the deref_ptr! macro which unconditionally dereferences and calls (*error).clean(), causing UB if the caller passed a null error pointer; change the implementation so the error out-param is null-safe: before invoking deref_ptr! ensure you only dereference/clean the error when error != std::ptr::null_mut(), or update/replace the macro with a null-aware variant that checks error for null and only calls (*error).clean() when non-null; keep all other validations for manager and wallet_id intact and continue to document error as optional (FFIError or null).key-wallet-ffi/src/utxo.rs (1)
85-97:⚠️ Potential issue | 🔴 CriticalFix
deref_ptr!macro to check for nullerrorbefore dereferencing.The macro unconditionally dereferences the
errorpointer via(*$error).clean()on the first invocation, contradicting the documented nullable semantics. This causes undefined behavior when callers pass null (which is explicitly permitted by the safety documentation). Either require error to always be non-null, or guard all error dereferences with null checks inside the macro before calling.clean()and.set().Both
managed_wallet_get_utxosand the deprecatedwallet_get_utxos(and all other FFI functions using this macro) will have UB when error is null.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/utxo.rs` around lines 85 - 97, The deref_ptr! macro currently dereferences the error pointer unconditionally (calling (*$error).clean()), violating the documented nullable semantics and causing UB when error is null (affects managed_wallet_get_utxos, wallet_get_utxos, and all FFI callers). Update deref_ptr! so it first checks if the provided error pointer is non-null before calling (*error).clean() or (*error).set(...); only perform those error-field operations when error != std::ptr::null_mut(), otherwise skip them; keep the macro's return behavior the same so functions like managed_wallet_get_utxos can continue to use deref_ptr!(managed_info, error) safely when callers supply a null error pointer.key-wallet-ffi/src/keys_tests.rs (1)
109-133:⚠️ Potential issue | 🟠 MajorKeep the xpriv-blocking regression test active.
This test verifies that account private-key extraction remains unavailable; marking it
#[ignore]removes coverage for a security-sensitive FFI behavior. If the expected error changed, update the assertion instead of ignoring the test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/keys_tests.rs` around lines 109 - 133, Re-enable the regression test by removing the #[ignore] on test_wallet_get_account_xpriv_not_implemented so the xpriv-blocking behavior stays covered; locate the test function named test_wallet_get_account_xpriv_not_implemented and the FFI call wallet_get_account_xpriv and ensure the assertions match current behavior (if the FFI now returns a different error code or non-null pointer, update the assert!(xpriv_str.is_null()) and assert_eq!(error.code, FFIErrorCode::WalletError) to the correct expected outcome instead of silencing the test).key-wallet-ffi/src/wallet.rs (1)
118-127:⚠️ Potential issue | 🔴 CriticalUse null-pointer checks without dereferencing for raw buffers passed to FFI functions.
The
deref_ptr!macro creates an unsafe reference via&*ptr, which assumes the pointer targets valid initialized memory. At line 118,seedis dereferenced before validatingseed_len == 64, violating the FFI contract that seed must point to a valid 64-byte buffer. Similarly, at line 225,id_outis dereferenced beforecopy_nonoverlappingwrites 32 bytes to it, without validating the target buffer size.For raw buffers where operations like
from_raw_partsorcopy_nonoverlappingfollow, use anis_null()-only check to avoid dereferencing until buffer validity is confirmed:Safer approach
- let _seed_byte = deref_ptr!(seed, error); // null-check only; we want the raw ptr below + if seed.is_null() { + (*error).set(FFIErrorCode::InvalidInput, "seed ptr is null"); + return ptr::null_mut(); + } if seed_len != 64 { (*error).set( FFIErrorCode::InvalidInput, &format!("Invalid seed length: {}, expected 64", seed_len), ); return ptr::null_mut(); }let wallet = deref_ptr!(wallet, error); - let _ = deref_ptr!(id_out, error); // null-check only; raw ptr used for write + if id_out.is_null() { + (*error).set(FFIErrorCode::InvalidInput, "id_out ptr is null"); + return false; + } let wallet_id = wallet.inner().wallet_id;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/wallet.rs` around lines 118 - 127, The code currently uses deref_ptr! (which performs &*ptr) on FFI buffers like seed and id_out before validating sizes, risking undefined behavior; change those checks to only test for null via is_null() for seed and id_out, then validate seed_len == 64 (and destination capacity for id_out) before creating a slice with slice::from_raw_parts or performing ptr::copy_nonoverlapping; specifically, remove or replace deref_ptr!(seed, error) and deref_ptr!(id_out, error) with null-pointer checks, set the FFI error on invalid lengths/sizes, and only then safely call slice::from_raw_parts(seed, seed_len) or copy_nonoverlapping into id_out.key-wallet-ffi/src/error.rs (1)
136-151:⚠️ Potential issue | 🟡 MinorAdd explicit unsafe blocks around unsafe function calls in unsafe context.
The crate uses Rust 2021 edition, but the coding guidelines specify following 2024 idioms. Unsafe calls within unsafe functions should be wrapped in explicit
unsafeblocks for clarity and future compatibility. Lines 137 and 175 callclean()without explicit unsafe blocks, and line 348 callsCString::from_raw()without an explicit unsafe block (line 151 is already correct).🔧 Proposed fix
pub unsafe fn clean(&mut self) { - self.free_message(); + unsafe { + self.free_message(); + } self.code = FFIErrorCode::Success; self.message = ptr::null_mut(); }unsafe fn map_to_ffi_err(self, error: &mut FFIError) -> Option<T> { - error.clean(); + unsafe { + error.clean(); + } self.map_to_ffi_err_impl(error) }pub unsafe extern "C" fn error_message_free(message: *mut c_char) { if !message.is_null() { - let _ = CString::from_raw(message); + let _ = unsafe { CString::from_raw(message) }; } }Also applies to: 174-175, 346-348
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/error.rs` around lines 136 - 151, Calls to unsafe functions inside unsafe functions must be wrapped in explicit unsafe blocks: update call sites of clean() and free_message() to use explicit unsafe { self.clean() } / unsafe { self.free_message() } and wrap the CString::from_raw(...) call in an explicit unsafe block (e.g., let _ = unsafe { CString::from_raw(self.message) };). Locate these by referencing the methods clean(), free_message(), and the CString::from_raw invocation and add the explicit unsafe { ... } around each unsafe call.
🟡 Minor comments (1)
key-wallet-ffi/src/wallet_manager_tests.rs-25-28 (1)
25-28:⚠️ Potential issue | 🟡 MinorReorder assertions: check manager is non-null before dereferencing it.
The test dereferences
manageron line 26 before the null assertion on line 28. Sincewallet_manager_createcan return null on runtime allocation failures, this creates undefined behavior risk. Move the null check first:🧪 Suggested test fix
let manager = unsafe { wallet_manager::wallet_manager_create(FFINetwork::Testnet, error) }; -assert_eq!(unsafe { (*manager).network() }, FFINetwork::Testnet); - assert!(!manager.is_null()); +assert_eq!(unsafe { (*manager).network() }, FFINetwork::Testnet); assert_eq!(unsafe { (*error).code }, FFIErrorCode::Success);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/wallet_manager_tests.rs` around lines 25 - 28, The test currently dereferences manager before checking for null which can UB; update the test to assert that manager is non-null immediately after creation (assert!(!manager.is_null())) and only then call unsafe { (*manager).network() } to compare with FFINetwork::Testnet; locate the creation call wallet_manager::wallet_manager_create and the dereference expression (*manager).network() and reorder so the null-check precedes any dereferencing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/src/account_derivation.rs`:
- Around line 162-169: The EdDSA and generic derivation functions
(eddsa_account_derive_private_key_from_seed,
account_derive_extended_private_key_from_seed,
account_derive_private_key_from_seed) are creating a slice from untrusted seed
and need the same seed_len bounds check used by the BLS path; before calling
std::slice::from_raw_parts(seed, seed_len) in each function (where
deref_ptr!(seed, error) and the seed_slice variable are used), validate seed_len
is within the allowed range (e.g. 1..=64), return the same error path on invalid
length, and only then build seed_slice and proceed to call
account.inner().derive_from_seed_private_key_at / related methods so you avoid
out-of-bounds memory access.
- Around line 38-52: The code dereferences the FFI error pointer unconditionally
(via deref_ptr!, unwrap_or_return!, and direct (*error).set(...)), which breaks
the documented nullable contract; fix by first checking if error.is_null() at
the top of each affected FFI function (e.g.,
account_derive_extended_private_key_at,
bls_account_derive_private_key_from_seed,
bls_account_derive_private_key_from_mnemonic and the other listed functions) and
if null, return ptr::null_mut() (or appropriate null) immediately, or
alternatively update the helper macros to accept Option<*mut FFIError> and
safely skip setting the error when the pointer is null; ensure any call sites
that currently assume non-null behavior are updated so no dereference occurs
unless the pointer has been validated.
In `@key-wallet-ffi/src/address_pool.rs`:
- Around line 294-295: The code currently uses deref_ptr!(info_out, error) (and
similarly for count_out) which creates Rust references to caller-owned
uninitialized out-parameters; instead keep deref_ptr!(managed_wallet, error) for
the input pointer but replace deref_ptr! checks on out-parameters with a
null-check-only helper (e.g., ensure_nonnull_ptr(info_out, error) /
ensure_nonnull_ptr(count_out, error)) that only tests for null and returns the
error without creating a reference or dereferencing the storage; apply the same
replacement wherever deref_ptr! is used on out-parameters (including the other
occurrence around the 798-801 area).
- Around line 294-295: The code currently unconditionally dereferences the FFI
error pointer via the deref_ptr! macro (e.g., deref_ptr!(managed_wallet, error);
deref_ptr!(info_out, error)), which contradicts docs that allow error to be
nullable; update the FFI contract or make dereferencing safe: either change the
function signatures to require a non-null error pointer across the API, or add a
null-check before every deref_ptr! usage and, if error is null, avoid writing to
it and return/report an appropriate failure code. Locate uses of deref_ptr!
(e.g., around managed_wallet, info_out and other spots mentioned) and implement
one consistent approach—preferably add a safe helper that checks if
error.is_null() and only writes/uses it when non-null, or mark the parameter as
non-null in the FFI surface and validate at entry—so all calls (including the
ones referenced) no longer perform unconditional dereferences.
In `@key-wallet-ffi/src/bip38.rs`:
- Around line 16-22: The exported FFI functions (e.g., bip38_encrypt_private_key
and the similar bip38_decrypt_private_key) currently call unimplemented!() which
panics across the C boundary; replace the panic by returning ptr::null_mut()
and, if the _error pointer is non-null, populate it with an appropriate
WalletError/NotImplemented value (via the FFIError wrapper used across this
crate), ensuring you do not dereference null pointers and follow existing
error-construction helpers used elsewhere in the module; keep the function
signature and lifetime behavior the same and avoid any panics or unwraps so the
C caller receives NULL plus a filled FFIError instead of a process abort.
In `@key-wallet-ffi/src/derivation.rs`:
- Around line 53-54: The code currently uses the deref_ptr! macro to null-check
FFI buffers (e.g., seed/seed_len and fingerprint_out) which creates a Rust
reference and is unsafe for output or uninitialized writable memory; replace
those deref_ptr! checks with a non-dereferencing pointer-check helper (a simple
null/check length helper) for seed/seed_len and for path_out/fingerprint_out,
then perform the actual slice::from_raw_parts or write-to-pointer operations
explicitly using raw pointers (seed -> slice::from_raw_parts(seed, seed_len) and
fingerprint_out via ptr::write or slice::from_raw_parts_mut) to avoid creating
temporary Rust references to potentially uninitialized buffers and to keep FFI
boundary handling safe.
- Around line 333-343: Wrap the individual unsafe operations in explicit unsafe
blocks: call slice::from_raw_parts(seed, seed_len) inside unsafe { ... }, call
CStr::from_ptr(path) (and its to_str()) inside unsafe { ... }, and wrap the
ptr::copy_nonoverlapping usage referenced in this function in an unsafe { ... }
block as well; keep the surrounding function signature (unsafe extern "C")
unchanged and only add scoped unsafe { } around these operations to match the
existing pattern used elsewhere (refer to identifiers seed, seed_len, path,
slice::from_raw_parts, CStr::from_ptr, ptr::copy_nonoverlapping, and the derived
values path_str/derivation_path).
In `@key-wallet-ffi/src/error.rs`:
- Around line 126-128: The set() method currently overwrites self.message
causing leaked CStrings; modify set (or make it unsafe/private) to free the
previous allocation before replacing it: if self.message is non-null reconstruct
a CString via CString::from_raw(self.message) to let it drop, then assign the
new CString::new(msg).unwrap_or_default().into_raw(); alternatively make set
private and require callers to call clean() (or provide a documented
reset/reset_and_set FFI-safe function) so ownership and freeing are explicit;
reference the set method, self.message field, and clean() when implementing the
chosen approach.
- Around line 198-203: The code uses panic-prone CString::new(...).unwrap() (and
nested unwrap_or(...).unwrap()) in FFI error conversions (e.g., impl<T>
FfiErrMapper<T> for Option<T> method map_to_ffi_err_impl and other
FfiErrMapper/mapper implementations), so add a small non-panicking helper (e.g.,
safe_cstring_into_raw or similar) that takes &str and returns *mut c_char by
handling interior NULs and allocation failures safely, then replace every
CString::new(...).unwrap() and unwrap_or(CString::new(...).unwrap()) in all
error conversion blocks with calls to that helper (update assignments to
err.message to use the helper) so no code path can panic at the FFI boundary.
- Around line 12-95: The macros deref_ptr!, deref_ptr_mut! and unwrap_or_return!
are dereferencing raw error pointers without null checks or explicit unsafe
blocks; update each macro to first check if $error.is_null() and only perform
(*$error).clean() and (*$error).set(...) inside an explicit unsafe { ... } when
non-null, and similarly wrap &mut *$error passed to FfiErrMapper::map_to_ffi_err
in an unsafe block; if $error is null, skip writing into it (for
unwrap_or_return! call FfiErrMapper::map_to_ffi_err with a temporary stack
FFIError and ignore its write) so FFI callers can pass null to opt out of error
reporting. Ensure changes are applied to both overloads of deref_ptr!,
deref_ptr_mut!, and both arms of unwrap_or_return!.
- Around line 213-238: In the From<Error> (and From<WalletError>)
implementation, the match currently consumes the non-Copy error (match value)
causing value to be moved before calling value.to_string(); change the match to
use a reference (match &value) so the error is not moved, leaving value
available for FFIError { message: CString::new(value.to_string())... }; update
the match patterns accordingly (e.g., Error::InvalidDerivationPath(_) becomes
Error::InvalidDerivationPath(_) when matching a reference) in the functions that
construct FFIError.
In `@key-wallet-ffi/src/keys.rs`:
- Around line 82-91: The exported FFI path currently reaches
unimplemented!("Private key extraction not implemented") which will panic across
the extern "C" boundary; replace that unreachable panic by setting the provided
error pointer (same pattern used earlier with (*error).set(...)) to a suitable
FFI error (e.g., FFIErrorCode::NotImplemented or FFIErrorCode::Unknown) with a
clear message like "Private key extraction not implemented" and then return
ptr::null_mut(); locate this change around the symbols wallet, account,
deref_ptr!, and unwrap_or_return! in keys.rs to ensure the function returns NULL
and sets the error instead of calling unimplemented!.
In `@key-wallet-ffi/src/managed_wallet.rs`:
- Around line 213-216: The code currently uses deref_ptr! on out-parameters
(addresses_out, count_out and other balance output pointers) which may be
uninitialized; instead, validate those raw pointers with .is_null() checks and
return an error if null, and then write results into them using pointer writes
(e.g., std::ptr::write or std::ptr::write_unaligned) rather than creating
references. Replace deref_ptr!(addresses_out, error) and deref_ptr!(count_out,
error) (and the analogous uses at the 440-445 region for balance outputs) with
explicit null checks, then perform unsafe pointer writes to set the output
values; continue to use deref_ptr_mut! only for managed_wallet if that pointer
must be a mutable reference. Ensure all FFI out-params are validated and written
to without constructing &T references to uninitialized memory.
In `@key-wallet-ffi/src/mnemonic.rs`:
- Around line 208-231: The null-check for output pointers seed_out and seed_len
currently uses deref_ptr! which dereferences them (unsafe for uninitialized
write-only outputs); change those checks to use seed_out.is_null() and
seed_len.is_null() (no dereference) and return the same error if null, then keep
the later unsafe block to write to *seed_len and copy into seed_out; ensure
deref_ptr! stays only for input pointers like mnemonic and passphrase, and
remove deref_ptr! uses for seed_out/seed_len to avoid reading uninitialized
memory.
In `@key-wallet-ffi/src/transaction_checking.rs`:
- Around line 113-119: The current code dereferences the out-parameter
`result_out` via `deref_ptr!(result_out, error)` which can create a reference to
uninitialized caller memory; instead perform a null-only check (e.g., if
result_out.is_null() return error) and remove the `deref_ptr!` call, then later
when you have the computed result write into the caller storage using a raw
write (e.g., `ptr::write(result_out, value)` or equivalent) so you never form a
Rust reference to uninitialized memory; apply this change in the same block
where `managed_wallet`, `tx_bytes`, `tx_len`, and
`Transaction::consensus_decode` are used so `result_out` is only validated for
nullity and only written after the transaction is decoded and final value is
ready.
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 102-108: The current FFI handling uses deref_ptr!(...) on
out-parameters (fee_out, tx_bytes_out, tx_len_out and similarly result_out,
private_keys_out) which unsafely creates Rust references before those C slots
are initialized; change those checks to null-only validation (use .is_null() on
the raw pointers) for all out-params and only call deref_ptr! or create Rust
references after you have allocated/initialized the output memory or written
values. Locate usages around manager_ref/wallet_ref deref (the deref_ptr! calls
near manager, wallet) and in the other flagged regions (around lines where
fee_out, tx_bytes_out, tx_len_out, result_out, private_keys_out are deref'd) and
replace immediate dereferencing with null checks, then defer actual deref/writes
until after safe initialization.
In `@key-wallet-ffi/src/wallet_manager.rs`:
- Around line 143-150: The helper macros (deref_ptr!, unwrap_or_return!) and
several callsites (e.g., the mnemonic/passphrase block using CStr::from_ptr,
direct (*error) writes) assume a non-null FF IError pointer and will crash when
error is null; make the error pointer contract null-safe by checking
error.is_null() before dereferencing and only writing to it when present.
Concretely: update callers like the mnemonic/passphrase parsing block (and the
other listed regions) to obtain an Option<&mut FFIError> via let error_opt = if
error.is_null() { None } else { Some(unsafe { &mut *error }) }, replace direct
deref_ptr!/(*error) usages to use error_opt, and change unwrap_or_return!
behavior so it returns fallback values without dereferencing when error_opt is
None and only sets error fields when Some(error_ref). Alternatively, modify the
macros deref_ptr! and unwrap_or_return! to accept a nullable error pointer and
perform the same guard internally; ensure all writes to FFIError happen only
when error is non-null to preserve the documented null contract.
- Around line 226-230: The code is unsafely turning output destination pointers
into Rust references via deref_ptr! (e.g., wallet_bytes_out,
wallet_bytes_len_out, wallet_id_out) during validation; instead, perform a
null-check on these raw pointers (ptr.is_null()) without creating references,
keep using the raw pointers for later writes, and only convert to references or
write through them at the point you actually allocate/copy data (inside the same
unsafe block) using ptr::write or slice::from_raw_parts_mut as appropriate;
replace calls to deref_ptr! for those *_out slots with simple null checks and
leave manager and mnemonic deref_ptr! usage unchanged where they are actual
inputs. This same change should be applied to the other occurrences mentioned
(the other *_out parameter checks referenced in the review).
In `@key-wallet-ffi/src/wallet.rs`:
- Around line 42-43: The macros deref_ptr! and unwrap_or_return! currently
unconditionally dereference the error pointer causing UB when NULL; update these
macros to first check whether the incoming error pointer is null and only
perform (*error).clean() or &mut *error when non-null, returning an appropriate
error code when needed, and adjust all callers (e.g.,
wallet_create_from_seed_with_options and other FFI functions using deref_ptr! /
unwrap_or_return!) to rely on the null-tolerant macros; alternatively, if opting
to require a non-null error pointer, update the public API/docs (FFI_API.md) and
add an explicit non-null assert at FFI entry points instead of dereferencing
unconditionally. Ensure the fixes reference the macros deref_ptr! and
unwrap_or_return! and the function wallet_create_from_seed_with_options so
reviewers can locate changes.
---
Outside diff comments:
In `@key-wallet-ffi/src/account_collection.rs`:
- Around line 85-92: The function wallet_get_account_collection calls
deref_ptr!(wallet, error) which unconditionally uses (*error).clean() and
therefore UB when callers pass error = null as the public API allows; fix by
handling a nullable error pointer instead of using the macro that assumes
non-null: in wallet_get_account_collection, check if error.is_null() before
invoking any error-clean/assign behavior and only call deref_ptr!-style
validation for wallet when a non-null error is present, or update the deref_ptr!
macro to perform a null-check on the error pointer before touching
(*error).clean(); ensure references to FFIError, deref_ptr!,
wallet_get_account_collection and FFIAccountCollection::new are preserved and
that the function still returns the newly boxed FFIAccountCollection on success.
In `@key-wallet-ffi/src/account.rs`:
- Around line 546-566: The deref_ptr! / deref_ptr_mut! macros currently call
(*$error).clean() before checking whether error is null, violating the FFI
safety contract used by functions like wallet_get_account_count and
wallet_get_account_collection; update the macros in error.rs so they first check
if $error.is_null() and only call (*$error).clean() when non-null (apply same
pattern to both mutable and 2-argument variants), ensuring all callers that pass
error = null do not cause an unconditional dereference of FFIError.
In `@key-wallet-ffi/src/error.rs`:
- Around line 136-151: Calls to unsafe functions inside unsafe functions must be
wrapped in explicit unsafe blocks: update call sites of clean() and
free_message() to use explicit unsafe { self.clean() } / unsafe {
self.free_message() } and wrap the CString::from_raw(...) call in an explicit
unsafe block (e.g., let _ = unsafe { CString::from_raw(self.message) };). Locate
these by referencing the methods clean(), free_message(), and the
CString::from_raw invocation and add the explicit unsafe { ... } around each
unsafe call.
In `@key-wallet-ffi/src/keys_tests.rs`:
- Around line 109-133: Re-enable the regression test by removing the #[ignore]
on test_wallet_get_account_xpriv_not_implemented so the xpriv-blocking behavior
stays covered; locate the test function named
test_wallet_get_account_xpriv_not_implemented and the FFI call
wallet_get_account_xpriv and ensure the assertions match current behavior (if
the FFI now returns a different error code or non-null pointer, update the
assert!(xpriv_str.is_null()) and assert_eq!(error.code,
FFIErrorCode::WalletError) to the correct expected outcome instead of silencing
the test).
In `@key-wallet-ffi/src/managed_account_collection.rs`:
- Around line 92-101: The function managed_wallet_get_account_collection
currently calls the deref_ptr! macro which unconditionally dereferences and
calls (*error).clean(), causing UB if the caller passed a null error pointer;
change the implementation so the error out-param is null-safe: before invoking
deref_ptr! ensure you only dereference/clean the error when error !=
std::ptr::null_mut(), or update/replace the macro with a null-aware variant that
checks error for null and only calls (*error).clean() when non-null; keep all
other validations for manager and wallet_id intact and continue to document
error as optional (FFIError or null).
In `@key-wallet-ffi/src/managed_account.rs`:
- Around line 204-218: The failure branch after calling
wallet_manager_get_managed_wallet_info currently copies error.message into the
FFIManagedCoreAccountResult but never frees the original FFIError::message,
leaking memory; update the branch so that after you convert error.message (via
CStr::from_ptr and to_string_lossy) you free the original C string (e.g., call
the crate's FFI string free helper or libc::free on error.message) and null out
error.message before returning; apply the same change to the other failure paths
mentioned (the analogous blocks around the calls producing FFIError at the other
locations) so each temporary FFIError.message is freed after copying.
- Around line 924-933: The function managed_wallet_get_account_count currently
calls the deref_ptr! macro with the error pointer even though the API documents
error may be null; because deref_ptr! calls (*error).clean() it will dereference
null and UB. Fix by null-checking the error pointer before any deref_ptr!
invocation (e.g., validate error != std::ptr::null_mut() and only call
deref_ptr! when non-null), or alternatively change the ABI/signature to require
a non-null *mut FFIError and update callers; ensure references to deref_ptr!,
FFIError, and managed_wallet_get_account_count are updated consistently.
In `@key-wallet-ffi/src/utxo.rs`:
- Around line 85-97: The deref_ptr! macro currently dereferences the error
pointer unconditionally (calling (*$error).clean()), violating the documented
nullable semantics and causing UB when error is null (affects
managed_wallet_get_utxos, wallet_get_utxos, and all FFI callers). Update
deref_ptr! so it first checks if the provided error pointer is non-null before
calling (*error).clean() or (*error).set(...); only perform those error-field
operations when error != std::ptr::null_mut(), otherwise skip them; keep the
macro's return behavior the same so functions like managed_wallet_get_utxos can
continue to use deref_ptr!(managed_info, error) safely when callers supply a
null error pointer.
In `@key-wallet-ffi/src/wallet.rs`:
- Around line 118-127: The code currently uses deref_ptr! (which performs &*ptr)
on FFI buffers like seed and id_out before validating sizes, risking undefined
behavior; change those checks to only test for null via is_null() for seed and
id_out, then validate seed_len == 64 (and destination capacity for id_out)
before creating a slice with slice::from_raw_parts or performing
ptr::copy_nonoverlapping; specifically, remove or replace deref_ptr!(seed,
error) and deref_ptr!(id_out, error) with null-pointer checks, set the FFI error
on invalid lengths/sizes, and only then safely call slice::from_raw_parts(seed,
seed_len) or copy_nonoverlapping into id_out.
---
Minor comments:
In `@key-wallet-ffi/src/wallet_manager_tests.rs`:
- Around line 25-28: The test currently dereferences manager before checking for
null which can UB; update the test to assert that manager is non-null
immediately after creation (assert!(!manager.is_null())) and only then call
unsafe { (*manager).network() } to compare with FFINetwork::Testnet; locate the
creation call wallet_manager::wallet_manager_create and the dereference
expression (*manager).network() and reorder so the null-check precedes any
dereferencing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a350259a-9048-4346-8030-a070b1722eb7
📒 Files selected for processing (40)
dash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/tests/dashd_sync/context.rsdash-spv-ffi/tests/test_wallet_manager.rskey-wallet-ffi/FFI_API.mdkey-wallet-ffi/src/account.rskey-wallet-ffi/src/account_collection.rskey-wallet-ffi/src/account_derivation.rskey-wallet-ffi/src/account_derivation_tests.rskey-wallet-ffi/src/account_tests.rskey-wallet-ffi/src/address.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/address_tests.rskey-wallet-ffi/src/bip38.rskey-wallet-ffi/src/derivation.rskey-wallet-ffi/src/derivation_tests.rskey-wallet-ffi/src/error.rskey-wallet-ffi/src/keys.rskey-wallet-ffi/src/keys_tests.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/managed_account_collection.rskey-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/managed_wallet_tests.rskey-wallet-ffi/src/mnemonic.rskey-wallet-ffi/src/mnemonic_tests.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/transaction_checking.rskey-wallet-ffi/src/utxo.rskey-wallet-ffi/src/utxo_tests.rskey-wallet-ffi/src/wallet.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_serialization_tests.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/src/wallet_tests.rskey-wallet-ffi/tests/debug_wallet_add.rskey-wallet-ffi/tests/integration_test.rskey-wallet-ffi/tests/test_addr_simple.rskey-wallet-ffi/tests/test_error_conversions.rskey-wallet-ffi/tests/test_import_wallet.rskey-wallet-ffi/tests/test_managed_account_collection.rskey-wallet-ffi/tests/test_passphrase_wallets.rs
ZocoLini
force-pushed
the
chore/ffi-error-handling
branch
from
5521e4b to
3c49bb0
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #670 +/- ##
=============================================
+ Coverage 68.39% 69.73% +1.34%
=============================================
Files 320 320
Lines 68023 66641 -1382
=============================================
- Hits 46524 46472 -52
+ Misses 21499 20169 -1330
|
ZocoLini
force-pushed
the
chore/ffi-error-handling
branch
from
3c49bb0 to
999e6b0
Compare
ZocoLini
force-pushed
the
chore/ffi-error-handling
branch
from
999e6b0 to
f091181
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
key-wallet-ffi/src/account.rs (1)
546-566:⚠️ Potential issue | 🟡 MinorStale safety doc:
erroris no longer allowed to be null.Line 551 still reads "
errormust be a valid pointer to an FFIError structure or null", butderef_ptr!(wallet, error)at line 558 unconditionally writes througherroron the null-wallet path per the refactor's contract. Drop the "or null" to match the rest of the PR's updated docs.🛠️ Proposed doc fix
-/// - `error` must be a valid pointer to an FFIError structure or null +/// - `error` must be a valid pointer to an FFIError structureBased on learnings: the
error: *mut FFIErrorparameter in FFI functions is required to be non-null, documented as a precondition in the# Safetysection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/account.rs` around lines 546 - 566, The safety doc for wallet_get_account_count is stale: update the `# Safety` section to reflect that the `error: *mut FFIError` parameter must be a valid, non-null pointer (remove "or null") because deref_ptr!(wallet, error) unconditionally writes through `error`; ensure the sentence referencing `error` matches this contract and mentions it must remain valid for the duration of the call (refer to wallet_get_account_count and the deref_ptr! usage).key-wallet-ffi/src/wallet.rs (1)
111-141:⚠️ Potential issue | 🟡 MinorUse
check_ptr!instead ofderef_ptr!for theseedbuffer.
_seed_byteis unused — the call is just a null check — butderef_ptr!synthesizes&*seedbeforeseed_lenis validated. If a caller passes a non-null but otherwise-invalid pointer withseed_len == 0(a common C idiom for "empty buffer"), that reference is UB before we reach the length check.derivation::derivation_new_master_key(derivation.rs:53) already usescheck_ptr!(seed, error)for the identical case — let's match that here.🛠️ Proposed fix
- let _seed_byte = deref_ptr!(seed, error); + check_ptr!(seed, error); if seed_len != 64 {You'll also need to add
check_ptrto theuse crate::{...}import at line 19.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/wallet.rs` around lines 111 - 141, In wallet_create_from_seed_with_options replace the initial deref_ptr!(seed, error) call with check_ptr!(seed, error) so we only null-check the pointer before validating seed_len (avoid UB from creating a reference to seed prior to the length check), remove the unused _seed_byte binding, and add check_ptr to the crate import list (where deref_ptr is imported) so the macro is available.
🧹 Nitpick comments (6)
key-wallet-ffi/src/transaction.rs (1)
948-967: Usederef_ptr!formanagerandwalletto matchwallet_build_and_sign_transaction.This function guards
manager/walletwithcheck_ptr!at lines 948–949 and then manually dereferences them at lines 966–967 (&*manager,&*wallet). The siblingwallet_build_and_sign_transactionat lines 102–103 instead usesderef_ptr!(manager, error)/deref_ptr!(wallet, error). Both are correct, but the inconsistency hurts readability for a PR explicitly aimed at centralizing this pattern.♻️ Suggested alignment
- check_ptr!(manager, error); - check_ptr!(wallet, error); - check_ptr!(funding_types, error); + let manager_ref = deref_ptr!(manager, error); + let wallet_ref = deref_ptr!(wallet, error); + check_ptr!(funding_types, error); ... - unsafe { - let manager_ref = &*manager; - let wallet_ref = &*wallet; - - let scripts_slice = ... + unsafe { + let scripts_slice = ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/transaction.rs` around lines 948 - 967, The code manually dereferences manager and wallet with unsafe let manager_ref = &*manager and let wallet_ref = &*wallet after using check_ptr!, which is inconsistent with the existing pattern in wallet_build_and_sign_transaction; replace those manual dereferences with the helper macro calls deref_ptr!(manager, error) and deref_ptr!(wallet, error) to match the established pattern and ensure consistent error handling and safety; keep the rest of the unsafe block unchanged and use the returned references (manager_ref, wallet_ref) as before.key-wallet-ffi/src/error.rs (1)
224-233: MakeOption<T>impl consistent with theResult<T,E>impl.Two minor issues in the
Option<T>mapper:
- It mutates
err.code/err.messagedirectly instead of using the*err = FFIError { … }pattern used for theResultimpl. TheResultpath benefits from theDropimpl automatically freeing a previously-set message on replacement; this path does not. It happens to be safe today only becausemap_to_ffi_errcallserror.clean()first, butmap_to_ffi_err_implis a public trait method and a direct caller would leak any pre-existing message.CString::new("Item not found").unwrap()is a rawunwrapin library code. Contradicts theunwrap_or(...)fallback pattern used in theFrom<…>impls below, and the crate-wide guideline to avoidunwrap()in library code.🧹 Suggested refactor
impl<T> FfiErrMapper<T> for Option<T> { fn map_to_ffi_err_impl(self, err: &mut FFIError) -> Option<T> { if self.is_none() { - err.code = FFIErrorCode::NotFound; - err.message = CString::new("Item not found").unwrap().into_raw(); + *err = FFIError { + code: FFIErrorCode::NotFound, + message: CString::new("Item not found") + .unwrap_or_default() + .into_raw(), + }; } - self } }As per coding guidelines: "Avoid
unwrap()andexpect()in library code; use proper error types".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/error.rs` around lines 224 - 233, The Option<T> FfiErrMapper impl should mirror the Result<T,E> pattern: instead of mutating err.code/err.message in place, replace the whole FFIError with *err = FFIError { code: FFIErrorCode::NotFound, message: ... } so the Drop impl will free any prior message; build the message without using unwrap by calling CString::new("Item not found") and handling the Result (e.g. map to into_raw() on Ok and fall back to a null pointer or alternative CString on Err), then return self from map_to_ffi_err_impl; update the impl for Option<T> (map_to_ffi_err_impl) and use FFIError / FFIErrorCode symbols referenced above.key-wallet-ffi/tests/test_account_collection.rs (1)
153-166: Nit: renametesttoerrorfor consistency.Elsewhere (comprehensive test, line 19) the variable is named
error. Usingtesthere is just confusing when it's passed as theerrorout-parameter.🪶 Proposed rename
- let test = &mut FFIError::default(); + let error = &mut FFIError::default(); ... - test, + error, ... - let collection = wallet_get_account_collection(wallet, test); + let collection = wallet_get_account_collection(wallet, error);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/tests/test_account_collection.rs` around lines 153 - 166, Rename the local variable FFIError::default() currently named `test` to `error` for clarity and consistency with other tests; update its declaration and all uses where it's passed as the out-parameter to wallet_create_from_mnemonic_with_options(...) and wallet_get_account_collection(...), so the variable name `error` is used instead of `test`.key-wallet-ffi/src/derivation.rs (1)
323-345: Minor: wrap raw-pointer ops in explicitunsafe { }blocks for consistency.
slice::from_raw_parts(seed, seed_len)(line 336) andCStr::from_ptr(path)(line 338) aren't wrapped inunsafe { }blocks here, while the other exported functions in this same file do wrapptr::copy_nonoverlapping(lines 98, 141, 181, 221, 263, 309). Same inconsistency applies to line 415 (derivation_xpub_fingerprint) and line 54 (derivation_new_master_key). Low priority under Rust 2021, but the crate-wide mix is worth cleaning up — and it's required under the 2024 edition'sunsafe_op_in_unsafe_fnlint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/derivation.rs` around lines 323 - 345, The exported function derivation_derive_private_key_from_seed (and the other mentioned exported functions derivation_xpub_fingerprint and derivation_new_master_key) perform raw-pointer operations without explicit unsafe blocks; update these to wrap calls like slice::from_raw_parts(seed, seed_len) and CStr::from_ptr(path) in explicit unsafe { ... } blocks (and similarly wrap any ptr::copy_nonoverlapping/other raw ops in those other functions) so all raw-pointer operations are consistently inside unsafe blocks to satisfy the unsafe_op_in_unsafe_fn lint and match the rest of the file.key-wallet-ffi/src/wallet_manager.rs (1)
499-540: Minor:deref_ptr_mut!here is stronger than needed.
FFIWalletManager::managerisArc<RwLock<...>>, so the inner mutation uses.write().awaiton a shared&RwLock.deref_ptr!(manager, error)(immutable borrow) would suffice here and be consistent withwallet_manager_add_wallet_from_mnemonic_with_options(line 143), which also only reads theruntime/managerArcs. Non-blocking — just tightens the contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/wallet_manager.rs` around lines 499 - 540, In wallet_manager_process_transaction replace the stronger unsafe deref_ptr_mut!(manager, error) with the immutable deref_ptr!(manager, error) since the function only needs shared access to the Arc<RwLock<...>> (you still call manager_ref.runtime and manager_ref.manager.write().await); update the reference acquisition to use deref_ptr! so manager_ref remains an immutable borrow consistent with wallet_manager_add_wallet_from_mnemonic_with_options, leaving the rest of the logic and error handling unchanged.key-wallet-ffi/src/keys.rs (1)
77-93:wallet_get_account_xprivstill a stub — document publicly or gate behind a feature.The non-watch-only branch always returns
InternalError/ null. This is a safe stub (no panic), but from a C caller's perspective the exported symbol is advertised inFFI_API.mdas functional. Consider either adding a#[deprecated]/doc-warning note or aNotImplementederror code so clients can distinguish "not implemented yet" from genuine internal failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/keys.rs` around lines 77 - 93, In wallet_get_account_xpriv, don't return InternalError for the unimplemented branch; either (preferred) set the error to a distinct NotImplemented/NotAvailable code and message (e.g., (*error).set(FFIErrorCode::NotImplemented, "Private key extraction not implemented"), then return null) so C callers can distinguish "not implemented" from real failures, or gate the symbol behind a feature/flag (e.g., #[cfg(feature = "xpriv")] on wallet_get_account_xpriv) and update FFI_API.md to mark the function as behind that feature or as deprecated/documented-not-implemented; update references to wallet_get_account_xpriv and FFI_API.md accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/FFI_API.md`:
- Around line 500-503: The docs contradict the new invariant that exported
unsafe extern "C" functions require a non-null output `error: *mut FFIError`;
update all remaining `# Safety` blocks (including the "Create a new wallet
manager" section and the `wallet_get_account_count` documentation) to state that
`error` must be a valid, non-null pointer to an `FFIError`, ensure references to
freeing remain (e.g., `wallet_manager_free`), then regenerate the FFI_API.md
artifact in this PR so the generated file consistently documents the non-null
`error` precondition for functions like `wallet_get_account_count` and other
`unsafe extern "C"` exports.
In `@key-wallet-ffi/src/managed_wallet.rs`:
- Around line 213-216: The out-parameters addresses_out and count_out must be
cleared before any early-returning pointer validations so stale values aren’t
left when deref_ptr_mut!(managed_wallet) or deref_ptr!(wallet) fail; modify the
function to immediately null/zero the destinations referenced by addresses_out
and count_out (e.g., set the pointer to null and count to 0) before invoking
check_ptr! / deref_ptr_mut! / deref_ptr!, and apply the same change to the
analogous block around the other occurrence (the block referenced by lines
327-330) so both call paths always reset outputs on error paths.
In `@key-wallet-ffi/src/mnemonic.rs`:
- Around line 219-221: The code forces Language::English when calling
Mnemonic::from_phrase which breaks non-English mnemonics; change
mnemonic_to_seed to detect/attempt the correct language instead of hardcoding
English by trying Mnemonic::from_phrase with the supported languages (or using
the library's language-detection utility) and use the successful Mnemonic to
call to_seed; update the call site that currently uses
Mnemonic::from_phrase(mnemonic_str, Language::English) so it iterates Language
(or uses Mnemonic::from_phrase_without_language if available) and returns an
error only if no language parses the phrase.
---
Outside diff comments:
In `@key-wallet-ffi/src/account.rs`:
- Around line 546-566: The safety doc for wallet_get_account_count is stale:
update the `# Safety` section to reflect that the `error: *mut FFIError`
parameter must be a valid, non-null pointer (remove "or null") because
deref_ptr!(wallet, error) unconditionally writes through `error`; ensure the
sentence referencing `error` matches this contract and mentions it must remain
valid for the duration of the call (refer to wallet_get_account_count and the
deref_ptr! usage).
In `@key-wallet-ffi/src/wallet.rs`:
- Around line 111-141: In wallet_create_from_seed_with_options replace the
initial deref_ptr!(seed, error) call with check_ptr!(seed, error) so we only
null-check the pointer before validating seed_len (avoid UB from creating a
reference to seed prior to the length check), remove the unused _seed_byte
binding, and add check_ptr to the crate import list (where deref_ptr is
imported) so the macro is available.
---
Nitpick comments:
In `@key-wallet-ffi/src/derivation.rs`:
- Around line 323-345: The exported function
derivation_derive_private_key_from_seed (and the other mentioned exported
functions derivation_xpub_fingerprint and derivation_new_master_key) perform
raw-pointer operations without explicit unsafe blocks; update these to wrap
calls like slice::from_raw_parts(seed, seed_len) and CStr::from_ptr(path) in
explicit unsafe { ... } blocks (and similarly wrap any
ptr::copy_nonoverlapping/other raw ops in those other functions) so all
raw-pointer operations are consistently inside unsafe blocks to satisfy the
unsafe_op_in_unsafe_fn lint and match the rest of the file.
In `@key-wallet-ffi/src/error.rs`:
- Around line 224-233: The Option<T> FfiErrMapper impl should mirror the
Result<T,E> pattern: instead of mutating err.code/err.message in place, replace
the whole FFIError with *err = FFIError { code: FFIErrorCode::NotFound, message:
... } so the Drop impl will free any prior message; build the message without
using unwrap by calling CString::new("Item not found") and handling the Result
(e.g. map to into_raw() on Ok and fall back to a null pointer or alternative
CString on Err), then return self from map_to_ffi_err_impl; update the impl for
Option<T> (map_to_ffi_err_impl) and use FFIError / FFIErrorCode symbols
referenced above.
In `@key-wallet-ffi/src/keys.rs`:
- Around line 77-93: In wallet_get_account_xpriv, don't return InternalError for
the unimplemented branch; either (preferred) set the error to a distinct
NotImplemented/NotAvailable code and message (e.g.,
(*error).set(FFIErrorCode::NotImplemented, "Private key extraction not
implemented"), then return null) so C callers can distinguish "not implemented"
from real failures, or gate the symbol behind a feature/flag (e.g.,
#[cfg(feature = "xpriv")] on wallet_get_account_xpriv) and update FFI_API.md to
mark the function as behind that feature or as
deprecated/documented-not-implemented; update references to
wallet_get_account_xpriv and FFI_API.md accordingly.
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 948-967: The code manually dereferences manager and wallet with
unsafe let manager_ref = &*manager and let wallet_ref = &*wallet after using
check_ptr!, which is inconsistent with the existing pattern in
wallet_build_and_sign_transaction; replace those manual dereferences with the
helper macro calls deref_ptr!(manager, error) and deref_ptr!(wallet, error) to
match the established pattern and ensure consistent error handling and safety;
keep the rest of the unsafe block unchanged and use the returned references
(manager_ref, wallet_ref) as before.
In `@key-wallet-ffi/src/wallet_manager.rs`:
- Around line 499-540: In wallet_manager_process_transaction replace the
stronger unsafe deref_ptr_mut!(manager, error) with the immutable
deref_ptr!(manager, error) since the function only needs shared access to the
Arc<RwLock<...>> (you still call manager_ref.runtime and
manager_ref.manager.write().await); update the reference acquisition to use
deref_ptr! so manager_ref remains an immutable borrow consistent with
wallet_manager_add_wallet_from_mnemonic_with_options, leaving the rest of the
logic and error handling unchanged.
In `@key-wallet-ffi/tests/test_account_collection.rs`:
- Around line 153-166: Rename the local variable FFIError::default() currently
named `test` to `error` for clarity and consistency with other tests; update its
declaration and all uses where it's passed as the out-parameter to
wallet_create_from_mnemonic_with_options(...) and
wallet_get_account_collection(...), so the variable name `error` is used instead
of `test`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62a41aef-64af-4dcd-9cf5-d280356c4753
📒 Files selected for processing (41)
dash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/tests/dashd_sync/context.rsdash-spv-ffi/tests/test_wallet_manager.rskey-wallet-ffi/FFI_API.mdkey-wallet-ffi/src/account.rskey-wallet-ffi/src/account_collection.rskey-wallet-ffi/src/account_derivation.rskey-wallet-ffi/src/account_derivation_tests.rskey-wallet-ffi/src/account_tests.rskey-wallet-ffi/src/address.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/address_tests.rskey-wallet-ffi/src/bip38.rskey-wallet-ffi/src/derivation.rskey-wallet-ffi/src/derivation_tests.rskey-wallet-ffi/src/error.rskey-wallet-ffi/src/keys.rskey-wallet-ffi/src/keys_tests.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/managed_account_collection.rskey-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/managed_wallet_tests.rskey-wallet-ffi/src/mnemonic.rskey-wallet-ffi/src/mnemonic_tests.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/transaction_checking.rskey-wallet-ffi/src/utxo.rskey-wallet-ffi/src/utxo_tests.rskey-wallet-ffi/src/wallet.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_serialization_tests.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/src/wallet_tests.rskey-wallet-ffi/tests/debug_wallet_add.rskey-wallet-ffi/tests/integration_test.rskey-wallet-ffi/tests/test_account_collection.rskey-wallet-ffi/tests/test_addr_simple.rskey-wallet-ffi/tests/test_error_conversions.rskey-wallet-ffi/tests/test_import_wallet.rskey-wallet-ffi/tests/test_managed_account_collection.rskey-wallet-ffi/tests/test_passphrase_wallets.rs
✅ Files skipped from review due to trivial changes (3)
- key-wallet-ffi/src/wallet_manager_serialization_tests.rs
- key-wallet-ffi/src/managed_wallet_tests.rs
- key-wallet-ffi/src/account_collection.rs
🚧 Files skipped from review as they are similar to previous changes (20)
- key-wallet-ffi/tests/test_import_wallet.rs
- dash-spv-ffi/tests/test_wallet_manager.rs
- key-wallet-ffi/src/address_tests.rs
- key-wallet-ffi/src/wallet_tests.rs
- key-wallet-ffi/tests/integration_test.rs
- key-wallet-ffi/tests/debug_wallet_add.rs
- key-wallet-ffi/src/utxo_tests.rs
- key-wallet-ffi/src/account_tests.rs
- key-wallet-ffi/tests/test_addr_simple.rs
- dash-spv-ffi/tests/dashd_sync/context.rs
- key-wallet-ffi/src/wallet_manager_tests.rs
- key-wallet-ffi/src/managed_account_collection.rs
- key-wallet-ffi/src/keys_tests.rs
- key-wallet-ffi/src/derivation_tests.rs
- key-wallet-ffi/src/address.rs
- key-wallet-ffi/tests/test_error_conversions.rs
- key-wallet-ffi/src/managed_account.rs
- key-wallet-ffi/tests/test_passphrase_wallets.rs
- key-wallet-ffi/src/transaction_checking.rs
- key-wallet-ffi/src/account_derivation.rs
Comment on lines
+500
to
+503
| Create a new wallet manager # Safety `error` must be a valid pointer to an `FFIError`. The returned pointer must be freed with `wallet_manager_free`. | ||
|
|
||
| **Safety:** | ||
| `error` must be a valid pointer to an `FFIError`. The returned pointer must be freed with `wallet_manager_free`. |
Contributor
There was a problem hiding this comment.
Regenerate after updating the remaining error safety docs.
These sections now require a non-null error, but the same generated file still documents wallet_get_account_count as accepting error ... or null at Lines 1614-1617. For an FFI precondition that can lead to UB, that contradiction is risky for downstream callers; please update the remaining source # Safety docs and regenerate this artifact in the same PR.
Based on learnings: exported unsafe extern "C" functions use error: *mut FFIError as a required, non-null output parameter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet-ffi/FFI_API.md` around lines 500 - 503, The docs contradict the
new invariant that exported unsafe extern "C" functions require a non-null
output `error: *mut FFIError`; update all remaining `# Safety` blocks (including
the "Create a new wallet manager" section and the `wallet_get_account_count`
documentation) to state that `error` must be a valid, non-null pointer to an
`FFIError`, ensure references to freeing remain (e.g., `wallet_manager_free`),
then regenerate the FFI_API.md artifact in this PR so the generated file
consistently documents the non-null `error` precondition for functions like
`wallet_get_account_count` and other `unsafe extern "C"` exports.
Collaborator
Author
There was a problem hiding this comment.
give me the link to that comment
Contributor
There was a problem hiding this comment.
@ZocoLini, here is the link to that comment:
🧠 Learnings used
Learnt from: ZocoLini
Repo: dashpay/rust-dashcore PR: 670
File: key-wallet-ffi/src/error.rs:12-126
Timestamp: 2026-04-21T20:20:36.896Z
Learning: In the key-wallet-ffi crate (dashpay/rust-dashcore), the design decision for the `error: *mut FFIError` parameter in FFI functions is to require it to be non-null (documented as a precondition in the `# Safety` section). All exported `unsafe extern "C"` functions assume a valid, non-null `error` pointer. Callers must always pass a valid `&mut FFIError`. The macros `deref_ptr!`, `deref_ptr_mut!`, and `unwrap_or_return!` do not guard against null `error` pointers by design.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactor of out error framework in the key-wallet-ffi crate
Motivation
What I did
should eventually remove
Summary by CodeRabbit
Bug Fixes
Refactor
Documentation
Tests