-
Notifications
You must be signed in to change notification settings - Fork 381
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
v2.1: Check account boundaries on overlapping memmove syscall (backport of #4563) #4598
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
* fix reverse memmove (cherry picked from commit 4b89b0f)
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -419,7 +419,7 @@ struct MemoryChunkIterator<'a> { | |
// exclusive end index (start + len, so one past the last valid address) | ||
vm_addr_end: u64, | ||
len: u64, | ||
account_index: usize, | ||
account_index: Option<usize>, | ||
is_account: Option<bool>, | ||
} | ||
|
||
|
@@ -446,7 +446,7 @@ impl<'a> MemoryChunkIterator<'a> { | |
len, | ||
vm_addr_start: vm_addr, | ||
vm_addr_end, | ||
account_index: 0, | ||
account_index: None, | ||
is_account: None, | ||
}) | ||
} | ||
|
@@ -490,14 +490,18 @@ impl<'a> Iterator for MemoryChunkIterator<'a> { | |
|
||
let region_is_account; | ||
|
||
let mut account_index = self.account_index.unwrap_or_default(); | ||
self.account_index = Some(account_index); | ||
|
||
loop { | ||
if let Some(account) = self.accounts.get(self.account_index) { | ||
if let Some(account) = self.accounts.get(account_index) { | ||
let account_addr = account.vm_data_addr; | ||
let resize_addr = account_addr.saturating_add(account.original_data_len as u64); | ||
|
||
if resize_addr < region.vm_addr { | ||
// region is after this account, move on next one | ||
self.account_index = self.account_index.saturating_add(1); | ||
account_index = account_index.saturating_add(1); | ||
self.account_index = Some(account_index); | ||
} else { | ||
region_is_account = | ||
region.vm_addr == account_addr || region.vm_addr == resize_addr; | ||
|
@@ -550,6 +554,41 @@ impl<'a> DoubleEndedIterator for MemoryChunkIterator<'a> { | |
} | ||
}; | ||
|
||
let region_is_account; | ||
|
||
let mut account_index = self | ||
.account_index | ||
.unwrap_or_else(|| self.accounts.len().saturating_sub(1)); | ||
self.account_index = Some(account_index); | ||
|
||
loop { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This loop could use some comments, it took me half an hour to understand what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's true for a lot of runtime code. I'll review and add some comments |
||
let Some(account) = self.accounts.get(account_index) else { | ||
// address is after all the accounts | ||
region_is_account = false; | ||
break; | ||
}; | ||
|
||
let account_addr = account.vm_data_addr; | ||
let resize_addr = account_addr.saturating_add(account.original_data_len as u64); | ||
|
||
if account_index > 0 && account_addr > region.vm_addr { | ||
account_index = account_index.saturating_sub(1); | ||
|
||
self.account_index = Some(account_index); | ||
} else { | ||
region_is_account = region.vm_addr == account_addr || region.vm_addr == resize_addr; | ||
break; | ||
} | ||
} | ||
|
||
if let Some(is_account) = self.is_account { | ||
if is_account != region_is_account { | ||
return Some(Err(SyscallError::InvalidLength.into())); | ||
} | ||
} else { | ||
self.is_account = Some(region_is_account); | ||
} | ||
|
||
let chunk_len = if region.vm_addr >= self.vm_addr_start { | ||
// consume the whole region | ||
let len = self.vm_addr_end.saturating_sub(region.vm_addr); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my bad for not reviewing the master PR, but shouldn't these tests all test:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add tests on master There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #4650 |
||
unsafe { sol_memmove(too_early(0).as_mut_ptr(), too_early(3).as_ptr(), 10) }; | ||
} | ||
15 => { | ||
// memmove dst overlaps end of account, reverse order | ||
unsafe { | ||
sol_memmove( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. which pointer here is causing the failure? I think they are both wrong? what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the destination falls within src + length There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. data[data_len..].as_mut_ptr() + 10 <- isn't this outside the account region? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I misread your comment. I've added a bunch of tests: #4650 |
||
data[data_len..].as_mut_ptr(), | ||
data[data_len.saturating_sub(3)..].as_mut_ptr(), | ||
10, | ||
) | ||
}; | ||
} | ||
_ => {} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_or_insert_with()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, this is nicer. Thanks! I'll create a PR on master