Skip to content

use saturating_sub in is_writable_index() - fix #150#154

Merged
rustopian merged 3 commits intoanza-xyz:masterfrom
rustopian:fix-150-is-writable-index-overflow
May 7, 2025
Merged

use saturating_sub in is_writable_index() - fix #150#154
rustopian merged 3 commits intoanza-xyz:masterfrom
rustopian:fix-150-is-writable-index-overflow

Conversation

@rustopian
Copy link
Copy Markdown
Contributor

@rustopian rustopian commented May 6, 2025 •
edited
Loading

legacy: use saturating_sub in is_writable_index()

Fixes #150.

Problem

is_writable_index() subtracts two u8 header counters.
If the caller skips Message::sanitize() the subtraction can underflow, panicking in debug builds or silently wrapping to 255 in release, making ≈all accounts appear writable.

Options considered

  • wrapping_sub

    • Zero-cost.
    • Fail-open: an unchecked message marks many indices writable.

  • saturating_sub (chosen)

    • Adds a micro-op, but this should cause no performance impact.

Change

Replace both unchecked subtractions with saturating_sub.
Add a unit test covering scenarios including those demonstrated in the original PoC.

Result

Now immune to underflow; behavior of legacy messages here aligns with behavior of v0 messages.

@rustopian rustopian requested a review from joncinque May 6, 2025 16:32
@rustopian rustopian marked this pull request as ready for review May 6, 2025 16:32
@rustopian rustopian requested a review from a team as a code owner May 6, 2025 16:32
joncinque
joncinque approved these changes May 6, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

The fix and the tests make sense to me, but it does beg the question about whether we should change this interface altogether since it'll just give false in broken situations. But maybe that's ok -- I'll let someone from tx-metadata give the final ✔️ .

jstarry
jstarry approved these changes May 7, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

I'm happy with using saturating_sub here and returning false for all keys in malformed messages

@rustopian rustopian merged commit 3508bd2 into anza-xyz:master May 7, 2025
24 checks passed
febo pushed a commit to febo/solana-sdk that referenced this pull request Sep 21, 2025
@rustopian @febo
use saturating_sub in is_writable_index() - fix anza-xyz#150 (anza-xy…
fed8c7a 
…z#154)

