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 Runtime: Runtime stack will panic from malformed asm #847

Closed
mriise opened this issue Sep 7, 2022 · 14 comments
Closed

EVM Runtime: Runtime stack will panic from malformed asm #847

mriise opened this issue Sep 7, 2022 · 14 comments
Assignees
Labels
Kind: Bug Something isn't working P1 P1: Must be resolved Topic: EVM runtime

Comments

@mriise
Copy link
Contributor

mriise commented Sep 7, 2022

EVM stack will panic with negative overflow if bytecode doesnt have all the values it expects to pop off. This will happen with malformed bytecode but we shouldn't panic from it.
this can be done easily enough in asm with the return opcode by itself:

let evm_bytecode = "
{constructor bytecode omitted} 
return # runtime will panic
"; 
@raulk raulk added the Kind: Bug Something isn't working label Sep 8, 2022
@Stebalien
Copy link
Member

In this case, we should probably check stack heights from the EVM dispatch code (we record the stack height requirements). That should make error handling simpler.

@vyzo
Copy link
Contributor

vyzo commented Sep 9, 2022

This is likely to be prohibitively expensive.

Ideally we'd have guard pages around the stack and a proper segfault handler and.... oh wait, we are writing in this in rust.
We should let the thing panic if it can.

@Stebalien
Copy link
Member

We should let the thing panic if it can.

This makes DELEGATECALL a bit tricker.

  • We can handle a panic (and, e.g., exit with the correct code).
  • We cannot recover from a panic (in rust).

This is likely to be prohibitively expensive.

if we do it right, it'll actually be faster. At the moment, every stack pop checks to make sure we have enough elements on the stack. If we check up-front (and convince the compiler to inline the instructions), we can elide all the individual checks.

@vyzo
Copy link
Contributor

vyzo commented Sep 9, 2022

wait, why does it panic if it checks already?

Or is that check causing the panic?

In that case yes, we are already paying the prohibitive cost and doing the full stack check upfront will be faster if we can elide the subsequent checks.

@Stebalien
Copy link
Member

Or is that check causing the panic?

This. Rust is a memory-safe language.

@vyzo
Copy link
Contributor

vyzo commented Sep 9, 2022

something or other people call it a fast language? :)

Joking aside, there has to be a way to do a single check upfront somehow.

@Stebalien
Copy link
Member

Yes. If you do a check up-front, rust will elide the other checks.

However, if you do this in two separate functions and rust doesn't inline the called function, it can't elide the checks.

One workaround is to pass in a fixed-sized "stack". Given that EVM operations specify how many elements they read/push, we can define Stack<N> object that's internally backed by a &[U256; N] array. If we pass that, the rust compiler should be able to get rid of the checks.

@mriise
Copy link
Contributor Author

mriise commented Sep 12, 2022

#[inline] forces inlining even in nested functions no?

@maciejwitowski maciejwitowski added this to the M2.1 milestone Oct 7, 2022
@Stebalien
Copy link
Member

@vyzo you've fixed this with the stack checks, right?

@Stebalien
Copy link
Member

(has that been merged and/or do you have a PR number?)

@vyzo
Copy link
Contributor

vyzo commented Oct 10, 2022

hasnt been merged yet and i want to do the refactoring we discussed first, but yes, it will be fixed.

@vyzo
Copy link
Contributor

vyzo commented Oct 10, 2022

ba 646

@Stebalien Stebalien assigned vyzo and unassigned mriise Oct 10, 2022
@Stebalien Stebalien modified the milestones: M2.1, M2.1 (rr10) Carbonado Nov 9, 2022
@Stebalien Stebalien added the P1 P1: Must be resolved label Nov 9, 2022
@Stebalien
Copy link
Member

@vyzo confirmed fixed?

@vyzo
Copy link
Contributor

vyzo commented Nov 29, 2022

yes with 1-\epsilon, will add a regression test tonight.

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

5 participants