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.

Sign up for GitHub

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

core/evm: avoid copying memory for input in calls #20177

Merged
merged 3 commits into from
Nov 4, 2019
Merged

core/evm: avoid copying memory for input in calls #20177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d39aed
core/evm, contracts: avoid copying memory for input in calls + make e…
holiman Oct 17, 2019
c9ff3e1
core/vm: optimize mstore a bit
holiman Oct 23, 2019
33b3c05
core/vm: change Get -> GetCopy in vm memory access
holiman Oct 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion core/vm/contracts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,13 @@ func (c *ecrecover) Run(input []byte) ([]byte, error) {
if !allZero(input[32:63]) || !crypto.ValidateSignatureValues(v, r, s, false) {
return nil, nil
}
// We must make sure not to modify the 'input', so placing the 'v' along with
// the signature needs to be done on a new allocation
sig := make([]byte, 65)
copy(sig, input[64:128])
sig[64] = v
// v needs to be at the end for libsecp256k1
pubKey, err := crypto.Ecrecover(input[:32], append(input[64:128], v))
pubKey, err := crypto.Ecrecover(input[:32], sig)
// make sure the public key is a valid one
if err != nil {
return nil, nil
Expand Down
63 changes: 63 additions & 0 deletions core/vm/contracts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package vm

import (
"bytes"
"fmt"
"math/big"
"reflect"
Expand Down Expand Up @@ -409,6 +410,11 @@ func testPrecompiled(addr string, test precompiledTest, t *testing.T) {
} else if common.Bytes2Hex(res) != test.expected {
t.Errorf("Expected %v, got %v", test.expected, common.Bytes2Hex(res))
}
// Verify that the precompile did not touch the input buffer
exp := common.Hex2Bytes(test.input)
if !bytes.Equal(in, exp) {
t.Errorf("Precompiled %v modified input data", addr)
}
})
}

Expand All @@ -423,6 +429,11 @@ func testPrecompiledFailure(addr string, test precompiledFailureTest, t *testing
if !reflect.DeepEqual(err, test.expectedError) {
t.Errorf("Expected error [%v], got [%v]", test.expectedError, err)
}
// Verify that the precompile did not touch the input buffer
exp := common.Hex2Bytes(test.input)
if !bytes.Equal(in, exp) {
t.Errorf("Precompiled %v modified input data", addr)
}
})
}

Expand Down Expand Up @@ -574,3 +585,55 @@ func TestPrecompileBlake2FMalformedInput(t *testing.T) {
testPrecompiledFailure("09", test, t)
}
}

// EcRecover test vectors
var ecRecoverTests = []precompiledTest{
{
input: "a8b53bdf3306a35a7103ab5504a0c9b492295564b6202b1942a84ef300107281" +
"000000000000000000000000000000000000000000000000000000000000001b" +
"3078356531653033663533636531386237373263636230303933666637316633" +
"6635336635633735623734646362333161383561613862383839326234653862" +
"1122334455667788991011121314151617181920212223242526272829303132",
expected: "",
name: "CallEcrecoverUnrecoverableKey",
},
{
input: "18c547e4f7b0f325ad1e56f57e26c745b09a3e503d86e00e5255ff7f715d3d1c" +
"000000000000000000000000000000000000000000000000000000000000001c" +
"73b1693892219d736caba55bdb67216e485557ea6b6af75f37096c9aa6a5a75f" +
"eeb940b1d03b21e36b0e47e79769f095fe2ab855bd91e3a38756b7d75a9c4549",
expected: "000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b",
name: "ValidKey",
},
{
input: "18c547e4f7b0f325ad1e56f57e26c745b09a3e503d86e00e5255ff7f715d3d1c" +
"100000000000000000000000000000000000000000000000000000000000001c" +
"73b1693892219d736caba55bdb67216e485557ea6b6af75f37096c9aa6a5a75f" +
"eeb940b1d03b21e36b0e47e79769f095fe2ab855bd91e3a38756b7d75a9c4549",
expected: "",
name: "InvalidHighV-bits-1",
},
{
input: "18c547e4f7b0f325ad1e56f57e26c745b09a3e503d86e00e5255ff7f715d3d1c" +
"000000000000000000000000000000000000001000000000000000000000001c" +
"73b1693892219d736caba55bdb67216e485557ea6b6af75f37096c9aa6a5a75f" +
"eeb940b1d03b21e36b0e47e79769f095fe2ab855bd91e3a38756b7d75a9c4549",
expected: "",
name: "InvalidHighV-bits-2",
},
{
input: "18c547e4f7b0f325ad1e56f57e26c745b09a3e503d86e00e5255ff7f715d3d1c" +
"000000000000000000000000000000000000001000000000000000000000011c" +
"73b1693892219d736caba55bdb67216e485557ea6b6af75f37096c9aa6a5a75f" +
"eeb940b1d03b21e36b0e47e79769f095fe2ab855bd91e3a38756b7d75a9c4549",
expected: "",
name: "InvalidHighV-bits-3",
},
}

