Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v1.18: Ensure mapping of callee is updated with direct mapping (backport of #1093) #1779

Closed
wants to merge 1 commit into from

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Jun 18, 2024

Consider this scenario:

  • Program increases length of an account
  • Program start CPI and adds this account as a read-only account
  • In fn update_callee_account() we resize account, which may change the pointer
  • Once CPI finishes, the program continues and may read/write from the account. The mapping must be up-to-date else we use stale pointers.

Note that we always call callee_account.set_data_length(), which may change the pointer. In testing I found that resizing a vector from 10240 down to 127 sometimes changes its pointer. So, always update the pointer.

Problem

Summary of Changes

Fixes #


This is an automatic backport of pull request #1093 done by [Mergify](https://mergify.com).

@mergify mergify bot requested a review from a team as a code owner June 18, 2024 09:21
@mergify mergify bot added the conflicts label Jun 18, 2024
Copy link
Author

mergify bot commented Jun 18, 2024

Cherry-pick of cd7f34e has failed:

On branch mergify/bp/v1.18/pr-1093
Your branch is up to date with 'origin/v1.18'.

You are currently cherry-picking commit cd7f34ecfc.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   programs/bpf_loader/src/syscalls/cpi.rs
	modified:   programs/sbf/rust/invoke/src/instructions.rs
	modified:   programs/sbf/tests/programs.rs

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   programs/sbf/rust/invoke/src/lib.rs

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

Consider this scenario:

 - Program increases length of an account
 - Program start CPI and adds this account as a read-only account
 - In fn update_callee_account() we resize account, which may change
   the pointer
 - Once CPI finishes, the program continues and may read/write from
   the account. The mapping must be up-to-date else we use stale
   pointers.

Note that we always call callee_account.set_data_length(), which
may change the pointer. In testing I found that resizing a vector
from 10240 down to 127 sometimes changes its pointer. So, always
update the pointer.

(cherry picked from commit cd7f34e)
@seanyoung seanyoung force-pushed the mergify/bp/v1.18/pr-1093 branch from ce5f3f1 to b7d9263 Compare June 18, 2024 09:42
@seanyoung seanyoung requested a review from alessandrod June 18, 2024 09:43
@seanyoung
Copy link

  • The non-direct mapping code path is unchanged.
  • We would like to enable direct mapping on testnet as soon as
  • This change is needed to enable direct mapping on testnet

@willhickey
Copy link

This change is needed to enable direct mapping on testnet

Testnet will adopt v2.0 (what is currently master) soon (hopefully within a week or two). I think this warrants backporting anyway, but the testnet direct mapping won't be blocked if we don't

@sakridge
Copy link

This change is needed to enable direct mapping on testnet

Testnet will adopt v2.0 (what is currently master) soon (hopefully within a week or two). I think this warrants backporting anyway, but the testnet direct mapping won't be blocked if we don't

Yes, this can be tested with v2.0 so I'm inclined to have it just wait for that and not risk 1.18.

@seanyoung seanyoung closed this Jun 20, 2024
@mergify mergify bot deleted the mergify/bp/v1.18/pr-1093 branch June 20, 2024 14:30
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.

4 participants