CPI: improve test coverage #31986

alessandrod · 2023-06-06T13:41:14Z

This adds a number of tests and checks the behavior with direct mapping on and off.

codecov · 2023-06-06T14:44:33Z

Codecov Report

Merging #31986 (bde85d6) into master (8970a37) will decrease coverage by 0.1%.
The diff coverage is 64.6%.

❗ Current head bde85d6 differs from pull request most recent head abc693a. Consider uploading reports for the commit abc693a to get more accurate results

@@            Coverage Diff            @@
##           master   #31986     +/-   ##
=========================================
- Coverage    82.0%    82.0%   -0.1%     
=========================================
  Files         784      784             
  Lines      212534   212755    +221     
=========================================
+ Hits       174341   174460    +119     
- Misses      38193    38295    +102

programs/sbf/tests/programs.rs

ryoqun · 2023-06-07T12:07:58Z

programs/sbf/tests/programs.rs

@@ -3933,3 +3933,98 @@ fn test_program_sbf_inner_instruction_alignment_checks() {
    let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction.clone());
    assert!(result.is_ok());
 }
+
+#[test]


these test looks nice. if it's not too much trouble, how about backporting only these tests to v1.14? after all our test coverage isn't that good, esp for error/edge cases. I think these tests should have existed way before direct mapping...

alessandrod · 2023-06-20T10:28:40Z

Ok this is now ready for another round of reviews:

I've locked down all AccountInfo pointers. In theory I could remove account info parsing entirely for direct mapping, but in the interest of not having the diff get unnecessarily larger I'm planning to do that only in master later.
I've tweaked the serialization code to split realloc padding in its own region so we can set permissions more granularly
I've changed the way CallerAccount::serialized_data is set (or rather not set) when direct mapping is enabled

The diff has grown kinda large, but it's mostly new tests for edge conditions that weren't tested before. I've found some other unrelated, not direct mapping related bugs that I'm going to fix separately

programs/bpf_loader/src/serialization.rs

programs/bpf_loader/src/lib.rs

programs/bpf_loader/src/serialization.rs

alessandrod · 2023-06-20T19:54:22Z

Note to self: this still needs a test for a builtin program that uses AccountSharedData directly to change the backing storage of an account, and a test and fix for ref_to_len_in_vm in the same vein as serialized_data

Done

programs/bpf_loader/src/syscalls/cpi.rs

alessandrod · 2023-07-18T02:25:47Z

Alright this is now ready for another round of reviews! I actually have a couple more tests that I want to add, but they aren't around known issues they're just for coverage, so no reason to delay this. Ideally I want to land and enable in testnet asap.

programs/bpf_loader/src/lib.rs

ryoqun · 2023-08-29T05:31:03Z

seems stable-sbf is failing in CI..

alessandrod · 2023-08-29T12:10:03Z

seems stable-sbf is failing in CI..

fixed, a test had to be updated after the ref_to_len_in_vm >= MM_INPUT_START check

ryoqun

lgtm with nits.

really thanks for the hard work.

…apping

Add TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE_MOVING_DATA_POINTER and TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE that exercise the new logic.

Remove some copy pasta and rename two tests to better describe what they're doing

…tableDataModified

…n changes

yihau · 2023-09-05T07:58:12Z

hey! it seems that this one broke master CI after merged 😢

https://buildkite.com/solana-labs/solana/builds/101262#018a643d-750f-4021-b395-4188ff9a982d

alessandrod · 2023-09-05T08:29:29Z

oops, fixing!

This reverts commit 6679153.

alessandrod force-pushed the dm-cpi-memory-perms branch 2 times, most recently from df1c36b to c6a8b29 Compare June 6, 2023 13:49

ryoqun self-requested a review June 7, 2023 05:43

ryoqun added the v1.16 PRs that should be backported to v1.16 label Jun 7, 2023

ryoqun reviewed Jun 7, 2023

View reviewed changes

programs/sbf/tests/programs.rs Outdated Show resolved Hide resolved

ryoqun reviewed Jun 7, 2023

View reviewed changes

alessandrod force-pushed the dm-cpi-memory-perms branch 6 times, most recently from b00ef5c to fa28958 Compare June 20, 2023 10:21

alessandrod requested review from ryoqun and Lichtso June 20, 2023 10:28

Lichtso reviewed Jun 20, 2023

View reviewed changes

programs/bpf_loader/src/serialization.rs Outdated Show resolved Hide resolved

programs/bpf_loader/src/lib.rs Outdated Show resolved Hide resolved

programs/bpf_loader/src/serialization.rs Outdated Show resolved Hide resolved

ryoqun reviewed Jun 21, 2023

View reviewed changes

programs/bpf_loader/src/syscalls/cpi.rs Outdated Show resolved Hide resolved

alessandrod force-pushed the dm-cpi-memory-perms branch 2 times, most recently from cb7833b to eca410e Compare June 25, 2023 06:28

