v2.1: Check account boundaries on overlapping memmove syscall (backport of #4563)#4598
v2.1: Check account boundaries on overlapping memmove syscall (backport of #4563)#4598
Conversation
* 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
alessandrod
left a comment
alessandrod
left a comment
There was a problem hiding this comment.
I'm going to approve but i think we have missing coverage
| } | ||
|
|
||
| 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
4 participants
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).