Skip to content

Fix panics on malformed transaction data in legacy instruction decoding#223

Merged
prasanna-anchorage merged 2 commits into
shahankhatch/add-fuzz-targetsfrom
shahankhatch/fix-fuzz-panics
Mar 25, 2026
Merged

Fix panics on malformed transaction data in legacy instruction decoding#223
prasanna-anchorage merged 2 commits into
shahankhatch/add-fuzz-targetsfrom
shahankhatch/fix-fuzz-panics

Conversation

@shahan-khatchadourian-anchorage
Copy link
Copy Markdown
Contributor

Summary

Fixes two panic sites in the legacy transaction path found by cargo-fuzz targets introduced in PR #204.

Bug 1: Unchecked array indexing in instructions.rs

decode_instructions indexes into account_keys using program_id_index and account indices from compiled instructions without bounds checks. Malformed transactions where these indices exceed the account keys array cause index-out-of-bounds panics.

thread panicked at src/core/instructions.rs:33:37:
index out of bounds: the len is 2 but the index is 255

Fix: Use filter_map with bounds checks, matching the pattern already used in v0.rs (lines 137-165). Instructions with out-of-bounds program_id_index are skipped; accounts with out-of-bounds indices are dropped from the instruction. An explicit Err is returned for empty account keys.

Bug 2: Arithmetic underflow in accounts/decode.rs

decode_accounts and decode_v0_accounts subtract header values (num_readonly_signed_accounts, num_readonly_unsigned_accounts) from array lengths without checking that the header values are consistent. Malformed transactions where header counts exceed the actual account keys length cause underflow panics in debug builds and wrapping in release builds.

thread panicked at src/core/accounts/decode.rs:27:45:
attempt to subtract with overflow

Fix: Replace bare subtraction with saturating_sub. On inconsistent headers, the writable/readonly classification degrades gracefully (accounts may be misclassified as writable or readonly) rather than panicking.

Silent failure trade-offs

Both fixes align with the existing v0 transaction handling strategy: silently skip or degrade on malformed data rather than returning errors. This is the pragmatic choice for a visualization tool — showing partial results is better than crashing — but it has risks:

Scenario Current behavior (after fix) Risk
Instruction with OOB program_id_index Instruction silently dropped User sees fewer instructions than the transaction contains
Instruction with OOB account index Account silently dropped from instruction Named accounts may be missing from the display
Inconsistent message header Accounts misclassified as writable/readonly Signer/writable badges may be wrong

Future option: These could be surfaced as VisualSignError variants or as warning fields in the SignablePayload output, so callers can distinguish "clean parse" from "degraded parse". This would require a new error variant or a warnings field in the output type.

Test plan

  • All 98 existing tests pass (cargo test -p visualsign-solana)
  • cargo +nightly fuzz run fuzz_transaction_string -- -max_total_time=30 — ~420,000 runs, zero crashes
  • cargo +nightly fuzz run fuzz_versioned_transaction -- -max_total_time=30 — ~510,000 runs, zero crashes
  • cargo clippy -p visualsign-solana --tests -- -D warnings — clean
  • cargo fmt -- --check — clean

🤖 Generated with Claude Code

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the legacy Solana transaction decoding path to avoid panics on malformed input, aligning behavior more closely with the existing v0 transaction handling strategy.

Changes:

  • Add bounds checks when expanding legacy compiled instructions to prevent index-out-of-bounds panics.
  • Prevent arithmetic underflow in account writable/readonly classification by switching to saturating_sub in both legacy and v0 account decoding.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
src/chain_parsers/visualsign-solana/src/core/instructions.rs Adds guard for empty account keys and bounds-checks compiled instruction indices to avoid panics during instruction expansion.
src/chain_parsers/visualsign-solana/src/core/accounts/decode.rs Replaces potentially-underflowing header arithmetic with saturating_sub for safer account classification on malformed headers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/chain_parsers/visualsign-solana/src/core/accounts/decode.rs
Comment thread src/chain_parsers/visualsign-solana/src/core/instructions.rs Outdated
Comment thread src/chain_parsers/visualsign-solana/src/core/instructions.rs Outdated
Comment thread src/chain_parsers/visualsign-solana/src/core/accounts/decode.rs
Two panic sites in the legacy transaction path:

