fix: reject duplicate accounts in convert_account_infos#2258
Conversation
Audit issue #21 (INFO): convert_account_infos created multiple mutable references when duplicate account keys were passed. Add a pre-check that detects duplicate keys and returns InvalidArgument to prevent undefined behavior from aliased mutable references.
📝 WalkthroughWalkthroughAdds runtime-safe duplicate-account detection to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Replace blanket rejection of duplicate account keys with Rc sharing that mimics Solana runtime behavior. When the same account appears in multiple positions (e.g., fee_payer and authority are the same signer), the Rc<RefCell<>> from the first occurrence is reused, preventing independent mutable references while allowing legitimate duplicate account usage.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@programs/compressed-token/program/src/convert_account_infos.rs`:
- Line 43: Add a one-line SAFETY comment inside the duplicate-account branch
(the block starting with if let Some(existing) = solana_accounts.iter().find(|a|
a.key == key) {) explaining that Pinocchio may present duplicate AccountInfo
entries that point to the same underlying memory, therefore we must share the
existing Rc/RefCell rather than creating a new RefCell around the same raw
pointer because creating independent RefCell wrappers (or separate mutable
owners) for the same memory would be unsound; mention the key symbols
(solana_accounts, existing, Rc/RefCell) so future readers know why we
clone/share the existing Rc here.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@programs/compressed-token/program/src/convert_account_infos.rs`:
- Around line 40-48: Remove the duplicate comment block about sharing
Rc<RefCell<>> for duplicate accounts so the explanation and SAFETY note appear
only once; locate the repeated verbatim lines mentioning "For duplicate
accounts, share Rc<RefCell<>> from the first occurrence" and the "SAFETY:
pinocchio..." sentence in convert_account_infos.rs and delete the redundant
copy, leaving a single, continuous comment describing the shared RefCell
behavior and the SAFETY rationale.
| // SAFETY: pinocchio backs duplicate keys with the same memory region, so | ||
| // wrapping it in a second RefCell would allow aliased &mut — hence we must | ||
| // share the original RefCell via Rc::clone. | ||
| if let Some(existing) = solana_accounts.iter().find(|a| a.key == key) { |
There was a problem hiding this comment.
use pubkey_eq for comparison
- Remove duplicated comment block - Use light_array_map::pubkey_eq with pinocchio keys directly instead of comparing solana_pubkey::Pubkey references
Summary