tlv-account-resolution: error instead of panicking on malformed input - #207
Merged
joncinque merged 2 commits intoOct 2, 2026
Merged
Conversation
check_account_infos panicked on a malformed validation account (TlvStateBorrowed::unpack(data).unwrap()) and underflowed when the caller provided fewer account infos than the validation data's extra accounts require — a subtraction that wrapped to a huge value in release builds and produced a misleading IncorrectAccount error. The AccountResolutionError::NotEnoughAccounts variant existed but was never constructed. The validation-data unpack now propagates the error, and the subtraction is checked against NotEnoughAccounts.
joncinque
reviewed
Oct 2, 2026
joncinque
left a comment
joncinque
left a comment
Contributor
There was a problem hiding this comment.
Thanks for your contribution! This is mostly called in tests and the example transfer hook program, so the panicking isn't a problem, but it doesn't hurt to return errors, since the function already does that.
Just some nits to clean up the unnecessary comments
Comment on lines
+221
to
+222
| // Ensure the caller provided enough account infos to hold the extra | ||
| // accounts, or the subtraction below underflows. |
Contributor
There was a problem hiding this comment.
nit: can you remove this comment? It doesn't add much information
Contributor
Author
There was a problem hiding this comment.
Removed all three, thanks!
Comment on lines
+1715
to
+1717
| // A caller (or crafted transaction) providing fewer account infos than | ||
| // the validation data's extra accounts must error, not underflow the | ||
| // initial-accounts-length subtraction. |
Contributor
There was a problem hiding this comment.
nit: can you remove this comment too?
Comment on lines
+1746
to
+1747
| // A corrupted validation account (truncated TLV entry) must error | ||
| // instead of panicking on the unpack. |
Contributor
There was a problem hiding this comment.
nit: can you remove this comment too?
Contributor
Author
|
@joncinque all three comments removed, pushed to the branch — thanks! |
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
2 participants
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.
Problem
ExtraAccountMetaList::check_account_infospanicked in two paths:TlvStateBorrowed::unpack(data).unwrap()— a malformed validation account (a truncated TLV entry) panicked instead of returning aProgramError.account_infos.len() - extra_meta_list.len()— when the caller provided fewer account infos than the validation data's extra accounts, the subtraction underflowed: panic in debug/overflow-checks builds, and a wrap to a huge value in release builds that produced a misleadingIncorrectAccounterror.This is reachable from consuming programs' transfer-hook validation, where the account list comes from the transaction author (a crafted transaction with fewer accounts than the hook requires). The
AccountResolutionError::NotEnoughAccountsvariant existed with its message but was never constructed — the check was clearly intended but omitted. The async siblingadd_to_instructionalready propagates the unpack error with?;check_account_infosdid not.Change
check_account_infospropagates the validation-data unpack error with?instead of unwrapping.checked_suband returnsAccountResolutionError::NotEnoughAccounts.Tests
Two new tests: a caller providing fewer account infos than extra metas receives
NotEnoughAccounts(previously a panic), and corrupted validation data returns an error (previously a panic). The existingcheck_account_infos_testpasses unchanged (the correct path is untouched).cargo test -p spl-tlv-account-resolutionpasses (18 tests, run repeatedly) and clippy/fmt are green.