ryoqun reviewed Jun 27, 2023

View reviewed changes

programs/bpf_loader/src/syscalls/cpi.rs Outdated Show resolved Hide resolved

alessandrod added the work in progress This isn't quite right yet label Jun 27, 2023

alessandrod force-pushed the dm-cpi-memory-perms branch 2 times, most recently from ad813b7 to ed1f9f4 Compare July 7, 2023 15:01

alessandrod force-pushed the dm-cpi-memory-perms branch 3 times, most recently from 8a79403 to 291e0a0 Compare July 18, 2023 02:24

alessandrod removed the work in progress This isn't quite right yet label Jul 18, 2023

alessandrod force-pushed the dm-cpi-memory-perms branch from bde85d6 to abc693a Compare August 23, 2023 17:03

ryoqun reviewed Aug 28, 2023

View reviewed changes

programs/bpf_loader/src/lib.rs Outdated Show resolved Hide resolved

alessandrod force-pushed the dm-cpi-memory-perms branch from abc693a to e711470 Compare August 29, 2023 01:38

ryoqun self-requested a review August 29, 2023 04:51

alessandrod force-pushed the dm-cpi-memory-perms branch 3 times, most recently from 9ec1798 to c0cb53e Compare August 29, 2023 11:11

ryoqun previously approved these changes Aug 29, 2023

View reviewed changes

alessandrod dismissed ryoqun’s stale review via 225fdff August 30, 2023 00:34

alessandrod force-pushed the dm-cpi-memory-perms branch from c0cb53e to 225fdff Compare August 30, 2023 00:34

alessandrod added 10 commits August 30, 2023 21:09

programs/sbf: add TEST_[FORBID|ALLOW]_WRITE_AFTER_OWNERSHIP_CHANGE*

1ffe12f

programs/sbf: add tests for the AccessViolation -> InstructionError m…

a81bb5f

…apping

cpi: add more tests

3f4682c

programs/sbf: add tests for immutable AccountInfo pointers

2bed5e8

programs/sbf: add tests for verification of SolAccountInfo pointers too

2a57e92

programs/sbf: add tests for ref_to_len_in_vm handling in CPI

c57a089

Add TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE_MOVING_DATA_POINTER and TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE that exercise the new logic.

cpi: tweak tests

e57c287

Remove some copy pasta and rename two tests to better describe what they're doing

cpi: add tests that check that CPI updates all accounts at once

b11bfd4

direct mapping: test that writes to executable accounts trigger Execu…

7fda2fc

…tableDataModified

programs/sbf: add explicit tests for when an account's data allocatio…

08942d9

…n changes

alessandrod force-pushed the dm-cpi-memory-perms branch from 225fdff to 08942d9 Compare August 30, 2023 14:10

Lichtso approved these changes Sep 5, 2023

View reviewed changes

alessandrod changed the title ~~direct mapping: update memory permissions after CPI~~ CPI: improve test coverage Sep 5, 2023

alessandrod merged commit 6679153 into solana-labs:master Sep 5, 2023

behzadnouri added a commit to behzadnouri/solana that referenced this pull request Sep 5, 2023

Revert "CPI: improve test coverage (solana-labs#31986)"

597733d

This reverts commit 6679153.

behzadnouri added a commit to behzadnouri/solana that referenced this pull request Sep 5, 2023

Revert "CPI: improve test coverage (solana-labs#31986)"

6259062

This reverts commit 6679153.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPI: improve test coverage #31986

CPI: improve test coverage #31986

alessandrod commented Jun 6, 2023 •

edited

Loading

codecov bot commented Jun 6, 2023 •

edited

Loading

ryoqun Jun 7, 2023

alessandrod commented Jun 20, 2023

alessandrod commented Jun 20, 2023 •

edited

Loading

alessandrod commented Jul 18, 2023

ryoqun commented Aug 29, 2023

alessandrod commented Aug 29, 2023

ryoqun left a comment

yihau commented Sep 5, 2023

alessandrod commented Sep 5, 2023

CPI: improve test coverage #31986

CPI: improve test coverage #31986

Conversation

alessandrod commented Jun 6, 2023 • edited Loading

codecov bot commented Jun 6, 2023 • edited Loading

Codecov Report

ryoqun Jun 7, 2023

Choose a reason for hiding this comment

alessandrod commented Jun 20, 2023

alessandrod commented Jun 20, 2023 • edited Loading

alessandrod commented Jul 18, 2023

ryoqun commented Aug 29, 2023

alessandrod commented Aug 29, 2023

ryoqun left a comment

Choose a reason for hiding this comment

yihau commented Sep 5, 2023

alessandrod commented Sep 5, 2023

alessandrod commented Jun 6, 2023 •

edited

Loading

codecov bot commented Jun 6, 2023 •

edited

Loading

alessandrod commented Jun 20, 2023 •

edited

Loading