Skip to content

eth/tracers/native: panic on memory read in prestateTracer #27691

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

Merged
s1na merged 2 commits into from
Jul 13, 2023
+67 −39
Merged

eth/tracers/native: panic on memory read in prestateTracer #27691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc29adb
eth/tracers/native: WIP fix for panic in prestate tracer
ryanschneider Jul 10, 2023
0f41194
Add testcase to cover memory padding in prestateTracer
s1na Jul 11, 2023
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
97 changes: 59 additions & 38 deletions eth/tracers/internal/tracetest/calltrace_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,13 +302,13 @@ func TestInternals(t *testing.T) {
byte(vm.CALL),
},
tracer: mkTracer("callTracer", nil),
want: `{"from":"0x000000000000000000000000000000000000feed","gas":"0xc350","gasUsed":"0x54d8","to":"0x00000000000000000000000000000000deadbeef","input":"0x","calls":[{"from":"0x00000000000000000000000000000000deadbeef","gas":"0x6cbf","gasUsed":"0x0","to":"0x00000000000000000000000000000000000000ff","input":"0x","value":"0x0","type":"CALL"}],"value":"0x0","type":"CALL"}`,
want: `{"from":"0x000000000000000000000000000000000000feed","gas":"0x13880","gasUsed":"0x54d8","to":"0x00000000000000000000000000000000deadbeef","input":"0x","calls":[{"from":"0x00000000000000000000000000000000deadbeef","gas":"0xe01a","gasUsed":"0x0","to":"0x00000000000000000000000000000000000000ff","input":"0x","value":"0x0","type":"CALL"}],"value":"0x0","type":"CALL"}`,
},
{
name: "Stack depletion in LOG0",
code: []byte{byte(vm.LOG3)},
tracer: mkTracer("callTracer", json.RawMessage(`{ "withLog": true }`)),
want: `{"from":"0x000000000000000000000000000000000000feed","gas":"0xc350","gasUsed":"0xc350","to":"0x00000000000000000000000000000000deadbeef","input":"0x","error":"stack underflow (0 \u003c=\u003e 5)","value":"0x0","type":"CALL"}`,
want: `{"from":"0x000000000000000000000000000000000000feed","gas":"0x13880","gasUsed":"0x13880","to":"0x00000000000000000000000000000000deadbeef","input":"0x","error":"stack underflow (0 \u003c=\u003e 5)","value":"0x0","type":"CALL"}`,
},
{
name: "Mem expansion in LOG0",
Expand All @@ -321,11 +321,11 @@ func TestInternals(t *testing.T) {
byte(vm.LOG0),
},
tracer: mkTracer("callTracer", json.RawMessage(`{ "withLog": true }`)),
want: `{"from":"0x000000000000000000000000000000000000feed","gas":"0xc350","gasUsed":"0x5b9e","to":"0x00000000000000000000000000000000deadbeef","input":"0x","logs":[{"address":"0x00000000000000000000000000000000deadbeef","topics":[],"data":"0x000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"}],"value":"0x0","type":"CALL"}`,
want: `{"from":"0x000000000000000000000000000000000000feed","gas":"0x13880","gasUsed":"0x5b9e","to":"0x00000000000000000000000000000000deadbeef","input":"0x","logs":[{"address":"0x00000000000000000000000000000000deadbeef","topics":[],"data":"0x000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"}],"value":"0x0","type":"CALL"}`,
},
{
// Leads to OOM on the prestate tracer
name: "Prestate-tracer - mem expansion in CREATE2",
name: "Prestate-tracer - CREATE2 OOM",
code: []byte{
byte(vm.PUSH1), 0x1,
byte(vm.PUSH1), 0x0,
Expand All @@ -339,41 +339,62 @@ func TestInternals(t *testing.T) {
byte(vm.PUSH1), 0x0,
byte(vm.LOG0),
},
tracer: mkTracer("prestateTracer", json.RawMessage(`{ "withLog": true }`)),
want: `{"0x0000000000000000000000000000000000000000":{"balance":"0x0"},"0x000000000000000000000000000000000000feed":{"balance":"0x1c6bf52640350"},"0x00000000000000000000000000000000deadbeef":{"balance":"0x0","code":"0x6001600052600164ffffffffff60016000f560ff6000a0"}}`,
tracer: mkTracer("prestateTracer", nil),
want: `{"0x0000000000000000000000000000000000000000":{"balance":"0x0"},"0x000000000000000000000000000000000000feed":{"balance":"0x1c6bf52647880"},"0x00000000000000000000000000000000deadbeef":{"balance":"0x0","code":"0x6001600052600164ffffffffff60016000f560ff6000a0"}}`,
},
{
// CREATE2 which requires padding memory by prestate tracer
name: "Prestate-tracer - CREATE2 Memory padding",
code: []byte{
byte(vm.PUSH1), 0x1,
byte(vm.PUSH1), 0x0,
byte(vm.MSTORE),
byte(vm.PUSH1), 0x1,
byte(vm.PUSH1), 0xff,
byte(vm.PUSH1), 0x1,
byte(vm.PUSH1), 0x0,
byte(vm.CREATE2),
byte(vm.PUSH1), 0xff,
byte(vm.PUSH1), 0x0,
byte(vm.LOG0),
},
tracer: mkTracer("prestateTracer", nil),
want: `{"0x0000000000000000000000000000000000000000":{"balance":"0x0"},"0x000000000000000000000000000000000000feed":{"balance":"0x1c6bf52647880"},"0x00000000000000000000000000000000deadbeef":{"balance":"0x0","code":"0x6001600052600160ff60016000f560ff6000a0"},"0x91ff9a805d36f54e3e272e230f3e3f5c1b330804":{"balance":"0x0"}}`,
},
} {
_, statedb := tests.MakePreState(rawdb.NewMemoryDatabase(),
core.GenesisAlloc{
to: core.GenesisAccount{
Code: tc.code,
},
origin: core.GenesisAccount{
Balance: big.NewInt(500000000000000),
},
}, false)
evm := vm.NewEVM(context, txContext, statedb, params.MainnetChainConfig, vm.Config{Tracer: tc.tracer})
msg := &core.Message{
To: &to,
From: origin,
Value: big.NewInt(0),
GasLimit: 50000,
GasPrice: big.NewInt(0),
GasFeeCap: big.NewInt(0),
GasTipCap: big.NewInt(0),
SkipAccountChecks: false,
}
st := core.NewStateTransition(evm, msg, new(core.GasPool).AddGas(msg.GasLimit))
if _, err := st.TransitionDb(); err != nil {
t.Fatalf("test %v: failed to execute transaction: %v", tc.name, err)
}
// Retrieve the trace result and compare against the expected
res, err := tc.tracer.GetResult()
if err != nil {
t.Fatalf("test %v: failed to retrieve trace result: %v", tc.name, err)
}
if string(res) != tc.want {
t.Fatalf("test %v: trace mismatch\n have: %v\n want: %v\n", tc.name, string(res), tc.want)
}
t.Run(tc.name, func(t *testing.T) {
_, statedb := tests.MakePreState(rawdb.NewMemoryDatabase(),
core.GenesisAlloc{
to: core.GenesisAccount{
Code: tc.code,
},
origin: core.GenesisAccount{
Balance: big.NewInt(500000000000000),
},
}, false)
evm := vm.NewEVM(context, txContext, statedb, params.MainnetChainConfig, vm.Config{Tracer: tc.tracer})
msg := &core.Message{
To: &to,
From: origin,
Value: big.NewInt(0),
GasLimit: 80000,
GasPrice: big.NewInt(0),
GasFeeCap: big.NewInt(0),
GasTipCap: big.NewInt(0),
SkipAccountChecks: false,
}
st := core.NewStateTransition(evm, msg, new(core.GasPool).AddGas(msg.GasLimit))
if _, err := st.TransitionDb(); err != nil {
t.Fatalf("test %v: failed to execute transaction: %v", tc.name, err)
}
// Retrieve the trace result and compare against the expected
res, err := tc.tracer.GetResult()
if err != nil {
t.Fatalf("test %v: failed to retrieve trace result: %v", tc.name, err)
}
if string(res) != tc.want {
t.Errorf("test %v: trace mismatch\n have: %v\n want: %v\n", tc.name, string(res), tc.want)
}
})
}
}
2 changes: 2 additions & 0 deletions eth/tracers/native/call.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/ethereum/go-ethereum/common/hexutil"
"github.com/ethereum/go-ethereum/core/vm"
"github.com/ethereum/go-ethereum/eth/tracers"
"github.com/ethereum/go-ethereum/log"
)

