chore(key-wallet-ffi): cbindgen.toml cleanup#629
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughRemoved export configuration sections from the cbindgen.toml file, including the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #629 +/- ##
=============================================
- Coverage 67.99% 67.96% -0.04%
=============================================
Files 319 319
Lines 67970 67970
=============================================
- Hits 46216 46193 -23
- Misses 21754 21777 +23
|
![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: 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
github-actions
Bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
ZocoLini
force-pushed
the
chore/rm-opaque-types
branch
from
66b0097 to
bbd2fda
Compare
github-actions
Bot
removed
the
merge-conflict
ZocoLini
changed the title
ZocoLini
force-pushed
the
chore/rm-opaque-types
branch
from
bbd2fda to
1e66026
Compare
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.
Removed cbindgen instructions that are not having any real effect
Summary by CodeRabbit