Skip to content
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

OOB memory access test cases #34

Closed
jwasinger opened this issue May 22, 2018 · 6 comments
Closed

OOB memory access test cases #34

jwasinger opened this issue May 22, 2018 · 6 comments
Assignees

Comments

@jwasinger
Copy link

jwasinger commented May 22, 2018

As we are deviating from EVM1 with regards to how the EEI treats reads from OOB memory (see ewasm/hera#222), we need test cases that capture this spec change.

externalCodeCopy, codeCopy and returnDataCopy need to have test cases to make sure that: copying from OOB memory traps.

Cf: ewasm/evm2wasm#250

@lrettig
Copy link
Member

lrettig commented Jun 22, 2018

I'm confused. The EVM behavior, and the behavior of existing clients, is not to trap but instead to just fill in the memory with zeroes as you suggested in ewasm/hera#221. If Hera exhibits different behavior then existing EVM tests will fail. Which behavior should we be testing for?

In particular, since Hera traps, all remaining gas gets consumed, which is not what EVM does.

CC @jwasinger @axic

@cdetrio
Copy link

cdetrio commented Jun 22, 2018

EVM behavior of RETURNDATACOPY is different from CALLDATACOPY. callDataCopy will copy zero bytes if given offsets beyond the calldata size. RETURNDATACOPY will throw an exception:

https://github.com/ethereum/EIPs/blob/06df3ce690e5c214e02d720658cd2b326f1db6cc/EIPS/eip-211.md

This opcode has similar semantics to CALLDATACOPY, but instead of copying data from the call data, it copies data from the return data buffer. Furthermore, accessing the return data buffer beyond its size results in a failure; i.e. if start + length overflows or results in a value larger than RETURNDATASIZE, the current call stops in an out-of-gas condition. In particular, reading 0 bytes from the end of the buffer will read 0 bytes; reading 0 bytes from one-byte out of the buffer causes an exception.

@lrettig
Copy link
Member

lrettig commented Jun 22, 2018

So we want to match the EVM behavior here as well, right?

@lrettig
Copy link
Member

lrettig commented Jun 24, 2018

It looks like codecopy and extcodecopy do not trap, only returndatacopy. I'll add tests for all of this expected behavior.

@lrettig
Copy link
Member

lrettig commented Jun 25, 2018

I'm still a little confused by @axic's comments in ewasm/hera#222:

I think we tried to break away from that EVM nonsense in ewasm

Are we mimicking EVM behavior as discussed in this thread or are we "breaking away"? And if so what does that mean?

@axic
Copy link
Member

axic commented Jun 25, 2018

It looks like codecopy and extcodecopy do not trap, only returndatacopy. I'll add tests for all of this expected behavior.
So we want to match the EVM behavior here as well, right?

Yes, the idea would be to do copy the logic from returndatacopy. That is a small improvement we can make over EVM :)

Though the test cases should match what Hera is doing and then we can change Hera. That change should trigger a test failure (showing that the tests indeed are useful) and we can adjust them.

lrettig added a commit to lrettig/tests that referenced this issue Jul 27, 2018
Test both fully and partially OOB cases.
Revert to existing Hera behavior (trapping) per
ewasm#34 rather than expecting zero
padding.
lrettig added a commit to lrettig/tests that referenced this issue Jul 30, 2018
Test both fully and partially OOB cases.
Revert to existing Hera behavior (trapping) per
ewasm#34 rather than expecting zero
padding.
lrettig added a commit that referenced this issue Jul 30, 2018
* Adds new codeCopyOOB test

Tests the effect of copying code from out-of-bounds location.

* Add new extCodeCopyOOB test

Test behavior of out-of-bounds extCodeCopy. Contains two checks, one for
totally out of bounds copy and one for partial out of bounds copy.

* WIP: writing returnDataCopyOOB test

Writing one test that runs a series of other contracts, each of which
calls returnDataCopy out-of-bounds in slightly different ways, to make
sure each call fails.

Running into a hera issue now with the first such test.

* Split codeCopyOOB into two tests

Test both fully and partially OOB cases.
Revert to existing Hera behavior (trapping) per
#34 rather than expecting zero
padding.

* Fix typo

* Split extCodeCopyOOB test into two simpler tests

Tests both fully and partially OOB cases.
Expect hera to trap, even though it currently zero-pads.

* Delete old fillers

* Remove debugging

* Rework the returnDataCopy test and split it up

Split this complex multi-level test into three separate, simpler tests

* Update filled tests
hugo-dc pushed a commit that referenced this issue Nov 3, 2018
* Adds new codeCopyOOB test

Tests the effect of copying code from out-of-bounds location.

* Add new extCodeCopyOOB test

Test behavior of out-of-bounds extCodeCopy. Contains two checks, one for
totally out of bounds copy and one for partial out of bounds copy.

* WIP: writing returnDataCopyOOB test

Writing one test that runs a series of other contracts, each of which
calls returnDataCopy out-of-bounds in slightly different ways, to make
sure each call fails.

Running into a hera issue now with the first such test.

* Split codeCopyOOB into two tests

Test both fully and partially OOB cases.
Revert to existing Hera behavior (trapping) per
#34 rather than expecting zero
padding.

* Fix typo

* Split extCodeCopyOOB test into two simpler tests

Tests both fully and partially OOB cases.
Expect hera to trap, even though it currently zero-pads.

* Delete old fillers

* Remove debugging

* Rework the returnDataCopy test and split it up

Split this complex multi-level test into three separate, simpler tests

* Update filled tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants