Skip to content
This repository has been archived by the owner on May 13, 2022. It is now read-only.

Implement dynamic memory and fix out of bounds issues #607

Merged
merged 1 commit into from
May 15, 2017

Conversation

silasdavis
Copy link
Contributor

@silasdavis silasdavis commented May 9, 2017

This PR introduces a dynamic memory implementation that satisfies the assumption (by Solidity) that memory will be dynamically allocated at the memory boundary when an MSTORE writes to the memory boundary provided by MSIZE.

The Solidity compiler assumes that memory will be dynamically allocated at the current memory upper bounds (as reported by MSIZE) if an MSTORE operation tries to write to it. I believe the reason for this behaviour is that it is an easy way to guarantee that a portion a of memory will be unused (since it has not yet been allocated). Obviously this only works if the EVM environment will dynamically allocate that memory for you. This PR implements this in Burrow.

For reference, the Solidity assumption around object creation is encoded in the Solidity compiler here: https://github.com/ethereum/solidity/blob/develop/libsolidity/codegen/ExpressionCompiler.cpp#L842-L851

And you can see the dynamic memory allocation in go-ethereum here: https://github.com/ethereum/go-ethereum/blob/master/core/vm/interpreter.go#L152-L173 . Where the memory size is just the mstore offset + one word calculated here: https://github.com/ethereum/go-ethereum/blob/master/core/vm/memory_table.go#L49-L51.

I have taken a simpler approach of growing the memory on a Write operation up to a configurable bounds. I make use of the underlying slice implementation in Go to efficiently grow arrays. This PR includes:

  • Simple Memory interface to encapsulate memory operations
  • Adds a memoryProvider argument to NewVM
  • Switches out memory slice for new Memory object
  • Fixes issues arising because the subslice function simply wasn't fit for purpose
  • Adds a variety of tests, including on in the VM to test for the specific 'allocation on boundary' behaviour Solidity expects.

I have verified that this fixes the following issues:

fixes #474
fixes #258
fixes #300 (could not use exactly the same code)

@@ -324,14 +324,14 @@ func (function *SNativeFunctionDescription) NArgs() int {
return len(function.Args)
}

func arg(name string, abiTypeName abi.TypeName) abi.Arg {
func abiArg(name string, abiTypeName abi.TypeName) abi.Arg {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was renamed to be consistent with ret below

return abi.Arg{
Name: name,
TypeName: abiTypeName,
}
}

func ret(name string, abiTypeName abi.TypeName) abi.Return {
func abiReturn(name string, abiTypeName abi.TypeName) abi.Return {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was renamed so as to not clash with the local variable ret in the vm package

Copy link
Contributor

@benjaminbollen benjaminbollen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; important conservative improvement to adjust for modern compiler behaviour (which assumes dynamic, unlimited memory allocation) and improving the robustness of the memory implementation. Some considerations:

  1. we will now always copy memory on reading (before the copy behaviour was less obvious); requiring explicit write, suffering minor (possible) performance cost in exchange for code clarity and better encapsulation; possible optimisations can be considered on top of the interface at later stage;
  2. memErr is "ignored" because I asked for it to be ignored at this point; rather we should evaluate more structure in VM errors at later date;
  3. the dynamic memory allocation brings back to the foreground that we need refine our use of the gaslimit during execution, as this adds weight to this possible attack vector.

@benjaminbollen
Copy link
Contributor

please fmt

@silasdavis
Copy link
Contributor Author

silasdavis commented May 15, 2017

I have just amended the commit to debug log the memErrs otherwise the same, did make fmt, and tweaked a couple of comments.

  1. A thought on this. For MLOAD we are hitting an unnecessary copy when we push the the value to the stack:
func (st *Stack) Push(d Word256) {
	st.useGas(GasStackOp)
	if st.ptr == cap(st.data) {
		st.setErr(ErrDataStackOverflow)
		return
	}
	st.data[st.ptr] = d
	st.ptr++
}

// ...

func LeftPadWord256(bz []byte) (word Word256) {
	copy(word[32-len(bz):], bz)
	return
}

For cases like this (where we want a copy somewhere downstream) we could provide a function on the Memory interface func ReadTo(offset, length int64, destination []byte) ([]byte, error). Then implement LeftPadWord256 as ReadTo(offset, length, word[32-length:]). The other main occasion that we Read is to get the input for calls CALLs ultimately these are pushed to stack (obligatory copy). So if we passed input []byte from call as input Memory we could postpone the copy and have ReadTo do it whilst still maintaining safety. Again... probably not an optimisation we need now...

  1. I'm logging memErr at least for now.

  2. If we were to sort out our gas pricing to be fit for our purposes then we wouldn't be doing any worse than other famous EVM implementations with this memory model. Essentially memory is priced rather cheaply (1 MSTORE cost per 16 MiB!), but at least it is bounded by a linear function in number of MSTOREs (alright also CALLDATACOPY and others, but still)

@silasdavis
Copy link
Contributor Author

@benjaminbollen
Copy link
Contributor

Further tests in monax/cli also pass locally for me.

@benjaminbollen benjaminbollen merged commit ab65a80 into hyperledger-archives:develop May 15, 2017
@silasdavis silasdavis deleted the evm-arrays branch August 4, 2017 15:54
@silasdavis silasdavis mentioned this pull request Sep 4, 2017
silasdavis pushed a commit to silasdavis/burrow that referenced this pull request Mar 9, 2019
Implement dynamic memory and fix out of bounds issues
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
2 participants