//go:generate go run github.com/fjl/gencodec -type callFrame -field-override callFrameMarshaling -out gen_callframe_json.go
Expand Down Expand Up @@ -183,6 +184,7 @@ func (t *callTracer) CaptureState(pc uint64, op vm.OpCode, gas, cost uint64, sco
data, err := tracers.GetMemoryCopyPadded(scope.Memory, int64(mStart.Uint64()), int64(mSize.Uint64()))
if err != nil {
// mSize was unrealistically large
log.Warn("failed to copy CREATE2 input", "err", err, "tracer", "callTracer", "offset", mStart, "size", mSize)
return
}

Expand Down
7 changes: 6 additions & 1 deletion eth/tracers/native/prestate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/ethereum/go-ethereum/core/vm"
"github.com/ethereum/go-ethereum/crypto"
"github.com/ethereum/go-ethereum/eth/tracers"
"github.com/ethereum/go-ethereum/log"
)

//go:generate go run github.com/fjl/gencodec -type account -field-override accountMarshaling -out gen_account_json.go
Expand Down Expand Up @@ -165,7 +166,11 @@ func (t *prestateTracer) CaptureState(pc uint64, op vm.OpCode, gas, cost uint64,
case stackLen >= 4 && op == vm.CREATE2:
offset := stackData[stackLen-2]
size := stackData[stackLen-3]
init := scope.Memory.GetCopy(int64(offset.Uint64()), int64(size.Uint64()))
init, err := tracers.GetMemoryCopyPadded(scope.Memory, int64(offset.Uint64()), int64(size.Uint64()))
if err != nil {
log.Warn("failed to copy CREATE2 input", "err", err, "tracer", "prestateTracer", "offset", offset, "size", size)
return
Copy link
Copy Markdown
Contributor Author

@ryanschneider ryanschneider Jul 11, 2023

Choose a reason for hiding this comment

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

@s1na thinking about this more personally I'd prefer this panic(err) instead of an early return, especially since the log warnings could get spammy under load since one line could be logged per RPC request triggering the issue.

Copy link
Copy Markdown
Contributor

@holiman holiman Jul 12, 2023

Choose a reason for hiding this comment

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

A panic in an rpc call would not crash the server, just the method-handler, and the user would get some form of error message like "method handler crashed", if I recall correctly.

As I see it, we have these options for what to return to the user:

  1. A mostly correct trace + server-side printout indicating error
  2. No trace + "method handler crashed" error
  3. No trace + specific error about what happened and when ("failed to copy create2 input")

Not sure what my preference is. But previously, we settled for 1.

}
inithash := crypto.Keccak256(init)
salt := stackData[stackLen-4]
addr := crypto.CreateAddress2(caller, salt.Bytes32(), inithash)
Expand Down