fix(lang): return AnchorError instead of panicking on truncated zero-copy accounts#4528
fix(lang): return AnchorError instead of panicking on truncated zero-copy accounts#4528abhicris wants to merge 2 commits into
Conversation
…copy accounts
`AccountLoader::{load, load_mut, load_init}` and the generated
`#[account(zero)]` validator sliced into account data using
`disc.len() + size_of::<T>()` (and, for `load_init` / `#[account(zero)]`,
also the discriminator prefix itself) without first verifying that the
buffer was that long. Any account that passed the discriminator
existence check but was shorter than the zero-copy body — e.g. a
discriminator-sized account, or an account truncated by 1 byte — would
panic with `range end index N out of range for slice of length M` and
surface to clients as `Program failed to complete` rather than as a
structured `AnchorError`.
This change:
- adds a `required_zero_copy_len::<T>` helper that performs the
`disc.len() + size_of::<T>()` arithmetic with `checked_add` and maps
overflow to `AccountDidNotDeserialize`.
- gates the body slice in `load`, `load_mut`, and `load_init` on
`data.len() >= required` and returns `AccountDidNotDeserialize` when
it isn't.
- adds the missing `data.len() < disc.len()` check at the top of
`load_init` (the existing one in `load` / `load_mut` was already
there).
- adds the same bounds check ahead of the discriminator slice in the
`generate_constraint_zeroed` codegen, returning
`AccountDiscriminatorNotFound` instead of panicking inside the
client's instruction.
Adds `lang/tests/account_loader_truncation.rs` with five regression
tests covering each panic site and one happy-path sanity test; all five
failure-mode tests panic on `master` and return the documented error
codes after this patch.
Closes otter-sec#4509.
|
@abhicris is attempting to deploy a commit to the Solana Foundation Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR eliminates index-out-of-bounds panics in
Confidence Score: 4/5The core panic-to-error conversion in account_loader.rs is correct and well-tested; the only gap is that the #[account(zero)] codegen path defers a body-too-short error to load_init() rather than catching it during constraint validation. All four panic sites in account_loader.rs are correctly guarded and covered by regression tests. The constraints.rs codegen fixes the discriminator-slice panic but does not add the matching body-size guard, meaning a truncated-body account with a valid-length discriminator region still produces a structured error (no panic), but without the account-name context that constraint-level checks normally provide. The change is safe to merge and accomplishes its stated goal; the missing guard is a quality gap rather than a regression. lang/syn/src/codegen/accounts/constraints.rs — the body-size check is absent from generate_constraint_zeroed Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[AccountLoader load_init called] --> B{is_writable?}
B -- No --> E1[Err: AccountNotMutable]
B -- Yes --> C[borrow_mut_data]
C --> D{data too short for discriminator?}
D -- Yes --> E2["Err: AccountDiscriminatorNotFound (NEW)"]
D -- No --> F[slice discriminator prefix]
F --> G{any byte is nonzero?}
G -- Yes --> E3[Err: AccountDiscriminatorAlreadySet]
G -- No --> H{data too short for body?}
H -- Yes --> E4["Err: AccountDidNotDeserialize (NEW)"]
H -- No --> I[bytemuck::from_bytes_mut on body slice]
I --> OK[Ok: RefMut to T]
style E2 fill:#f9c,stroke:#c66
style E4 fill:#f9c,stroke:#c66
style OK fill:#cfc,stroke:#6c6
|
Summary
AccountLoader::{load, load_mut, load_init}and the generated#[account(zero)]validator slice into account data usingdisc.len() + size_of::<T>()(and, forload_init/#[account(zero)], also the discriminator prefix itself) without first verifying that the buffer is that long. Any account that passes the discriminator existence check but is shorter than the zero-copy body — e.g. a discriminator-sized account, or an account truncated by one byte — panics withrange end index N out of range for slice of length Mand surfaces to clients asProgram failed to completerather than as a structuredAnchorError. This complicates client-side error handling, monitoring, and retry logic for any program that uses zero-copy accounts.What changed
lang/src/accounts/account_loader.rs:required_zero_copy_len::<T>helper that computesdisc.len() + size_of::<T>()withchecked_addand maps overflow toAccountDidNotDeserialize(defence in depth — overflow is not reachable for any practicalT, but the alternative was a silent wrap).load,load_mut, andload_initondata.len() >= requiredand returnsAccountDidNotDeserializewhen it isn't.data.len() < disc.len()check at the top ofload_init(loadandload_mutalready had one — onlyload_initwas missing it).lang/syn/src/codegen/accounts/constraints.rs(generate_constraint_zeroed):data.len() < disc.len()bounds check ahead of the discriminator slice. The generated code now returnsAccountDiscriminatorNotFound(with the account name) instead of panicking inside the user's instruction.The error codes match what the rest of
anchor-langalready returns for the equivalent situation on the non-zero-copyAccountpath, so client SDKs andAnchorErrorconsumers see no new variants.How I validated it
New file
lang/tests/account_loader_truncation.rswith five regression tests, each exercising one panic site:load_returns_error_when_data_shorter_than_zero_copy_bodyloadslice ataccount_loader.rs:169range end index 32 out of range for slice of length 8AccountDidNotDeserializeload_mut_returns_error_when_data_shorter_than_zero_copy_bodyload_mutslice at:194range end index 32 out of range for slice of length 31AccountDidNotDeserializeload_init_returns_error_when_data_shorter_than_zero_copy_bodyload_initslice at:220AccountDidNotDeserializeload_init_returns_error_when_data_shorter_than_discriminatorload_initdisc-prefix slice at:212AccountDiscriminatorNotFoundload_succeeds_on_exactly_sized_accountI confirmed by stashing the source-side fix that the four failure-mode tests reproduce the original panics verbatim on
master, and that the happy-path test continues to pass with the fix applied.Run:
Full
anchor-langsuite stays green:Also clean:
cargo clippy -p anchor-lang --lib --tests -- -D warnings,cargo check --workspace --exclude anchor-cli, andrustfmt --checkon the touched files.Out of scope
#[account(zero)]codegen still slices the discriminator prefix on a separate path inside#from_account_info(the generatedAccountInfo-to-AccountLoaderconversion). With this PR's fix toAccountLoader::try_from, that path no longer panics — it returnsAccountDiscriminatorNotFound. No further codegen change required, but worth flagging.Accountpath uses the user'sAccountDeserializeimpl, which already returnsAccountDidNotDeserializeon a short read; not touched here.mem::size_of::<T>() > isize::MAX— Solana's runtime account-size cap is far below that, so thechecked_addis purely defensive.Closes #4509.