Fix a crash in CCP and zero-filling issues in CCP and LDC #524
Conversation
|
Could you give me the expected behavior for each of the bugs this PR is addressing? For example "out-of-contract-bounds input" sounds like it should result in unhappy behavior, if not "crash" what should it do? |
Sure: The expected behavior is to follow the spec for each of these. Specifically: CCP instruction could crash when given out-of-contract-bounds input.
That's a bit inexact, as here CCP instruction would only zero memory when asked to copy more bytes than the contract has.The expected behavior is that LDC instruction would revert when asked more to load more bytes than the contract has.
Spec requires zero-extending the part outside the contract bounds, but we were reverting instead of doing that. |
| || contract_id_end as Word > VM_MAX_RAM | ||
| || length > MEM_MAX_ACCESS_SIZE as usize | ||
| || length > self.contract_max_size as usize | ||
| if memory_offset.saturating_add(length) > *self.hp |
There was a problem hiding this comment.
Why did this change from >= hp to > hp? Since this variable is only used in a method that says noownershipchecks, wouldn't that mean this could allow a stack overflow?
There was a problem hiding this comment.
This was fixed and cleaned up in #511. I merged master to this branch just now.
| @@ -140,6 +140,13 @@ impl MemoryRange { | |||
| Self(start..end) | |||
| } | |||
|
|
|||
| /// Splits range at given relative offset. Panics if offset > range length. | |||
| pub fn split_at_offset(self, at: usize) -> (Self, Self) { | |||
There was a problem hiding this comment.
Maybe we should return an optional return type here instead of using a panicking assert? Since this is a public api it may be hard to guarantee that it's used correctly in the future.
There was a problem hiding this comment.
It seems you use this function only in one place, so maybe it is better to move this code there instead.
There was a problem hiding this comment.
The API here mirrors slice::split_at from core, including the panic behavior. The code is written here instead of the call site since it's a useful abstraction, and IMO makes the code more readable.
| @@ -140,6 +140,13 @@ impl MemoryRange { | |||
| Self(start..end) | |||
| } | |||
|
|
|||
| /// Splits range at given relative offset. Panics if offset > range length. | |||
| pub fn split_at_offset(self, at: usize) -> (Self, Self) { | |||
There was a problem hiding this comment.
It seems you use this function only in one place, so maybe it is better to move this code there instead.
| dst_addr: Word, | ||
| contract_id_addr: Word, | ||
| contract_offset: Word, | ||
| length: Word, |
There was a problem hiding this comment.
Do we need to worry about padding here? Or is it the responsibility of the CCP user?
There was a problem hiding this comment.
hmm it's not in the specs. I guess since this isn't meant to be executable code it's not so relevant here.
Fixes #523.
Partial overlap with #511, but the order doesn't matter much.Addresses the following issues:
As a side not, the logic around
$fp->code_sizeseems quite complicated and it looks like it's not used for anything. Maybe we could remove it?