v2.1: Check account boundaries on overlapping memmove syscall (backport of #4563) #4598
Conversation
![mergify[bot]](https://avatars.githubusercontent.com/in/10562?s=60&v=4){kind=small}
* fix reverse memmove (cherry picked from commit 4b89b0f)
|
The Firedancer team maintains a line-for-line reimplementation of the |
| let mut account_index = self | ||
| .account_index | ||
| .unwrap_or_else(|| self.accounts.len().saturating_sub(1)); | ||
| self.account_index = Some(account_index); |
There was a problem hiding this comment.
You're right, this is nicer. Thanks! I'll create a PR on master
| @@ -108,7 +108,20 @@ pub fn process_instruction( | |||
| // memmov dst overlaps begin of account | |||
| unsafe { sol_memmove(too_early(3).as_mut_ptr(), buf.as_ptr(), 10) }; | |||
| } | |||
|
|
|||
| 14 => { | |||
| // memmove dst overlaps begin of account, reverse order | |||
There was a problem hiding this comment.
my bad for not reviewing the master PR, but shouldn't these tests all test:
- success in the boundary condition: dst=too_early(0), src=too_early(0)
- failure at boundary-1: dst=too_early(0), src=too_early(1)
- failure at boundary-N: dst=too_early(0), src=too_early(N)
| 15 => { | ||
| // memmove dst overlaps end of account, reverse order | ||
| unsafe { | ||
| sol_memmove( |
There was a problem hiding this comment.
which pointer here is causing the failure? I think they are both wrong? what
about the case where dst is ok but src is wrong? and vice versa?
There was a problem hiding this comment.
the destination falls within src + length
There was a problem hiding this comment.
data[data_len..].as_mut_ptr() + 10 <- isn't this outside the account region?
There was a problem hiding this comment.
Sorry, I misread your comment. I've added a bunch of tests: #4650
| .unwrap_or_else(|| self.accounts.len().saturating_sub(1)); | ||
| self.account_index = Some(account_index); | ||
|
|
||
| loop { |
There was a problem hiding this comment.
This loop could use some comments, it took me half an hour to understand what
was going on 😅
There was a problem hiding this comment.
That's true for a lot of runtime code. I'll review and add some comments
Problem
Since SIMD-0219, any memmove cannot cross account data and non-account data boundaries. This was implemented in PR 3744.
However there was a check missing: memmove may do an overlapping copy which requires reverse iteration, and this code path did not have the check in place.
Summary of Changes
Found by @yufeng-jump and @mjain-jump
This is an automatic backport of pull request #4563 done by [Mergify](https://mergify.com).