Skip to content

accounts/abi: Index inputs are not present in output #15568

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

Closed
dshulyak wants to merge 3 commits into from
Closed

accounts/abi: Index inputs are not present in output #15568

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
e3bca64
accounts/abi: Indexed inputs are not present in output
Nov 26, 2017
4fc2aa3
accounts/abi: count array fields after unpacking
dshulyak Nov 30, 2017
2191706
accounts/abi: Improve tests for provided fixes
dshulyak Dec 8, 2017
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
10 changes: 7 additions & 3 deletions accounts/abi/event.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,20 @@ func (e Event) tupleUnpack(v interface{}, output []byte) error {
for i := 0; i < len(e.Inputs); i++ {
input := e.Inputs[i]
if input.Indexed {
// indexed inputs are not available in log output
j--
Copy link
Copy Markdown
Member

@VoR0220 VoR0220 Nov 29, 2017

Choose a reason for hiding this comment

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

This seems fairly elegant. Works for me.

Copy link
Copy Markdown
Member

@VoR0220 VoR0220 Nov 29, 2017

Choose a reason for hiding this comment

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

Second thought, I'm not sure this plays nicely with a static array element insofar as indexing is concerned.

Copy link
Copy Markdown
Contributor Author

@dshulyak dshulyak Nov 30, 2017

Choose a reason for hiding this comment

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

thanks for review, i added required test, but i think that i spotted another issue with array types. please see my comment below

// can't read, continue
continue
} else if input.Type.T == ArrayTy {
// need to move this up because they read sequentially
j += input.Type.Size
}
marshalledValue, err := toGoType((i+j)*32, input.Type, output)
if err != nil {
return err
}
if input.Type.T == ArrayTy {
Copy link
Copy Markdown
Contributor Author

@dshulyak dshulyak Nov 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

In current codebase array fields are counted before unpacking output.
So, if we have array with 2 fields: toGoType function will try to unpack
values from 64 till 64 + 2*32 which is not correct in my opinion.

Counting will be done after processing array, so in case array is the first
field, and it has 2 elements: we will read it from 0 till 2*32, and next element will start at 64.

Copy link
Copy Markdown
Contributor Author

@dshulyak dshulyak Nov 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I was using this contract for test:


contract Test {
  
  event Echo(uint8[2] message, address from);
  
  function emit() {
    uint8[2] memory r = [1,2]; 
    Echo(r, msg.sender);
  }

}

Error message:
2017/11/30 12:37:46 abi: cannot marshal in to go array: offset 96 would go over slice boundary (len=128)

Copy link
Copy Markdown
Member

@VoR0220 VoR0220 Dec 7, 2017

Choose a reason for hiding this comment

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

Why would that be incorrect? Sounds right at the beginning.

Copy link
Copy Markdown
Contributor Author

@dshulyak dshulyak Dec 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

how is that correct? example that i provided actually fails on current master

let's say we will try to unpack an event with ([7,8], 9) all simple integers,
it will be encoded as

0000000000000000000000000000000000000000000000000000000000000007
0000000000000000000000000000000000000000000000000000000000000008
0000000000000000000000000000000000000000000000000000000000000009

without this change we will try to read array from 64 till 128, and integer from 96 till 128, and actually will fail with this error 2017/12/07 16:04:29 abi: cannot marshal in to go array: offset 96 would go over slice boundary (len=128)

with this change first read will be done from 0 till 64 (array), and for integer from 64 till 96

Copy link
Copy Markdown
Member

@VoR0220 VoR0220 Dec 9, 2017

Choose a reason for hiding this comment

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

Ah. Okay. Yea I'm sorry for the wait, I've reviewed the code, it looks fairly good, but I would just add a comment about the j += input.Type.Size - 1...specifically why you're subtracting the 1. Just leave a comment on that and I'll approve this...for whatever that's worth.

Copy link
Copy Markdown
Contributor Author

@dshulyak dshulyak Dec 11, 2017

Choose a reason for hiding this comment

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

@VoR0220 added a comment

// combined index ('i' + 'j') need to be adjusted only by size of array, thus
// we need to decrement 'j' because 'i' was incremented
j += input.Type.Size - 1
}
reflectValue := reflect.ValueOf(marshalledValue)

switch value.Kind() {
Expand Down
93 changes: 93 additions & 0 deletions accounts/abi/event_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,16 @@
package abi

import (
"bytes"
"fmt"
"math/big"
"reflect"
"strings"
"testing"

"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/crypto"
"github.com/stretchr/testify/require"
)

func TestEventId(t *testing.T) {
Expand Down Expand Up @@ -54,3 +59,91 @@ func TestEventId(t *testing.T) {
}
}
}

type testResult struct {
Values [2]*big.Int
Value1 *big.Int
Value2 *big.Int
}

type testCase struct {
definition string
want testResult
}

func (tc testCase) encoded(intType, arrayType Type) []byte {
var b bytes.Buffer
if tc.want.Value1 != nil {
val, _ := intType.pack(reflect.ValueOf(tc.want.Value1))
b.Write(val)
}

if !reflect.DeepEqual(tc.want.Values, [2]*big.Int{nil, nil}) {
val, _ := arrayType.pack(reflect.ValueOf(tc.want.Values))
b.Write(val)
}
if tc.want.Value2 != nil {
val, _ := intType.pack(reflect.ValueOf(tc.want.Value2))
b.Write(val)
}
return b.Bytes()
}

// TestEventUnpackIndexed verifies that indexed field will be skipped by event decoder.
func TestEventUnpackIndexed(t *testing.T) {
definition := `[{"name": "test", "type": "event", "inputs": [{"indexed": true, "name":"value1", "type":"uint8"},{"indexed": false, "name":"value2", "type":"uint8"}]}]`
type testStruct struct {
Value1 uint8
Value2 uint8
}
abi, err := JSON(strings.NewReader(definition))
require.NoError(t, err)
var b bytes.Buffer
b.Write(packNum(reflect.ValueOf(uint8(8))))
var rst testStruct
require.NoError(t, abi.Unpack(&rst, "test", b.Bytes()))
require.Equal(t, uint8(0), rst.Value1)
require.Equal(t, uint8(8), rst.Value2)
}

// TestEventMultiValueWithArrayUnpack verifies that array fields will be counted after parsing array.
func TestEventMultiValueWithArrayUnpack(t *testing.T) {
definition := `[{"name": "test", "type": "event", "inputs": [{"indexed": false, "name":"value1", "type":"uint8[2]"},{"indexed": false, "name":"value2", "type":"uint8"}]}]`
type testStruct struct {
Value1 [2]uint8
Value2 uint8
}
abi, err := JSON(strings.NewReader(definition))
require.NoError(t, err)
var b bytes.Buffer
var i uint8 = 1
for ; i <= 3; i++ {
b.Write(packNum(reflect.ValueOf(i)))
}
var rst testStruct
require.NoError(t, abi.Unpack(&rst, "test", b.Bytes()))
require.Equal(t, [2]uint8{1, 2}, rst.Value1)
require.Equal(t, uint8(3), rst.Value2)
}

// TestEventIndexedWithArrayUnpack verifies that decoder will not overlow when static array is indexed input.
func TestEventIndexedWithArrayUnpack(t *testing.T) {
definition := `[{"name": "test", "type": "event", "inputs": [{"indexed": true, "name":"value1", "type":"uint8[2]"},{"indexed": false, "name":"value2", "type":"string"}]}]`
type testStruct struct {
Value1 [2]uint8
Value2 string
}
abi, err := JSON(strings.NewReader(definition))
require.NoError(t, err)
var b bytes.Buffer
stringOut := "abc"
// number of fields that will be encoded * 32
b.Write(packNum(reflect.ValueOf(32)))
b.Write(packNum(reflect.ValueOf(len(stringOut))))
b.Write(common.RightPadBytes([]byte(stringOut), 32))
fmt.Println(b.Bytes())
var rst testStruct
require.NoError(t, abi.Unpack(&rst, "test", b.Bytes()))
require.Equal(t, [2]uint8{0, 0}, rst.Value1)
require.Equal(t, stringOut, rst.Value2)
}