func TestPrecompiledEcrecover(t *testing.T) {
for _, test := range ecRecoverTests {
testPrecompiled("01", test, t)
}

}
24 changes: 11 additions & 13 deletions core/vm/instructions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ func opSAR(pc *uint64, interpreter *EVMInterpreter, contract *Contract, memory *

func opSha3(pc *uint64, interpreter *EVMInterpreter, contract *Contract, memory *Memory, stack *Stack) ([]byte, error) {
offset, size := stack.pop(), stack.pop()
data := memory.Get(offset.Int64(), size.Int64())
data := memory.GetPtr(offset.Int64(), size.Int64())

if interpreter.hasher == nil {
interpreter.hasher = sha3.NewLegacyKeccak256().(keccakState)
Expand Down Expand Up @@ -602,11 +602,9 @@ func opPop(pc *uint64, interpreter *EVMInterpreter, contract *Contract, memory *
}

func opMload(pc *uint64, interpreter *EVMInterpreter, contract *Contract, memory *Memory, stack *Stack) ([]byte, error) {
offset := stack.pop()
val := interpreter.intPool.get().SetBytes(memory.Get(offset.Int64(), 32))
stack.push(val)

interpreter.intPool.put(offset)
v := stack.peek()
offset := v.Int64()
v.SetBytes(memory.GetPtr(offset, 32))
return nil, nil
}

Expand Down Expand Up @@ -691,7 +689,7 @@ func opCreate(pc *uint64, interpreter *EVMInterpreter, contract *Contract, memor
var (
value = stack.pop()
offset, size = stack.pop(), stack.pop()
input = memory.Get(offset.Int64(), size.Int64())
input = memory.GetCopy(offset.Int64(), size.Int64())
gas = contract.Gas
)
if interpreter.evm.chainRules.IsEIP150 {
Expand Down Expand Up @@ -725,7 +723,7 @@ func opCreate2(pc *uint64, interpreter *EVMInterpreter, contract *Contract, memo
endowment = stack.pop()
offset, size = stack.pop(), stack.pop()
salt = stack.pop()
input = memory.Get(offset.Int64(), size.Int64())
input = memory.GetCopy(offset.Int64(), size.Int64())
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need copies in create? I know I asked this already, sorry, don't remember :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure we need it, but Create/Create2 holds on to that code, and stores it in a struct for later processing. I didn't consider it safe to change that now

gas = contract.Gas
)

Expand Down Expand Up @@ -757,7 +755,7 @@ func opCall(pc *uint64, interpreter *EVMInterpreter, contract *Contract, memory
toAddr := common.BigToAddress(addr)
value = math.U256(value)
// Get the arguments from the memory.
args := memory.Get(inOffset.Int64(), inSize.Int64())
args := memory.GetPtr(inOffset.Int64(), inSize.Int64())

if value.Sign() != 0 {
gas += params.CallStipend
Expand Down Expand Up @@ -786,7 +784,7 @@ func opCallCode(pc *uint64, interpreter *EVMInterpreter, contract *Contract, mem
toAddr := common.BigToAddress(addr)
value = math.U256(value)
// Get arguments from the memory.
args := memory.Get(inOffset.Int64(), inSize.Int64())
args := memory.GetPtr(inOffset.Int64(), inSize.Int64())

if value.Sign() != 0 {
gas += params.CallStipend
Expand Down Expand Up @@ -814,7 +812,7 @@ func opDelegateCall(pc *uint64, interpreter *EVMInterpreter, contract *Contract,
addr, inOffset, inSize, retOffset, retSize := stack.pop(), stack.pop(), stack.pop(), stack.pop(), stack.pop()
toAddr := common.BigToAddress(addr)
// Get arguments from the memory.
args := memory.Get(inOffset.Int64(), inSize.Int64())
args := memory.GetPtr(inOffset.Int64(), inSize.Int64())

ret, returnGas, err := interpreter.evm.DelegateCall(contract, toAddr, args, gas)
if err != nil {
Expand All @@ -839,7 +837,7 @@ func opStaticCall(pc *uint64, interpreter *EVMInterpreter, contract *Contract, m
addr, inOffset, inSize, retOffset, retSize := stack.pop(), stack.pop(), stack.pop(), stack.pop(), stack.pop()
toAddr := common.BigToAddress(addr)
// Get arguments from the memory.
args := memory.Get(inOffset.Int64(), inSize.Int64())
args := memory.GetPtr(inOffset.Int64(), inSize.Int64())

ret, returnGas, err := interpreter.evm.StaticCall(contract, toAddr, args, gas)
if err != nil {
Expand Down Expand Up @@ -895,7 +893,7 @@ func makeLog(size int) executionFunc {
topics[i] = common.BigToHash(stack.pop())
}

d := memory.Get(mStart.Int64(), mSize.Int64())
d := memory.GetCopy(mStart.Int64(), mSize.Int64())
interpreter.evm.StateDB.AddLog(&types.Log{
Address: contract.Address(),
Topics: topics,
Expand Down
4 changes: 2 additions & 2 deletions core/vm/instructions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,12 +509,12 @@ func TestOpMstore(t *testing.T) {
v := "abcdef00000000000000abba000000000deaf000000c0de00100000000133700"
stack.pushN(new(big.Int).SetBytes(common.Hex2Bytes(v)), big.NewInt(0))
opMstore(&pc, evmInterpreter, nil, mem, stack)
if got := common.Bytes2Hex(mem.Get(0, 32)); got != v {
if got := common.Bytes2Hex(mem.GetCopy(0, 32)); got != v {
t.Fatalf("Mstore fail, got %v, expected %v", got, v)
}
stack.pushN(big.NewInt(0x1), big.NewInt(0))
opMstore(&pc, evmInterpreter, nil, mem, stack)
if common.Bytes2Hex(mem.Get(0, 32)) != "0000000000000000000000000000000000000000000000000000000000000001" {
if common.Bytes2Hex(mem.GetCopy(0, 32)) != "0000000000000000000000000000000000000000000000000000000000000001" {
t.Fatalf("Mstore failed to overwrite previous value")
}
poolOfIntPools.put(evmInterpreter.intPool)
Expand Down
2 changes: 1 addition & 1 deletion core/vm/memory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func (m *Memory) Resize(size uint64) {
}

// Get returns offset + size as a new slice
func (m *Memory) Get(offset, size int64) (cpy []byte) {
func (m *Memory) GetCopy(offset, size int64) (cpy []byte) {
if size == 0 {
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion eth/tracers/tracer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func (mw *memoryWrapper) slice(begin, end int64) []byte {
log.Warn("Tracer accessed out of bound memory", "available", mw.memory.Len(), "offset", begin, "size", end-begin)
return nil
}
return mw.memory.Get(begin, end-begin)
return mw.memory.GetCopy(begin, end-begin)
}

// getUint returns the 32 bytes at the specified address interpreted as a uint.
Expand Down