-
Notifications
You must be signed in to change notification settings - Fork 73
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
fix: avoid raising internal python error on large memory access #442
Conversation
these can otherwise raise `OverflowError: cannot fit 'int' into an index-sized integer`
h/t @m4k2 |
awesome, could you also add these nice examples as tests? |
do you think they fit as unit tests? The problem is that in both cases we get an error in both cases (but a different flavor), so the returncode is just 1 anyway right? |
actually I think I have a decent pattern to test for this: function test_megaMem_sha3(bool coinflip) external {
bytes32 hash;
uint256 size = coinflip ? MEGA_SIZE : 32;
assembly {
hash := keccak256(0, size)
}
}
|
in 0d2aab3 |
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.
great, thanks for thoughtfully handling all the various cases!
src/halmos/sevm.py
Outdated
|
||
if not isinstance(ret_, ByteVec): | ||
raise HalmosException(f"Invalid return value: {ret_}") | ||
data = ret_.slice(0, effective_ret_size) |
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.
if i understand correctly, this needs the full scan even if effective_ret_size == len(ret_)
. could you make sure slice() returns immediately in such case?
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.
good point, let me look into it
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.
in 48424ca
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.
48424ca looks good. but it addressed the same issue in a different location, leaving this one unaddressed. alternatively, you might consider improving the bytevec.slice(0, size) function to immediately return when size is the full length?
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 might consider improving the bytevec.slice(0, size) function to immediately return when size is the full length?
that may be a little tricky to handle properly, because bytevecs are mutable. So in principle we need to return a copy, but in this case we can skip the copy because we just blit it in memory and we know we're not going to modify the returndata any further
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.
I'll handle it at this callsite and we can think about reducing copies further or introducing immutable bytevecs in a separate PR
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.
sorry for the confusion, I refactored both places to call the same function: ebc6da6
offset, offset + size | ||
) | ||
ex.st.memory.set_slice(loc, end_loc, codeslice) | ||
codeslice: ByteVec = account_code.slice(offset, size) |
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.
oh good catch!
keccak256_op was optimized to return(0, 0), which does not revert, which caused the test to fail for context, newer foundry versions have the optimizer turned off (but ci uses the stable channel of foundry, which still has the optimizer turned on by default)
these can otherwise raise
OverflowError: cannot fit 'int' into an index-sized integer
For example: