chore(key-wallet-ffi): remove opaque types from FFI#629
chore(key-wallet-ffi): remove opaque types from FFI#629
Conversation
📝 WalkthroughWalkthroughFFI handle types across the codebase are systematically refactored from storing concrete Rust types to opaque Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #629 +/- ##
=============================================
+ Coverage 67.14% 67.82% +0.67%
=============================================
Files 320 318 -2
Lines 67424 67828 +404
=============================================
+ Hits 45273 46005 +732
+ Misses 22151 21823 -328
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
key-wallet-ffi/src/keys.rs (1)
377-393:⚠️ Potential issue | 🔴 CriticalReclaim the erased inner key objects in the free functions.
The new opaque wrappers own a second heap allocation in
inner(created viaBox::into_raw(Box::new(...))), but the free functions at lines 379, 392, 744, and 759 only drop the outerFFI*Keybox. This leaks all four key allocations, and for the private variants (private_key_freeandextended_private_key_free) it also leaves secret material resident in memory after the destructor returns, violating the requirement to never expose private keys.Proposed fix
pub unsafe extern "C" fn private_key_free(key: *mut FFIPrivateKey) { if !key.is_null() { - let _ = unsafe { Box::from_raw(key) }; + let key = unsafe { Box::from_raw(key) }; + if !key.inner.is_null() { + let _ = unsafe { Box::from_raw(key.inner as *mut secp256k1::SecretKey) }; + } } } pub unsafe extern "C" fn extended_private_key_free(key: *mut FFIExtendedPrivateKey) { if !key.is_null() { - let _ = unsafe { Box::from_raw(key) }; + let key = unsafe { Box::from_raw(key) }; + if !key.inner.is_null() { + let _ = unsafe { Box::from_raw(key.inner as *mut key_wallet::bip32::ExtendedPrivKey) }; + } } } pub unsafe extern "C" fn public_key_free(key: *mut FFIPublicKey) { if !key.is_null() { - unsafe { - let _ = Box::from_raw(key); - } + let key = unsafe { Box::from_raw(key) }; + if !key.inner.is_null() { + let _ = unsafe { Box::from_raw(key.inner as *mut secp256k1::PublicKey) }; + } } } pub unsafe extern "C" fn extended_public_key_free(key: *mut FFIExtendedPublicKey) { if !key.is_null() { - unsafe { - let _ = Box::from_raw(key); - } + let key = unsafe { Box::from_raw(key) }; + if !key.inner.is_null() { + let _ = unsafe { Box::from_raw(key.inner as *mut key_wallet::bip32::ExtendedPubKey) }; + } } }Also applies to: 741-760
🤖 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 377 - 393, The free functions (private_key_free, extended_private_key_free and their counterparts around the other indices) only drop the outer FFI*Key Box and leak the inner heap allocation (FFI*Key.inner), leaving secret material in memory; fix by: for each free function (e.g., private_key_free, extended_private_key_free, and the other two free functions), first check for null, reconstruct the outer Box from the raw pointer, then if outer.inner is a raw pointer to a heap allocation reconstruct and drop (and for private keys overwrite/zeroize the secret if applicable) that inner Box before letting the outer Box go out of scope so both allocations are reclaimed and secrets are cleared. Ensure you reference and use the FFIPrivateKey.inner / FFIExtendedPrivateKey.inner fields when freeing.key-wallet-ffi/src/derivation.rs (2)
45-50:⚠️ Potential issue | 🟠 MajorThis public FFI rename needs a breaking-change plan.
Changing these exported signatures from
FFIExtendedPrivKey/FFIExtendedPubKeytoFFIExtendedPrivateKey/FFIExtendedPublicKeychanges the generated C surface and will break existing bindings at compile time. Please either keep compatibility aliases for a transition release or ship this as an explicit breaking change instead of achore.As per coding guidelines, "Check whether the PR title prefix allowed in the pr-title.yml workflow accurately describes the changes."
Also applies to: 425-431, 501-508, 535-542, 577-584, 619-627
🤖 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 45 - 50, The exported FFI type/name changes (e.g., replacing FFIExtendedPrivKey/FFIExtendedPubKey with FFIExtendedPrivateKey/FFIExtendedPublicKey, as seen in the signature of derivation_new_master_key and the other renamed FFI functions) will break downstream C bindings; either restore backward-compatible aliases for the old symbols (typedefs and exported function wrappers that forward to the new types/functions) so both names exist for a transition release, or mark and publish this as an explicit breaking change in the PR title and changelog; update symbol references for derivation_new_master_key, its related derivation_* functions, and the FFI type declarations to implement aliases if choosing compatibility.
649-671:⚠️ Potential issue | 🔴 CriticalMemory leak:
derivation_xpriv_free()andderivation_xpub_free()do not free inner allocations.Both
derivation_xpriv_free()andderivation_xpub_free()callBox::from_raw()on the wrapper struct, which only drops the outerFFIExtendedPrivateKey/FFIExtendedPublicKeycontainer (a single pointer). The innerExtendedPrivKey/ExtendedPubKeyallocations—created viaBox::into_raw(Box::new(inner))infrom_inner()—are never freed because:
- The wrapper types have no
Dropimplementation- The free functions do not reconstruct and drop the inner raw pointer
Every call to
derivation_xpriv_free()orderivation_xpub_free()leaks memory. The fix is to either implementDropfor the wrappers that reconstructs and drops the inner pointer, or have the free functions explicitly recover and drop the inner allocation.🤖 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 649 - 671, The free functions derivation_xpriv_free and derivation_xpub_free currently only call Box::from_raw on the wrapper structs and thus leak the inner allocations; update them to reconstruct and drop the inner raw pointer stored inside FFIExtendedPrivateKey / FFIExtendedPublicKey (the pointer created by from_inner via Box::into_raw) before dropping the outer wrapper, or alternatively implement Drop for FFIExtendedPrivateKey and FFIExtendedPublicKey to recover and free the inner ExtendedPrivKey / ExtendedPubKey; ensure you locate the inner pointer field on FFIExtendedPrivateKey/FFIExtendedPublicKey, convert it back into a Box and let it drop, then convert the outer pointer back into a Box (Box::from_raw) so both inner and outer allocations are freed (apply this for both derivation_xpriv_free and derivation_xpub_free).
🧹 Nitpick comments (1)
key-wallet-ffi/src/types.rs (1)
145-149: Please call out this FFI contract change in the PR title/notes.Switching exported handles to raw
void*ownership wrappers changes the consumer-visible layout/lifetime contract, so this reads larger than a routinechore.As per coding guidelines, "Check whether the PR title prefix allowed in the pr-title.yml workflow accurately describes the changes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/types.rs` around lines 145 - 149, The PR changed the FFI handle layout by replacing the typed Arc<Wallet> wrapper with a raw void pointer in the FFIWallet struct (see FFIWallet and its inner field), which changes consumer-visible layout and ownership/lifetime semantics; update the PR title and description to explicitly call out this breaking FFI contract change (mention FFIWallet.inner, Arc<Wallet> -> *mut c_void swap), note the impact on ABI/lifetimes and consumers, and ensure the pr-title.yml prefix reflects a breaking or major change per the repo guidelines.
🤖 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 3127-3130: The safety doc incorrectly instructs consumers to free
the returned address pool with `[-]` instead of the actual destructor; update
the rustdoc/generator that produces the FFI documentation so the safety contract
for the function that returns the external address pool from an
FFIManagedCoreAccount (the function that returns the external/receive address
pool for Standard accounts) references the correct free function
`address_pool_free`, then regenerate FFl_API.md so the returned pool's release
instruction reads "The returned pool must be freed with `address_pool_free` when
no longer needed."
In `@key-wallet-ffi/src/account_collection.rs`:
- Around line 23-31: The FFI wrapper allocates a cloned AccountCollection into
inner (in FFIAccountCollection::new) but never frees that boxed clone, causing
leaks; implement Drop for FFIAccountCollection to reconstruct and drop the boxed
key_wallet::AccountCollection from the raw inner pointer (e.g., if self.inner !=
std::ptr::null_mut() then unsafe { Box::from_raw(self.inner as *mut
key_wallet::AccountCollection); } ), and ensure account_collection_free consumes
the FFIAccountCollection pointer via Box::from_raw to trigger Drop; update
inner() if needed to handle nulls safely.
In `@key-wallet-ffi/src/account.rs`:
- Around line 21-24: The FFIAccount::new creates a Box<Arc<...>> and stores its
raw pointer in FFIAccount.inner, but the corresponding free functions
(account_free, bls_account_free, eddsa_account_free) currently only drop the
outer wrapper and leak the boxed Arc; update each free path to reconstruct and
drop the original Box<Arc<...>> by casting inner back to *mut
Box<Arc<key_wallet::Account>> (or the appropriate concrete type) and letting it
fall out of scope, or alternatively implement Drop for FFIAccount (and the
bls/eddsa wrapper types) to perform the same Box::from_raw -> drop logic so the
heap-allocated Box<Arc<...>> is properly reclaimed.
In `@key-wallet-ffi/src/managed_account.rs`:
- Around line 872-873: The safety docs currently contain the placeholder '[-]'
as the destructor for the returned pool; replace that placeholder with the real
destructor name `address_pool_free` so callers know how to free the returned
pool (i.e., change the line "- The returned pool must be freed with `[-]` when
no longer needed" to "- The returned pool must be freed with `address_pool_free`
when no longer needed"). Ensure the doc comment in managed_account.rs updates
the exact safety paragraph that mentions the returned pool and uses the
backticked symbol `address_pool_free`.
- Around line 57-60: The FFIManagedPlatformAccount::new currently allocates
Box::new(Arc::new(account.clone())) and stores a raw pointer in
FFIManagedPlatformAccount.inner but managed_platform_account_free only drops the
outer struct, leaking that boxed allocation; update
managed_platform_account_free to reconstruct the
Box<Arc<ManagedPlatformAccount>> from inner (e.g. Box::from_raw(inner as *mut
Arc<ManagedPlatformAccount>)) so it gets dropped, and ensure any null checks are
handled safely before reconstructing; also fix the safety doc placeholder `[-]`
to the actual destructor name `managed_platform_account_free` in the safety
documentation for the FFI type to avoid the incomplete reference.
In `@key-wallet-ffi/src/types.rs`:
- Around line 145-166: The FFIWallet currently exposes a public raw pointer
field (inner) and public safe accessors (inner() and inner_mut()) and lacks
Drop, leading to leaks and UB; make the inner field private, implement Drop for
FFIWallet that reconstructs the Box<Arc<Wallet>> (using Box::from_raw) and lets
it drop, and change the raw-pointer accessors: either make inner() and
inner_mut() crate-private and unsafe (or rename to unsafe fn inner_raw() /
inner_raw_mut()) so all raw-pointer dereferencing is confined to private/unsafe
code; keep new() public but ensure it is the only safe constructor that sets
inner via Box::into_raw(Box::new(Arc::new(wallet))).
---
Outside diff comments:
In `@key-wallet-ffi/src/derivation.rs`:
- Around line 45-50: The exported FFI type/name changes (e.g., replacing
FFIExtendedPrivKey/FFIExtendedPubKey with
FFIExtendedPrivateKey/FFIExtendedPublicKey, as seen in the signature of
derivation_new_master_key and the other renamed FFI functions) will break
downstream C bindings; either restore backward-compatible aliases for the old
symbols (typedefs and exported function wrappers that forward to the new
types/functions) so both names exist for a transition release, or mark and
publish this as an explicit breaking change in the PR title and changelog;
update symbol references for derivation_new_master_key, its related derivation_*
functions, and the FFI type declarations to implement aliases if choosing
compatibility.
- Around line 649-671: The free functions derivation_xpriv_free and
derivation_xpub_free currently only call Box::from_raw on the wrapper structs
and thus leak the inner allocations; update them to reconstruct and drop the
inner raw pointer stored inside FFIExtendedPrivateKey / FFIExtendedPublicKey
(the pointer created by from_inner via Box::into_raw) before dropping the outer
wrapper, or alternatively implement Drop for FFIExtendedPrivateKey and
FFIExtendedPublicKey to recover and free the inner ExtendedPrivKey /
ExtendedPubKey; ensure you locate the inner pointer field on
FFIExtendedPrivateKey/FFIExtendedPublicKey, convert it back into a Box and let
it drop, then convert the outer pointer back into a Box (Box::from_raw) so both
inner and outer allocations are freed (apply this for both derivation_xpriv_free
and derivation_xpub_free).
In `@key-wallet-ffi/src/keys.rs`:
- Around line 377-393: The free functions (private_key_free,
extended_private_key_free and their counterparts around the other indices) only
drop the outer FFI*Key Box and leak the inner heap allocation (FFI*Key.inner),
leaving secret material in memory; fix by: for each free function (e.g.,
private_key_free, extended_private_key_free, and the other two free functions),
first check for null, reconstruct the outer Box from the raw pointer, then if
outer.inner is a raw pointer to a heap allocation reconstruct and drop (and for
private keys overwrite/zeroize the secret if applicable) that inner Box before
letting the outer Box go out of scope so both allocations are reclaimed and
secrets are cleared. Ensure you reference and use the FFIPrivateKey.inner /
FFIExtendedPrivateKey.inner fields when freeing.
---
Nitpick comments:
In `@key-wallet-ffi/src/types.rs`:
- Around line 145-149: The PR changed the FFI handle layout by replacing the
typed Arc<Wallet> wrapper with a raw void pointer in the FFIWallet struct (see
FFIWallet and its inner field), which changes consumer-visible layout and
ownership/lifetime semantics; update the PR title and description to explicitly
call out this breaking FFI contract change (mention FFIWallet.inner, Arc<Wallet>
-> *mut c_void swap), note the impact on ABI/lifetimes and consumers, and ensure
the pr-title.yml prefix reflects a breaking or major change per the repo
guidelines.
🪄 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: 2654fd21-937c-4a35-9ba2-df925d603ba4
📒 Files selected for processing (11)
key-wallet-ffi/FFI_API.mdkey-wallet-ffi/cbindgen.tomlkey-wallet-ffi/src/account.rskey-wallet-ffi/src/account_collection.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/derivation.rskey-wallet-ffi/src/derivation_tests.rskey-wallet-ffi/src/keys.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/types.rs
💤 Files with no reviewable changes (1)
- key-wallet-ffi/cbindgen.toml
| Get the external address pool from a managed account This function returns the external (receive) address pool for Standard accounts. Returns NULL for account types that don't have separate external/internal pools. # Safety - `account` must be a valid pointer to an FFIManagedCoreAccount instance - The returned pool must be freed with `[-]` when no longer needed | ||
|
|
||
| **Safety:** | ||
| - `account` must be a valid pointer to an FFIManagedCoreAccount instance - The returned pool must be freed with `address_pool_free` when no longer needed | ||
| - `account` must be a valid pointer to an FFIManagedCoreAccount instance - The returned pool must be freed with `[-]` when no longer needed |
There was a problem hiding this comment.
Restore the real destructor name in this safety contract.
[-] is not a callable free function, and this section tells FFI consumers how to release the returned handle. Please fix the source rustdoc/generator to emit address_pool_free here and then regenerate this file.
🤖 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 3127 - 3130, The safety doc
incorrectly instructs consumers to free the returned address pool with `[-]`
instead of the actual destructor; update the rustdoc/generator that produces the
FFI documentation so the safety contract for the function that returns the
external address pool from an FFIManagedCoreAccount (the function that returns
the external/receive address pool for Standard accounts) references the correct
free function `address_pool_free`, then regenerate FFl_API.md so the returned
pool's release instruction reads "The returned pool must be freed with
`address_pool_free` when no longer needed."
| pub fn new(collection: &key_wallet::AccountCollection) -> Self { | ||
| FFIAccountCollection { | ||
| collection: collection.clone(), | ||
| inner: Box::into_raw(Box::new(collection.clone())) as *mut std::ffi::c_void, | ||
| } | ||
| } | ||
|
|
||
| pub fn inner(&self) -> &key_wallet::AccountCollection { | ||
| unsafe { &*(self.inner as *const key_wallet::AccountCollection) } | ||
| } |
There was a problem hiding this comment.
Free the boxed AccountCollection, not just the wrapper.
FFIAccountCollection::new() now allocates a cloned AccountCollection into inner, but account_collection_free() still only drops the outer FFIAccountCollection. Every wallet_get_account_collection() call now leaks that clone, and ordinary Rust drops leak too because the wrapper has no Drop.
Suggested fix
+impl Drop for FFIAccountCollection {
+ fn drop(&mut self) {
+ if !self.inner.is_null() {
+ unsafe {
+ let _ = Box::from_raw(self.inner as *mut key_wallet::AccountCollection);
+ }
+ self.inner = ptr::null_mut();
+ }
+ }
+}
+
#[no_mangle]
pub unsafe extern "C" fn account_collection_free(collection: *mut FFIAccountCollection) {
if !collection.is_null() {
let _ = Box::from_raw(collection);
}
}Also applies to: 111-114
🤖 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 23 - 31, The FFI
wrapper allocates a cloned AccountCollection into inner (in
FFIAccountCollection::new) but never frees that boxed clone, causing leaks;
implement Drop for FFIAccountCollection to reconstruct and drop the boxed
key_wallet::AccountCollection from the raw inner pointer (e.g., if self.inner !=
std::ptr::null_mut() then unsafe { Box::from_raw(self.inner as *mut
key_wallet::AccountCollection); } ), and ensure account_collection_free consumes
the FFIAccountCollection pointer via Box::from_raw to trigger Drop; update
inner() if needed to handle nulls safely.
| pub fn new(account: &key_wallet::Account) -> Self { | ||
| FFIAccount { | ||
| account: Arc::new(account.clone()), | ||
| inner: Box::into_raw(Box::new(Arc::new(account.clone()))) as *mut std::ffi::c_void, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd key-wallet-ffi && find . -name "account.rs" -type fRepository: dashpay/rust-dashcore
Length of output: 81
🏁 Script executed:
cat -n key-wallet-ffi/src/account.rs | head -200Repository: dashpay/rust-dashcore
Length of output: 7816
Reclaim the boxed inner handle in the *_free paths.
All three constructors heap-allocate Box<Arc<...>> into inner, but account_free, bls_account_free, and eddsa_account_free only drop the outer wrapper. That leaks every account handle allocated through this API.
Each free function must reconstruct and drop self.inner as Box<Arc<...>>. Alternatively, implement Drop for all three wrapper types to ensure both Rust drops and FFI frees free the inner allocation.
🤖 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 21 - 24, The FFIAccount::new
creates a Box<Arc<...>> and stores its raw pointer in FFIAccount.inner, but the
corresponding free functions (account_free, bls_account_free,
eddsa_account_free) currently only drop the outer wrapper and leak the boxed
Arc; update each free path to reconstruct and drop the original Box<Arc<...>> by
casting inner back to *mut Box<Arc<key_wallet::Account>> (or the appropriate
concrete type) and letting it fall out of scope, or alternatively implement Drop
for FFIAccount (and the bls/eddsa wrapper types) to perform the same
Box::from_raw -> drop logic so the heap-allocated Box<Arc<...>> is properly
reclaimed.
| pub fn new(account: &ManagedPlatformAccount) -> Self { | ||
| FFIManagedPlatformAccount { | ||
| account: Arc::new(account.clone()), | ||
| inner: Box::into_raw(Box::new(Arc::new(account.clone()))) as *mut std::ffi::c_void, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and find the relevant lines
wc -l key-wallet-ffi/src/managed_account.rsRepository: dashpay/rust-dashcore
Length of output: 107
🏁 Script executed:
# Read the critical sections: new function around line 57-60
sed -n '55,65p' key-wallet-ffi/src/managed_account.rsRepository: dashpay/rust-dashcore
Length of output: 549
🏁 Script executed:
# Read the free function around line 1310-1313
sed -n '1308,1320p' key-wallet-ffi/src/managed_account.rsRepository: dashpay/rust-dashcore
Length of output: 501
🏁 Script executed:
# Read safety docs around line 872-873
sed -n '870,880p' key-wallet-ffi/src/managed_account.rsRepository: dashpay/rust-dashcore
Length of output: 455
🏁 Script executed:
# Also check for any existing drop implementations for FFIManagedPlatformAccount
rg "impl.*Drop.*FFIManagedPlatformAccount" key-wallet-ffi/src/managed_account.rsRepository: dashpay/rust-dashcore
Length of output: 47
🏁 Script executed:
# Check if Arc is being used and how reference counting works
rg "Arc<ManagedPlatformAccount>" key-wallet-ffi/src/managed_account.rs | head -20Repository: dashpay/rust-dashcore
Length of output: 277
Free the inner Box holding the platform-account Arc.
Line 59 allocates Box::new(Arc::new(account.clone())) and stores its raw pointer in inner. The managed_platform_account_free function at line 1311 only reconstructs and drops the outer FFIManagedPlatformAccount struct, leaving the inner Box<Arc<ManagedPlatformAccount>> allocation unreleased. This causes a memory leak on every platform-account deallocation.
Proposed fix
pub unsafe extern "C" fn managed_platform_account_free(account: *mut FFIManagedPlatformAccount) {
if !account.is_null() {
- let _ = Box::from_raw(account);
+ let account = Box::from_raw(account);
+ if !account.inner.is_null() {
+ let _ = Box::from_raw(account.inner as *mut Arc<ManagedPlatformAccount>);
+ }
}
}Also, the safety documentation at line 872 contains an incomplete placeholder [-] instead of the actual destructor function name.
🤖 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 57 - 60, The
FFIManagedPlatformAccount::new currently allocates
Box::new(Arc::new(account.clone())) and stores a raw pointer in
FFIManagedPlatformAccount.inner but managed_platform_account_free only drops the
outer struct, leaking that boxed allocation; update
managed_platform_account_free to reconstruct the
Box<Arc<ManagedPlatformAccount>> from inner (e.g. Box::from_raw(inner as *mut
Arc<ManagedPlatformAccount>)) so it gets dropped, and ensure any null checks are
handled safely before reconstructing; also fix the safety doc placeholder `[-]`
to the actual destructor name `managed_platform_account_free` in the safety
documentation for the FFI type to avoid the incomplete reference.
| /// - `account` must be a valid pointer to an FFIManagedCoreAccount instance | ||
| /// - The returned pool must be freed with `address_pool_free` when no longer needed | ||
| /// - The returned pool must be freed with `[-]` when no longer needed |
There was a problem hiding this comment.
Replace the [-] placeholder in the safety docs.
Line 873 is part of the exported FFI contract and currently doesn't name a real destructor. This should point callers to address_pool_free, otherwise the public docs tell consumers nothing actionable.
Proposed fix
-/// - The returned pool must be freed with `[-]` when no longer needed
+/// - The returned pool must be freed with `address_pool_free` when no longer needed🤖 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 872 - 873, The safety
docs currently contain the placeholder '[-]' as the destructor for the returned
pool; replace that placeholder with the real destructor name `address_pool_free`
so callers know how to free the returned pool (i.e., change the line "- The
returned pool must be freed with `[-]` when no longer needed" to "- The returned
pool must be freed with `address_pool_free` when no longer needed"). Ensure the
doc comment in managed_account.rs updates the exact safety paragraph that
mentions the returned pool and uses the backticked symbol `address_pool_free`.
| /// Wrapper for a Arc<Wallet> pointer | ||
| #[repr(C)] | ||
| pub struct FFIWallet { | ||
| pub(crate) wallet: Arc<Wallet>, | ||
| pub inner: *mut std::ffi::c_void, // Arc<Wallet>, | ||
| } | ||
|
|
||
| impl FFIWallet { | ||
| /// Create a new FFI wallet handle | ||
| pub fn new(wallet: Wallet) -> Self { | ||
| FFIWallet { | ||
| wallet: Arc::new(wallet), | ||
| inner: Box::into_raw(Box::new(Arc::new(wallet))) as *mut std::ffi::c_void, | ||
| } | ||
| } | ||
|
|
||
| /// Get a reference to the inner wallet | ||
| pub fn inner(&self) -> &Wallet { | ||
| self.wallet.as_ref() | ||
| unsafe { &*(self.inner as *const Arc<Wallet>) } | ||
| } | ||
|
|
||
| /// Get a mutable reference to the inner wallet (requires Arc::get_mut) | ||
| pub fn inner_mut(&mut self) -> Option<&mut Wallet> { | ||
| Arc::get_mut(&mut self.wallet) | ||
| unsafe { Arc::get_mut(&mut *(self.inner as *mut Arc<Wallet>)) } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and check its location and size
find . -name "types.rs" -path "*/key-wallet-ffi/*" | head -5
wc -l ./key-wallet-ffi/src/types.rs 2>/dev/null || echo "File not found or unable to check"Repository: dashpay/rust-dashcore
Length of output: 128
🏁 Script executed:
# Read the specific lines mentioned in the review (145-166)
sed -n '140,180p' ./key-wallet-ffi/src/types.rsRepository: dashpay/rust-dashcore
Length of output: 1175
🏁 Script executed:
# Search for Drop implementations in the file
grep -n "impl.*Drop" ./key-wallet-ffi/src/types.rsRepository: dashpay/rust-dashcore
Length of output: 47
🏁 Script executed:
# Check the PR metadata (title and description if available)
git log -1 --format=%B 2>/dev/null || echo "Git info not available"Repository: dashpay/rust-dashcore
Length of output: 94
🏁 Script executed:
# Get broader context: check what's exported and visibility rules
sed -n '1,50p' ./key-wallet-ffi/src/types.rsRepository: dashpay/rust-dashcore
Length of output: 1442
🏁 Script executed:
# Check git diff to see what changed in this file
git diff HEAD~1 ./key-wallet-ffi/src/types.rs 2>/dev/null | head -100 || echo "Git diff unavailable"Repository: dashpay/rust-dashcore
Length of output: 47
🏁 Script executed:
# Search for any previous FFIWallet definition in git history
git log --all --oneline -S "FFIWallet" -- ./key-wallet-ffi/src/types.rs 2>/dev/null | head -10 || echo "Git log search unavailable"Repository: dashpay/rust-dashcore
Length of output: 164
🏁 Script executed:
# Check if there are tests for FFIWallet
grep -n "FFIWallet" ./key-wallet-ffi/src/types.rs | head -20Repository: dashpay/rust-dashcore
Length of output: 274
🏁 Script executed:
# Look for Drop implementations or unsafe usage patterns elsewhere in the file
rg -n "impl.*Drop|unsafe\s*\{" ./key-wallet-ffi/src/types.rsRepository: dashpay/rust-dashcore
Length of output: 200
🏁 Script executed:
# Check the entire impl block for FFIWallet to see if there's more code
sed -n '145,250p' ./key-wallet-ffi/src/types.rsRepository: dashpay/rust-dashcore
Length of output: 3592
Implement Drop for FFIWallet and keep raw-pointer handling crate-private.
FFIWallet owns a boxed Arc<Wallet> via Box::into_raw() but has no Drop implementation, causing memory leaks on drop. Additionally, the inner field is public and the inner() and inner_mut() methods are safe public functions, allowing safe Rust code to construct FFIWallet { inner: std::ptr::null_mut() } and invoke these methods without an unsafe block, resulting in undefined behavior.
Make inner private and wrap all raw-pointer access in Drop and private/unsafe methods to enforce proper ownership and lifetime guarantees.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet-ffi/src/types.rs` around lines 145 - 166, The FFIWallet currently
exposes a public raw pointer field (inner) and public safe accessors (inner()
and inner_mut()) and lacks Drop, leading to leaks and UB; make the inner field
private, implement Drop for FFIWallet that reconstructs the Box<Arc<Wallet>>
(using Box::from_raw) and lets it drop, and change the raw-pointer accessors:
either make inner() and inner_mut() crate-private and unsafe (or rename to
unsafe fn inner_raw() / inner_raw_mut()) so all raw-pointer dereferencing is
confined to private/unsafe code; keep new() public but ensure it is the only
safe constructor that sets inner via Box::into_raw(Box::new(Arc::new(wallet))).
1 participant
Summary by CodeRabbit
Release Notes
Documentation
Refactor