1. instructions.rs: unchecked indexing into account_keys with
   program_id_index and account indices from compiled instructions.
   Malformed transactions with out-of-bounds indices cause
   index-out-of-bounds panics.

2. accounts/decode.rs: arithmetic underflow when message header
   values (num_readonly_signed_accounts, num_readonly_unsigned_accounts)
   exceed the actual account keys array length.

Fix: apply the same defensive patterns already used in the v0
transaction path — filter_map with bounds checks for instruction
indices, saturating_sub for header arithmetic, and an explicit error
for empty account keys.

Verified with cargo-fuzz: ~930,000 runs across both fuzz targets
(fuzz_transaction_string, fuzz_versioned_transaction) with zero crashes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Clarify comment to distinguish skipped instructions (OOB program_id)
  from omitted accounts (OOB account index)
- Use "Legacy transaction" in error message for consistency with v0 path
- Add unit tests for decode_accounts and decode_v0_accounts with
  inconsistent header values to lock in saturating_sub behavior

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
let readonly_signer_start = message.header.num_required_signatures as usize
- message.header.num_readonly_signed_accounts as usize;
let readonly_signer_start = (message.header.num_required_signatures as usize)
.saturating_sub(message.header.num_readonly_signed_accounts as usize);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, I always forget about saturating_sub

@prasanna-anchorage prasanna-anchorage merged commit febf931 into shahankhatch/add-fuzz-targets Mar 25, 2026
7 checks passed
@prasanna-anchorage prasanna-anchorage deleted the shahankhatch/fix-fuzz-panics branch March 25, 2026 23:15
prasanna-anchorage pushed a commit that referenced this pull request Mar 27, 2026
* Add cargo fuzz targets for visualsign-solana

Two libFuzzer targets covering the full visualsign-solana stack:
- fuzz_transaction_string: arbitrary bytes into transaction_string_to_visual_sign
- fuzz_versioned_transaction: arbitrary bytes deserialized as VersionedTransaction
  then passed to versioned_transaction_to_visual_sign

Run with: cargo +nightly fuzz run <target>
(from src/chain_parsers/visualsign-solana/fuzz/)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Add proptest and fuzz label-triggered CI jobs

- proptest label: runs cargo test -p visualsign-solana
- fuzz label: installs nightly + cargo-fuzz, runs each fuzz target for 30s
- ubuntu job: restricted to main push/PR to avoid triggering on label events

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Post fuzz crash summary as PR comment on failure

On crash, extracts the libFuzzer summary (everything after the ─── line)
and posts it as a PR comment via gh. No artifacts or separate jobs needed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Split proptest and fuzz workflows into separate files

Move proptest and fuzz jobs out of main.yml into dedicated workflow
files (proptest.yml, fuzz.yml) so they appear as distinct named checks.
Add pull-requests: write permission to fuzz job to allow posting crash
comments via gh pr comment.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Add reusable post-failure-comment action tagging @copilot

Shared composite action posts crash/failure output as a PR comment
and tags @copilot to fix the issue. Fuzz and proptest workflows use
it via extract steps that write output to GITHUB_OUTPUT.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Extract shared pipeline helpers into common module

Move build_transaction, options_with_idl, instruction_fields, find_text
and related helpers from pipeline_integration.rs into tests/common/mod.rs
so they can be reused by other test files without duplication.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Add semantic pipeline tests for 8 embedded IDLs

Test the full visualization pipeline with realistic Borsh-serialized
instruction data for Drift deposit, Lifinity swap, Raydium swapBaseInput,
Orca swap (including u128 sqrtPriceLimit), Meteora swap, Kamino deposit,
Stabble swap (Option<u64> arg), and OpenBook deposit.

Each test uses assert_semantic() to verify decoded instruction name and
arg values match what was serialized. Lives in its own file to keep
pipeline_integration.rs focused on proptest-based property tests.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Allow triggering CI on any PR via 'ci' label

Add 'labeled' to pull_request trigger types and allow the ubuntu job
to run when the 'ci' label is present, not just for PRs targeting main.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Fix fuzz build and simplify CI workflows

- Pin fuzz Cargo.lock to avoid spl-token-2022 breakage on nightly
  (spl-token-group-interface 0.7.2 pulled in solana-nullable which is
  incompatible with spl-token-2022 10.0.0)
