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

EVM: Fix data copying logic #1021

Closed
Stebalien opened this issue Oct 24, 2022 · 5 comments
Closed

EVM: Fix data copying logic #1021

Stebalien opened this issue Oct 24, 2022 · 5 comments
Assignees
Labels
Kind: Bug Something isn't working P1 P1: Must be resolved Topic: EVM runtime

Comments

@Stebalien
Copy link
Member

The EVM generally treats memory as "infinite", limited only by gas.

E.g., if a user asks for bytes 5-15 out of a 10 byte return value, the EVM will:

  1. Write bytes 5-10 to the return slice.
  2. Zero the rest of the return slice.

If a user asks for, say, bytes starting at 2^200, the EVM will not fail. It'll just return a bunch of zeros.

We need to fix:

  • CALL (and friends)
  • EXTCODECOPY

And probably quite a few other opcodes to follow this behavior.

@Stebalien Stebalien added Kind: Bug Something isn't working P1 P1: Must be resolved Topic: EVM runtime labels Oct 24, 2022
@Stebalien Stebalien added this to the M2.1 milestone Oct 24, 2022
@vyzo
Copy link
Contributor

vyzo commented Oct 24, 2022

I think we might want to put the brakes for insane behaviour for the sake of spec compliance.

I might as well have a sane interpreter that is not exactly EVM at the rough edges than go out of my way to support insane behaviour.

Just my EUR0.02.

@Stebalien
Copy link
Member Author

We need to match the EVM, especially in cases like this, because people will likely be relying on this behavior. It's actually pretty convenient and important. E.g., if a contract reverts, it won't return anything, but the return value/offset still need to be correct.

In general, we need to default to "whatever the EVM does". We can differ, but we need to know and understand why the EVM behaves the way it does first and need to understand the repercussions. Otherwise, https://en.wiktionary.org/wiki/Chesterton's_fence.

@Stebalien
Copy link
Member Author

Ok, so, some history:

https://github.com/ethereum/pm/blob/master/AllCoreDevs-Meetings/Meeting%2013.md#making-returndatacopy-and-calldata-to-throw-in-certain-situations-nick

ethereum/EIPs#211 (comment)

Bascially, this is considered a mistake, but there are at least some compilers relying on it for optimizations.

The only contracts so far that this has been found in is Augur related Serpent contracts due to a compiler optimizations in Serpent.

While serpent itself is deprecated, I'm not sure if we're in a position to make a judgment call here and change the behavior.

@Stebalien
Copy link
Member Author

Ok, so, I've been listening to the meeting. Relevant section https://youtu.be/aGeGvZ5uS8s?t=3785

It really sounds like nobody should actually use this behavior.

This behavior also explains (cc @mriise) the insane right-pad behavior in precompiles.

We'll discuss this in the huddle tomorrow, but I'm up for at least considering dropping this. If we did:

  1. I'd keep this behavior for CALLDATALOAD (we've already implemented that). Solidity uses this to lookup the EVM function hash, and relies on always being able to call CALLDATALOAD(0) even if passed no arguments.
  2. I'd remove this behavior from the precompiles (sorry @mriise). The precompiles right-pad with zeros to emulate CALLDATACOPY.
  3. We need to be consistent. Right now:
    1. EXTCODECOPY uses copy_to_memory returns an error on out of bounds reads.
    2. CALLDATACOPY fills with zeros.
    3. RETURNDATACOPY returns an error on out of bounds reads.

What I really don't want to do is to leave this in an inconsistent state. If we're going to enforce memory bounds, we need to be consistent.

@Stebalien
Copy link
Member Author

Ok, nope, we're not touching this:

https://github.com/ethereum/solidity/blob/4100a59ccaf6b921c5c8edbf66537d22d6e3e974/libsolidity/codegen/CompilerUtils.cpp#L646

Yeah, that's the solidity compiler abusing the fact that calldatacopy returns infinite zeros after the calldata to zero memory.

Stebalien added a commit to filecoin-project/builtin-actors that referenced this issue Nov 16, 2022
- CALL: truncates the return value, but doesn't zero fill.
- CALLDATA, EXTCODECOPY, CODECOPY: treat input as if it were followed by
  infinite zeros.
- RETURNDATACOPY: explicitly forbids out-of-bounds reads.

We might be able to change the behavior of EXTCODECOPY and CODECOPY to
match RETURNDATACOPY, but we _can't_ change CALLDATA as solidity abuses
this feature for zeroing memory. So we're just going to match what the
EVM does because it's safer (and because we need to implement that
behavior _anyways_ for CALLDATA).

fixes filecoin-project/ref-fvm#1021
fixes filecoin-project/ref-fvm#1024
Stebalien added a commit to filecoin-project/builtin-actors that referenced this issue Nov 16, 2022
- CALL: truncates the return value, but doesn't zero fill.
- CALLDATA, EXTCODECOPY, CODECOPY: treat input as if it were followed by
  infinite zeros.
- RETURNDATACOPY: explicitly forbids out-of-bounds reads.

We might be able to change the behavior of EXTCODECOPY and CODECOPY to
match RETURNDATACOPY, but we _can't_ change CALLDATA as solidity abuses
this feature for zeroing memory. So we're just going to match what the
EVM does because it's safer (and because we need to implement that
behavior _anyways_ for CALLDATA).

fixes filecoin-project/ref-fvm#1021
fixes filecoin-project/ref-fvm#1024
Stebalien added a commit to filecoin-project/builtin-actors that referenced this issue Nov 16, 2022
- CALL: truncates the return value, but doesn't zero fill.
- CALLDATA, EXTCODECOPY, CODECOPY: treat input as if it were followed by
  infinite zeros.
- RETURNDATACOPY: explicitly forbids out-of-bounds reads.

We might be able to change the behavior of EXTCODECOPY and CODECOPY to
match RETURNDATACOPY, but we _can't_ change CALLDATA as solidity abuses
this feature for zeroing memory. So we're just going to match what the
EVM does because it's safer (and because we need to implement that
behavior _anyways_ for CALLDATA).

fixes filecoin-project/ref-fvm#1021
fixes filecoin-project/ref-fvm#1024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Kind: Bug Something isn't working P1 P1: Must be resolved Topic: EVM runtime
Projects
None yet
Development

No branches or pull requests

2 participants