Return error if Transaction contains writable executable or ProgramData accounts by CriesofCarrots · Pull Request #19629 · solana-labs/solana

CriesofCarrots · 2021-09-03T23:14:46Z

Problem

Accounts marked executable can only be written to if being upgraded (ie. if the UpgradeableLoader is present in the transaction). As of #19593, we're demoting top-level program ids incorrectly marked as writable, but we can't detect other executables until the accounts are loaded.
Similarly, ProgramData accounts can only be written if the UpgradeableLoader is present in the transaction, but we can't identify these accounts until they are loaded.

Summary of Changes

Return InvalidAccountIndex error to fail these types of Transactions and release the write lock quickly. Open to creating a new error type, if this seems significant enough.

Needs rebase on #19593
@jackcmay @jstarry , the last 3 commits are ready for review

jackcmay · 2021-09-03T23:35:07Z

Last 3 commits look good so far!

…loader isn't present

CriesofCarrots · 2021-09-04T07:54:02Z

Ready for final review!

jackcmay · 2021-09-07T19:15:48Z

-                                return Err(TransactionError::InvalidProgramForExecution);
+                                // If a ProgramData account is present, it should only be writable
+                                // if the bpf_loader_upgradeable account is also present
+                                if let Ok(UpgradeableLoaderState::ProgramData { .. }) =


Would it be more efficient to first check the books and then deserialize?

Could we skip the deserialization altogether and disallow writes to any account owned by the upgradeable loader? The only time a program other than the upgradeable loader can write to one of these accounts is if they top up the lamports which doesn't seem useful anyways.

jstarry · 2021-09-07T22:05:49Z

-                                return Err(TransactionError::InvalidProgramForExecution);
+                                // If a ProgramData account is present, it should only be writable
+                                // if the bpf_loader_upgradeable account is also present
+                                if let Ok(UpgradeableLoaderState::ProgramData { .. }) =


Could we skip the deserialization altogether and disallow writes to any account owned by the upgradeable loader? The only time a program other than the upgradeable loader can write to one of these accounts is if they top up the lamports which doesn't seem useful anyways.

jackcmay · 2021-09-07T23:36:21Z

Adding more lamports might be necessary when we add support for realloc...

jstarry · 2021-09-08T00:00:07Z

True, do you think the bpf loader program could be responsible for that?

jackcmay · 2021-09-08T00:20:52Z

I think so, the current plan is continue funding the account via the buffer account, so realloc to larger ProgramData account would require a larger buffer account with a larger associated number of tokens

CriesofCarrots · 2021-09-08T06:14:03Z

I'm not clear, @jackcmay , are you saying the upgradeable-loader program need to be present to do that sort of realloc?
If so, then I think we can skip the deserialization, because any type of UpgradeableLoaderState woudl require the program be present for writes.

jackcmay · 2021-09-08T06:44:24Z

In order for a program to be realloc'd larger, the ProgramData account's balance will have to increase in order to still be rent-exempt. The question was who/how transfers those lamports. The current plan is for the larger buffer account to ferry along those lamports which means the upgradeable loader will always exist for all program reallocs. aka, we can probably skip deserialization as Justin suggested.

…sent; remove is_upgradeable_loader_present exception for all other executables

CriesofCarrots · 2021-09-08T18:11:55Z

Okay, the last commit changes the logic to return an error if:

A writable account is owned by the upgradeable loader, but the upgradeable loader is not included in the tx
OR, a writable account is executable

Look sufficient @jackcmay @jstarry ?

codecov · 2021-09-08T19:02:31Z

Codecov Report

Merging #19629 (9a5213e) into master (decec3c) will increase coverage by 0.0%.
The diff coverage is 96.0%.

@@           Coverage Diff            @@
##           master   #19629    +/-   ##
========================================
  Coverage    82.5%    82.5%            
========================================
  Files         468      468            
  Lines      131847   132036   +189     
========================================
+ Hits       108774   109036   +262     
+ Misses      23073    23000    -73

…ta accounts (#19629) * Return error if Transaction locks an executable as writable * Return error if a ProgramData account is writable but the upgradable loader isn't present * Remove unreachable clause * Fixup bpf tests * Review comments * Add new TransactionError * Disallow writes to any upgradeable-loader account when loader not present; remove is_upgradeable_loader_present exception for all other executables (cherry picked from commit 38bbb77) # Conflicts: # programs/bpf/tests/programs.rs # runtime/src/accounts.rs # runtime/src/bank.rs # sdk/src/transaction.rs # storage-proto/proto/transaction_by_addr.proto # storage-proto/src/convert.rs

…ta accounts (backport #19629) (#19729) * Return error if Transaction contains writable executable or ProgramData accounts (#19629) * Return error if Transaction locks an executable as writable * Return error if a ProgramData account is writable but the upgradable loader isn't present * Remove unreachable clause * Fixup bpf tests * Review comments * Add new TransactionError * Disallow writes to any upgradeable-loader account when loader not present; remove is_upgradeable_loader_present exception for all other executables (cherry picked from commit 38bbb77) # Conflicts: # programs/bpf/tests/programs.rs # runtime/src/accounts.rs # runtime/src/bank.rs # sdk/src/transaction.rs # storage-proto/proto/transaction_by_addr.proto # storage-proto/src/convert.rs * Fix conflicts Co-authored-by: Tyera Eulberg <teulberg@gmail.com> Co-authored-by: Tyera Eulberg <tyera@solana.com>

…ta accounts (backport #19629) (#19730) * Return error if Transaction contains writable executable or ProgramData accounts (#19629) * Return error if Transaction locks an executable as writable * Return error if a ProgramData account is writable but the upgradable loader isn't present * Remove unreachable clause * Fixup bpf tests * Review comments * Add new TransactionError * Disallow writes to any upgradeable-loader account when loader not present; remove is_upgradeable_loader_present exception for all other executables (cherry picked from commit 38bbb77) # Conflicts: # programs/bpf/tests/programs.rs # runtime/src/accounts.rs # runtime/src/bank.rs # sdk/src/transaction.rs # storage-proto/proto/transaction_by_addr.proto # storage-proto/src/convert.rs * Fix conflicts Co-authored-by: Tyera Eulberg <teulberg@gmail.com> Co-authored-by: Tyera Eulberg <tyera@solana.com>

…ta accounts (solana-labs#19629) * Return error if Transaction locks an executable as writable * Return error if a ProgramData account is writable but the upgradable loader isn't present * Remove unreachable clause * Fixup bpf tests * Review comments * Add new TransactionError * Disallow writes to any upgradeable-loader account when loader not present; remove is_upgradeable_loader_present exception for all other executables

…rogramData accounts (solana-labs#19629)" This reverts commit 737475e.

CriesofCarrots added v1.6 labels Sep 3, 2021

CriesofCarrots requested review from jackcmay and jstarry September 3, 2021 23:14

jstarry reviewed Sep 3, 2021

View reviewed changes

Comment thread runtime/src/accounts.rs Outdated

Comment thread runtime/src/accounts.rs Outdated

CriesofCarrots force-pushed the cpi-writable-program-tx branch 2 times, most recently from bcffe21 to 26184e8 Compare September 4, 2021 01:17

KeKo6988 approved these changes Sep 4, 2021

View reviewed changes

CriesofCarrots force-pushed the cpi-writable-program-tx branch from 26184e8 to e6a1ea2 Compare September 4, 2021 02:22

Tyera Eulberg added 3 commits September 3, 2021 21:29

Return error if Transaction locks an executable as writable

b75d0c3

Return error if a ProgramData account is writable but the upgradable …

0eca269

…loader isn't present

Remove unreachable clause

bb85c6b

CriesofCarrots force-pushed the cpi-writable-program-tx branch from e6a1ea2 to bb85c6b Compare September 4, 2021 03:29

Fixup bpf tests

c29316f

sakridge reviewed Sep 7, 2021

View reviewed changes

Comment thread runtime/src/accounts.rs Outdated

jackcmay reviewed Sep 7, 2021

View reviewed changes

Comment thread runtime/src/accounts.rs Outdated

jackcmay reviewed Sep 7, 2021

View reviewed changes

Comment thread runtime/src/accounts.rs Outdated

CriesofCarrots force-pushed the cpi-writable-program-tx branch from 9321e88 to b77314d Compare September 7, 2021 18:58

jackcmay reviewed Sep 7, 2021

View reviewed changes

CriesofCarrots force-pushed the cpi-writable-program-tx branch from b77314d to 11224c1 Compare September 7, 2021 19:42

Tyera Eulberg added 2 commits September 7, 2021 14:44

Review comments

ee28a52

Add new TransactionError

a9fdc52

CriesofCarrots force-pushed the cpi-writable-program-tx branch from 11224c1 to a9fdc52 Compare September 7, 2021 19:45

jackcmay reviewed Sep 7, 2021

View reviewed changes

Comment thread runtime/src/accounts.rs

jstarry reviewed Sep 7, 2021

View reviewed changes

Disallow writes to any upgradeable-loader account when loader not pre…

9a5213e

…sent; remove is_upgradeable_loader_present exception for all other executables

CriesofCarrots force-pushed the cpi-writable-program-tx branch from 283554b to 9a5213e Compare September 8, 2021 16:24

CriesofCarrots merged commit 38bbb77 into solana-labs:master Sep 8, 2021

mergify Bot mentioned this pull request Sep 8, 2021

Return error if Transaction contains writable executable or ProgramData accounts (backport #19629) #19729

Merged

mergify Bot mentioned this pull request Sep 8, 2021

Return error if Transaction contains writable executable or ProgramData accounts (backport #19629) #19730

Merged

Lichtso mentioned this pull request Sep 11, 2021

Index loaders / executable accounts #19469

Merged

frits-metalogix added a commit to identity-com/solana that referenced this pull request Nov 24, 2021

Revert "Return error if Transaction contains writable executable or P…

d91134e

…rogramData accounts (solana-labs#19629)" This reverts commit 737475e.

Conversation

CriesofCarrots commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

jackcmay commented Sep 3, 2021

Uh oh!

Uh oh!

Uh oh!

CriesofCarrots commented Sep 4, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jackcmay Sep 7, 2021

Choose a reason for hiding this comment

Uh oh!

jstarry Sep 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jstarry Sep 7, 2021

Choose a reason for hiding this comment

Uh oh!

jackcmay commented Sep 7, 2021

Uh oh!

jstarry commented Sep 8, 2021

Uh oh!

jackcmay commented Sep 8, 2021

Uh oh!

CriesofCarrots commented Sep 8, 2021

Uh oh!

jackcmay commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CriesofCarrots commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Sep 8, 2021

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CriesofCarrots commented Sep 3, 2021 •

edited

Loading

jackcmay commented Sep 8, 2021 •

edited

Loading

CriesofCarrots commented Sep 8, 2021 •

edited

Loading