- Remove stale Cargo.lock gitignore rule from visualsign-solana
- Remove post-failure-comment action and simplify fuzz/proptest workflows
- Use continue-on-error on fuzz steps so crashes show as warnings
  (known pre-existing panics in instructions.rs bounds checking)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Add failure labels to fuzz and proptest workflows

On crash/failure, add fuzz-failure or proptest-failure label to the PR.
On clean run, remove the label. This gives visible signal without
blocking the check.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Harden label steps in fuzz and proptest workflows

Add || true to --add-label calls so the step doesn't fail if the label
operation itself has issues (e.g. label not yet created in the repo).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* docs: add cargo fuzz, semantic tests, and CI labels to testing guide

Extend the property-based testing section with cargo fuzz targets,
semantic pipeline tests, CI workflow labels (proptest, fuzz, ci),
and failure label behavior (fuzz-failure, proptest-failure).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Address Copilot review: pin nightly, lock cargo-fuzz, fix cache key

- Pin nightly toolchain to 2026-03-13 (known-good) in both
  rust-toolchain.toml and fuzz.yml
- Use --locked for cargo install cargo-fuzz in CI and docs
- Cache fuzz/target/ and key off fuzz/Cargo.lock instead of src/Cargo.lock

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Address code review: Default::default(), env var for PR number

- Use ..VisualSignOptions::default() in test helpers to prevent
  breakage when new fields are added to the struct