* fix issue anza-xyz#150 (possible underflow if is_writable_index is called directly with malformed data).
* tests against the issue's PoCs and similar cases.
febo added a commit to febo/solana-sdk that referenced this pull request Sep 24, 2025
@febo
Simplify invoke instruction (anza-xyz#154)
18ff6cd 
* Fix review comments

* Revert offset increment change

* Improve invoke performance

* Add invoke instruction helper

* Typos

* Remove new helpers

* Remove unused

* Address review comments

* Tweak iteration

* More tweaks

* Limit the length of the slice

* Uncomment code

* Validate account length

* Tweak inline attributes

* Use invoke signed unchecked

* Refactor inline

* Renamed to with_bounds

* Update docs

* Fix accounts length test

* Revert change

* Add constant length check

* Review comments

* Fix error variant
febo added a commit to febo/solana-sdk that referenced this pull request Sep 25, 2025
@febo
Simplify invoke instruction (anza-xyz#154)
8b7c42c 
* Fix review comments

* Revert offset increment change

* Improve invoke performance

* Add invoke instruction helper

* Typos

* Remove new helpers

* Remove unused

* Address review comments

* Tweak iteration

* More tweaks

* Limit the length of the slice

* Uncomment code

* Validate account length

* Tweak inline attributes

* Use invoke signed unchecked

* Refactor inline

* Renamed to with_bounds

* Update docs

* Fix accounts length test

* Revert change

* Add constant length check

* Review comments

* Fix error variant
febo added a commit to febo/solana-sdk that referenced this pull request Oct 3, 2025
@febo
Simplify invoke instruction (anza-xyz#154)
5b03fa6 
* Fix review comments

* Revert offset increment change

* Improve invoke performance

* Add invoke instruction helper

* Typos

* Remove new helpers

* Remove unused

* Address review comments

* Tweak iteration

* More tweaks

* Limit the length of the slice

* Uncomment code

* Validate account length

* Tweak inline attributes

* Use invoke signed unchecked

* Refactor inline

* Renamed to with_bounds

* Update docs

* Fix accounts length test

* Revert change

* Add constant length check

* Review comments

* Fix error variant
febo added a commit to febo/solana-sdk that referenced this pull request Oct 18, 2025
@febo
Simplify invoke instruction (anza-xyz#154)
f2fd601 
* Fix review comments

* Revert offset increment change

* Improve invoke performance

* Add invoke instruction helper

* Typos

* Remove new helpers

* Remove unused

* Address review comments

* Tweak iteration

* More tweaks

* Limit the length of the slice

* Uncomment code

* Validate account length

* Tweak inline attributes

* Use invoke signed unchecked

* Refactor inline

* Renamed to with_bounds

* Update docs

* Fix accounts length test

* Revert change

* Add constant length check

* Review comments

* Fix error variant
febo added a commit to febo/solana-sdk that referenced this pull request Oct 23, 2025
@febo
Simplify invoke instruction (anza-xyz#154)
2bb0242 
* Fix review comments

* Revert offset increment change

* Improve invoke performance

* Add invoke instruction helper

* Typos

* Remove new helpers

* Remove unused

* Address review comments

* Tweak iteration

* More tweaks

* Limit the length of the slice

* Uncomment code

* Validate account length

* Tweak inline attributes

* Use invoke signed unchecked

* Refactor inline

* Renamed to with_bounds

* Update docs

* Fix accounts length test

* Revert change

* Add constant length check

* Review comments

* Fix error variant
febo added a commit to febo/solana-sdk that referenced this pull request Nov 19, 2025
@febo
Simplify invoke instruction (anza-xyz#154)
ce25cef 
* Fix review comments

* Revert offset increment change

* Improve invoke performance

* Add invoke instruction helper

* Typos

* Remove new helpers

* Remove unused

* Address review comments

* Tweak iteration

* More tweaks

* Limit the length of the slice

* Uncomment code

* Validate account length

* Tweak inline attributes

* Use invoke signed unchecked

* Refactor inline

* Renamed to with_bounds

* Update docs

* Fix accounts length test

* Revert change

* Add constant length check

* Review comments

* Fix error variant
febo added a commit that referenced this pull request Nov 21, 2025
@febo @publicqi @stegaBOB
instruction-view: Add new crate (#401)
6b7745f 
* Add derive address helpers

* Update lock file

* Fix doc links

* Add missing dependency

* Address review comments (#78)

* [wip]: Address review comments

* [wip]: Fix pointer reference

* [wip]: Add logger buffer size tests

* Remove unused

* More logger tests

* Rename program to cpi

* Remove dynamic allocation

* Fixed signed tests

* Fix review comments

* Fix unsigned test case

* Add is_owner_by helper

* Deprecate AccountInfo::realloc (#174)

* Add resize

* Deprecate realloc

* Ignore `zero_init` parameter (#203)

Ignore zero_init parameter

* Remove unused

* Refactor directory structure (#18)

* Use macro rules

* Update directory structure

* Add from_slice helper (#23)

* Fix signer from helpers

* Add signer macro

* Fix doctest

* Add `checked_create_program_address` helper (#30)

* Add unchecked helper

* Fix lint

* Add inline

* Rename to checked

* Cosmetics

* Fix sol log params

* sdk: Improve comments (#64)

* [wip]: Add new scripts

* [wip]: Use matric strategy

* [wip]: Fix members parsing

* [wip]: Add CI env variables

* [wip]: Remove nothrow

* [wip]: Filter changes

* [wip]: Add audit step

* [wip]: Add semver checks

* [wip]: Refactor publish workflow

* [wip]: Refactor

* [wip]: Fix commands

* Fix formatting

* Remove detect changes step

* Review comments

* Fix lint comments

* Expand crate comment

* Ignore crate comment tests

* Add missing docs

* More missing docs

* Add missing release component

* Pin cargo-release version

* Fix merge

* Review comments

* add `seeds` macro (#73)

* use `seeds` macro over `signer` macro

* fix doc to pass doc test

* deprecate  macro

* Address review comments (#78)

* [wip]: Address review comments

* [wip]: Fix pointer reference

* [wip]: Add logger buffer size tests

* Remove unused

* More logger tests

* Rename program to cpi

* Remove dynamic allocation

* Fixed signed tests

* Fix review comments

* Fix unsigned test case

* Add is_owner_by helper

* add memo program (#105)

* add memo program

* use MAX_CPI_ACCOUNTS over MAX_TX_ACCOUNTS

* nit

* add account info borrowing check

* update docstring and comments

* use &str over &[u8]

* check num_accounts

* avoid unnecessary codegen

* update doc to use &str over &[u8]

* fix doc

* seperate lifetime

* Account borrow state check (#147)

* Improve fallback and docs

* Add borrow state check

* Add inline

* Review comments

* Revert doc link merge change

* Add const qualifier (#159)

* Simplify invoke instruction (#154)

* Fix review comments

* Revert offset increment change

* Improve invoke performance

* Add invoke instruction helper

* Typos

* Remove new helpers

* Remove unused

* Address review comments

* Tweak iteration

* More tweaks

* Limit the length of the slice

* Uncomment code

* Validate account length

* Tweak inline attributes

* Use invoke signed unchecked

* Refactor inline

* Renamed to with_bounds

* Update docs

* Fix accounts length test

* Revert change

* Add constant length check

* Review comments

* Fix error variant

* ci: Add spellcheck step (#164)

* Add invoke instruction helper

* Typos

* Remove new helpers

* Remove unused

* Address review comments

* Tweak inline attributes

* Use invoke signed unchecked

* Refactor inline

* Renamed to with_bounds

* Update docs

* Revert change

* Add constant length check

* Add spellcheck step

* Tweak action

* Fix typos

* More fixes

* Yet more fixes

* Fixes

* Add j1 option

* More and more fixes

* Add missing acronym

* Fix merge

* Fix spelling

* Fix spelling

* Clarify the use of constant values (#200)

* Add comments on constants

* Improve offset comments

* Add bitmask to dictionary

* Renamed to field_at_offset

* Feat: Add debug/copy derives and enable missing debug/copy lint (#228)

* Add debug/copy derives and enable missing debug/copy lint

* Update sdk/pinocchio/src/sysvars/rent.rs

Co-authored-by: Fernando Otero <febo@anza.xyz>

* Update sdk/pinocchio/src/entrypoint/mod.rs

Co-authored-by: Fernando Otero <febo@anza.xyz>

* Update sdk/pinocchio/src/instruction.rs

Co-authored-by: Fernando Otero <febo@anza.xyz>

* Update sdk/pinocchio/src/sysvars/clock.rs

Co-authored-by: Fernando Otero <febo@anza.xyz>

* Update sdk/pinocchio/src/sysvars/fees.rs

* Update sdk/pinocchio/src/sysvars/fees.rs

* Update sdk/pinocchio/src/sysvars/instructions.rs

Co-authored-by: Fernando Otero <febo@anza.xyz>

* Update sdk/pinocchio/src/sysvars/instructions.rs

Co-authored-by: Fernando Otero <febo@anza.xyz>

* Update sdk/pinocchio/src/sysvars/instructions.rs

Co-authored-by: Fernando Otero <febo@anza.xyz>

* Update sdk/pinocchio/src/sysvars/clock.rs

* Fix syntax error in Instructions struct derive macro

---------

Co-authored-by: Fernando Otero <febo@anza.xyz>

* pinocchio: Add `pubkey_eq` helper (#248)

* Add pubkey_eq helper

* Fix typo

* Update pubkey comparison

* Add proptest

* Add unlikely

* Replace proptest

* Use solana-address

* Fix syscall parameter

* Fix docs

* Update references to pubkey

* Use solana-account-view

* Update dependencies

* Add instruction view

* Update lamports reference

* Tweks

* Add target_arch bpf

* Fix rebase

* Fix lint

* Simplify definition

* Fix doc feature

* Address review comments

* Refactored slice support

* Improve code reuse

* Update msrv check

* More review comments

* Use crate rust-version

* Double quotes

* Fix docs

---------

Co-authored-by: publicqi <56060664+publicqi@users.noreply.github.com>
Co-authored-by: Sammy Harris <41593264+stegaBOB@users.noreply.github.com>
grod220 pushed a commit that referenced this pull request Mar 9, 2026
@dependabot @grod220
build(deps): bump serde from 1.0.219 to 1.0.223 (#154)
16806e5 
Bumps [serde](https://github.com/serde-rs/serde) from 1.0.219 to 1.0.223.
- [Release notes](https://github.com/serde-rs/serde/releases)
- [Commits](serde-rs/serde@v1.0.219...v1.0.223)

---
updated-dependencies:
- dependency-name: serde
  dependency-version: 1.0.223
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
grod220 pushed a commit that referenced this pull request Mar 16, 2026
@dependabot @grod220
build(deps): bump serde from 1.0.219 to 1.0.223 (#154)
179d7c5 
Bumps [serde](https://github.com/serde-rs/serde) from 1.0.219 to 1.0.223.
- [Release notes](https://github.com/serde-rs/serde/releases)
- [Commits](serde-rs/serde@v1.0.219...v1.0.223)

---
updated-dependencies:
- dependency-name: serde
  dependency-version: 1.0.223
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
grod220 pushed a commit that referenced this pull request Mar 18, 2026
@dependabot @grod220
build(deps): bump serde from 1.0.219 to 1.0.223 (#154)
0e10715 
Bumps [serde](https://github.com/serde-rs/serde) from 1.0.219 to 1.0.223.
- [Release notes](https://github.com/serde-rs/serde/releases)
- [Commits](serde-rs/serde@v1.0.219...v1.0.223)

---
updated-dependencies:
- dependency-name: serde
  dependency-version: 1.0.223
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joncinque joncinque joncinque approved these changes

+1 more reviewer

@jstarry jstarry jstarry approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

is_writable_index() may panic when overflow checking is enabled

3 participants

@rustopian @joncinque @jstarry