- Pass github.event.pull_request.number through env var instead
  of direct context interpolation in shell blocks

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Fix panics on malformed transaction data in legacy instruction decoding (#223)

* fix: guard against panics on malformed transaction data

Two panic sites in the legacy transaction path:

1. instructions.rs: unchecked indexing into account_keys with
   program_id_index and account indices from compiled instructions.
   Malformed transactions with out-of-bounds indices cause
   index-out-of-bounds panics.

2. accounts/decode.rs: arithmetic underflow when message header
   values (num_readonly_signed_accounts, num_readonly_unsigned_accounts)
   exceed the actual account keys array length.

Fix: apply the same defensive patterns already used in the v0
transaction path — filter_map with bounds checks for instruction
indices, saturating_sub for header arithmetic, and an explicit error
for empty account keys.

Verified with cargo-fuzz: ~930,000 runs across both fuzz targets
(fuzz_transaction_string, fuzz_versioned_transaction) with zero crashes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Address Copilot review: improve comments, error message, add tests

- Clarify comment to distinguish skipped instructions (OOB program_id)
  from omitted accounts (OOB account index)
- Use "Legacy transaction" in error message for consistency with v0 path
- Add unit tests for decode_accounts and decode_v0_accounts with
  inconsistent header values to lock in saturating_sub behavior

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
prasanna-anchorage pushed a commit that referenced this pull request Apr 8, 2026
* Add cargo fuzz targets for visualsign-solana

Two libFuzzer targets covering the full visualsign-solana stack:
- fuzz_transaction_string: arbitrary bytes into transaction_string_to_visual_sign
- fuzz_versioned_transaction: arbitrary bytes deserialized as VersionedTransaction
  then passed to versioned_transaction_to_visual_sign

Run with: cargo +nightly fuzz run <target>
(from src/chain_parsers/visualsign-solana/fuzz/)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Add proptest and fuzz label-triggered CI jobs

- proptest label: runs cargo test -p visualsign-solana
- fuzz label: installs nightly + cargo-fuzz, runs each fuzz target for 30s
- ubuntu job: restricted to main push/PR to avoid triggering on label events

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Post fuzz crash summary as PR comment on failure

On crash, extracts the libFuzzer summary (everything after the ─── line)
and posts it as a PR comment via gh. No artifacts or separate jobs needed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Split proptest and fuzz workflows into separate files

Move proptest and fuzz jobs out of main.yml into dedicated workflow
files (proptest.yml, fuzz.yml) so they appear as distinct named checks.
Add pull-requests: write permission to fuzz job to allow posting crash
comments via gh pr comment.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Add reusable post-failure-comment action tagging @copilot

Shared composite action posts crash/failure output as a PR comment
and tags @copilot to fix the issue. Fuzz and proptest workflows use
it via extract steps that write output to GITHUB_OUTPUT.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Extract shared pipeline helpers into common module

Move build_transaction, options_with_idl, instruction_fields, find_text
and related helpers from pipeline_integration.rs into tests/common/mod.rs
so they can be reused by other test files without duplication.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Add semantic pipeline tests for 8 embedded IDLs

Test the full visualization pipeline with realistic Borsh-serialized
instruction data for Drift deposit, Lifinity swap, Raydium swapBaseInput,
Orca swap (including u128 sqrtPriceLimit), Meteora swap, Kamino deposit,
Stabble swap (Option<u64> arg), and OpenBook deposit.

Each test uses assert_semantic() to verify decoded instruction name and
arg values match what was serialized. Lives in its own file to keep
pipeline_integration.rs focused on proptest-based property tests.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Allow triggering CI on any PR via 'ci' label

Add 'labeled' to pull_request trigger types and allow the ubuntu job
to run when the 'ci' label is present, not just for PRs targeting main.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Fix fuzz build and simplify CI workflows

- Pin fuzz Cargo.lock to avoid spl-token-2022 breakage on nightly
  (spl-token-group-interface 0.7.2 pulled in solana-nullable which is
  incompatible with spl-token-2022 10.0.0)
- Remove stale Cargo.lock gitignore rule from visualsign-solana
- Remove post-failure-comment action and simplify fuzz/proptest workflows
- Use continue-on-error on fuzz steps so crashes show as warnings
  (known pre-existing panics in instructions.rs bounds checking)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Add failure labels to fuzz and proptest workflows

On crash/failure, add fuzz-failure or proptest-failure label to the PR.
On clean run, remove the label. This gives visible signal without
blocking the check.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Harden label steps in fuzz and proptest workflows

Add || true to --add-label calls so the step doesn't fail if the label
operation itself has issues (e.g. label not yet created in the repo).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* docs: add cargo fuzz, semantic tests, and CI labels to testing guide

Extend the property-based testing section with cargo fuzz targets,
semantic pipeline tests, CI workflow labels (proptest, fuzz, ci),
and failure label behavior (fuzz-failure, proptest-failure).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Address Copilot review: pin nightly, lock cargo-fuzz, fix cache key

- Pin nightly toolchain to 2026-03-13 (known-good) in both
  rust-toolchain.toml and fuzz.yml
- Use --locked for cargo install cargo-fuzz in CI and docs
- Cache fuzz/target/ and key off fuzz/Cargo.lock instead of src/Cargo.lock

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Address code review: Default::default(), env var for PR number

- Use ..VisualSignOptions::default() in test helpers to prevent
  breakage when new fields are added to the struct
- Pass github.event.pull_request.number through env var instead
  of direct context interpolation in shell blocks

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Fix panics on malformed transaction data in legacy instruction decoding (#223)

* fix: guard against panics on malformed transaction data

Two panic sites in the legacy transaction path:

1. instructions.rs: unchecked indexing into account_keys with
   program_id_index and account indices from compiled instructions.
   Malformed transactions with out-of-bounds indices cause
   index-out-of-bounds panics.

2. accounts/decode.rs: arithmetic underflow when message header
   values (num_readonly_signed_accounts, num_readonly_unsigned_accounts)
   exceed the actual account keys array length.

Fix: apply the same defensive patterns already used in the v0
transaction path — filter_map with bounds checks for instruction
indices, saturating_sub for header arithmetic, and an explicit error
for empty account keys.

Verified with cargo-fuzz: ~930,000 runs across both fuzz targets
(fuzz_transaction_string, fuzz_versioned_transaction) with zero crashes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Address Copilot review: improve comments, error message, add tests

- Clarify comment to distinguish skipped instructions (OOB program_id)
  from omitted accounts (OOB account index)
- Use "Legacy transaction" in error message for consistency with v0 path
- Add unit tests for decode_accounts and decode_v0_accounts with
  inconsistent header values to lock in saturating_sub behavior

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test: add real-IDL structural validation tests

Extract load_idl_from_env into common/mod.rs and add
real_idl_validation.rs with deterministic structural invariant
tests for production IDLs: discriminator presence/uniqueness,
instruction name uniqueness, and IDL hash stability.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test: add proptest-based real-IDL coverage tests

Add 4 new property tests against real production IDLs:
- arg name completeness in parsed output
- overlong data rejection (trailing byte)
- truncated data rejection (missing byte)
- discriminator isolation (cross-instruction swap)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants