accounts/abi: add another unpack interface #15770

holiman · 2017-12-28T10:26:57Z

This PR adds the ability to unpack ABI-encoded data without supplying a data structure to place the data into.

It allows the caller to do method.Inputs.UnpackValues(data) , and receive a []interface{} containing the data in whatever type is used by the abi. This enables the usecase that the actual abi-spec is not known in advance, such as when the signer wants to decode arbitrary data.

The old API-methods are left intact; when a programmer wants to interact with a specific (known) ABI, it's probably (?) useful to be able to handles custom structs instead of casting suff on-the-fly.

This PR uses the same set of testcases that was already in use for Unpack, and use them to test the new method(s). The PR does (should) not interfere with existing code/features, and should be fairly safe to merge.

holiman · 2017-12-30T15:09:31Z

I've mow modified this PR, so that the more complex unpack-into-struct uses the unpackValues under the hood, and does not duplicate the decoding part. This makes a cleaner separation between the two layers, and decreases the overall complexity.

holiman · 2017-12-30T15:11:23Z

(which also means that I could remove the unpack_testV2 file)

gballet

Just a minor comment for using an iterator pattern instead of building an array on the fly, otherwise looks good to me.

gballet · 2018-01-07T23:17:05Z

accounts/abi/argument.go

@@ -67,59  67,55 @@ func (arguments Arguments) LengthNonIndexed() int {
 	return out
 }

+func (arguments Arguments) NonIndexed() Arguments {


wouldn't it make sense to rewrite it as an iterator, since it's mostly called with range and the only time that you don't, you only need the first value? That would avoid parsing the entire array if you don't need to.

@holiman Please add a method doc.

@gballet Usually a method will have only a handful of arguments. Creating and maintaining an iterator would be a much larger overhead than cycling through 5 elements of a slice.

karalabe

In general I like this PR, since it separates the ABI decoding a bit and adds an intermediate layer in between that we can unit test.

A question however is whether we can attack this intermediate layer? When marshaling into explicit Go types, we immediately discard binary data that doesn't match what we expect. Here however we accept any meaningful data and only validate it later.

I.e. I think it would be very very wise to run this code through gofuzz for starters, and also to explore whether it could go OOM with an appropriately constructed input.

karalabe · 2018-01-09T10:05:16Z

accounts/abi/abi.go

@@ -137,11  137,11 @@ func (abi *ABI) UnmarshalJSON(data []byte) error {

 // MethodById looks up a method by the 4-byte id
 // returns nil if none found
-func (abi *ABI) MethodById(sigdata []byte) *Method {
+func (abi *ABI) MethodById(sigdata []byte) (*Method, error) {


Maybe we could use [4]byte here to compile time ensure the size is ok?

We could, but then the caller needs to convert his slice of bytes into an array instead, with some for-loop. Is that really nicer?

karalabe · 2018-01-09T10:07:18Z

accounts/abi/abi.go

 		}
 	}
-	return nil
+	return nil, fmt.Errorf("ABI spec does not contain method signature in data: 0x%x", sigdata[:4])


Generally Go uses shorter error messages since they tend to get appended to one another. If possible, try to make it a bit more concise. E.g. fmt.Errorf("no method with id %#x", sigdata[:4])

Note 0x%x is the same at %#x for formatting.

karalabe · 2018-01-09T10:08:20Z

accounts/abi/abi_test.go

-		b := fmt.Sprintf("%v", abi.MethodById(m.Id()))
+		m2, err := abi.MethodById(m.Id())
+		if err != nil {
+			t.Fatal(err)


And this is where you append errors :)

t.Fatalf("Failed to look up ABI method: %v", err)

karalabe · 2018-01-09T10:10:33Z

accounts/abi/argument.go

@@ -67,59  67,55 @@ func (arguments Arguments) LengthNonIndexed() int {
 	return out
 }

+func (arguments Arguments) NonIndexed() Arguments {


@holiman Please add a method doc.

@gballet Usually a method will have only a handful of arguments. Creating and maintaining an iterator would be a much larger overhead than cycling through 5 elements of a slice.

karalabe · 2018-01-09T10:12:21Z

accounts/abi/argument.go

+	if err != nil {
+		return err
+	}
+


Nitpick, but why so many empty lines? They are useful for explicitly separating logical blocks, but here the } line in itself is already a nice visual separator so adding 1 empty line seems a tad wasteful.

karalabe · 2018-01-09T10:14:25Z

accounts/abi/argument.go

+	retval := make([]interface{}, 0, arguments.LengthNonIndexed())
+
+	virtualArgs := 0
+


Why an empty line?

karalabe · 2018-01-09T10:14:45Z

accounts/abi/argument.go

+		marshalledValue, err := toGoType((index virtualArgs)*32, arg.Type, data)
+
+		if arg.Type.T == ArrayTy {
+			//If we have a static array, like [3]uint256, these are coded as


Space between // and ``If`

karalabe · 2018-01-09T10:16:15Z

accounts/abi/argument.go

+	return retval, nil
+}
+
+// UnpackValues performs the operation Go format -> Hexdata


UnpackValues -> PackValues

karalabe · 2018-01-09T10:17:06Z

accounts/abi/argument.go

+
+// UnpackValues performs the operation Go format -> Hexdata
+// It is the semantic opposite of UnpackValues
+func (arguments Arguments) PackValues(args []interface{}) ([]byte, error) {


Why do we need this method, if we can just call Pack(args...) instead?

We don't need it, I just added to have coder<->decoder pipe with identical input<->output format. LMK if I should remove it.

karalabe · 2018-01-09T10:18:07Z

accounts/abi/unpackv2_test.go

@@ -0,0  1,335 @@
+// Copyright 2015 The go-ethereum Authors


What's with the v2 stuff?

I'm going to remove this file. Since this is a copy of the existing one, but uses the 'raw' unpacker, this is now not needed since they complex parsing happens on top of the 'raw' parsing.

holiman · 2018-01-13T14:15:01Z

A question however is whether we can attack this intermediate layer? When marshaling into explicit Go types, we immediately discard binary data that doesn't match what we expect. Here however we accept any meaningful data and only validate it later.

Good point. I'll experiment a bit with that.

holiman · 2018-01-13T14:36:48Z

It does handle very large arrays, and detects out-of-bound size: Error: abi: cannot marshal in to go type: length insufficient 128 require 9223372036837998658. However, if the size is even larger, it becomes negative (crash), or goes above 64 byte, then it just ignores that.

holiman · 2018-01-13T15:06:39Z

The original code kind of intentionally ignored all offset and length data above 64 bits. It was vulnerable to negative input, though. I've now made the following changes:

Check the full 32 bytes of offset and size. Error if bad.
Check that length can never be negative (which is no longer an issue, though, since it's converted from a bigint, but it's on a separate layer so it's good for robustness if we decide to change back).
Testcases for 64bit-negative, 64-bit very large, above 64-bit larrge both offset and size.

holiman · 2018-01-13T15:13:04Z

Here however we accept any meaningful data and only validate it later.

It's not quite the case, thought. The toGoType is pretty defensive, and checks that it does not reference outside the size of the input data, nor allocate slices larger than the actual input data size.

holiman · 2018-01-13T15:25:56Z

Rebased on master

holiman · 2018-01-13T17:58:36Z

Check the full 32 bytes of offset and size. Error if bad.

Note: I don't really care if we do that, or if we keep it as it was previously. It almost looks like the previous code was written like that for a purpose (?)

holiman · 2018-01-23T08:21:14Z

Ping @karalabe

holiman requested a review from gballet December 28, 2017 10:27

holiman force-pushed the abi_nostruct branch from 1bb8865 to 1b235e1 Compare December 30, 2017 15:21

gballet approved these changes Jan 7, 2018

View reviewed changes

karalabe requested changes Jan 9, 2018

View reviewed changes

holiman force-pushed the abi_nostruct branch from 0337271 to c34d064 Compare January 13, 2018 15:25

holiman force-pushed the abi_nostruct branch from c34d064 to fc25d91 Compare January 15, 2018 07:44

holiman added 5 commits February 21, 2018 12:27

accounts/abi: add another unpack interface

1ede683

accounts/abi: Deduplicate code in unpacker

f0f863d

accounts/abi: address review concerns

08c5d4d

accounts/abi: harden unpacking against malicious input

bd6ed23

abi: fix missing method on go 1.7/1.8

61f2279

holiman force-pushed the abi_nostruct branch from 0a7142f to 61f2279 Compare February 21, 2018 11:28

holiman mentioned this pull request Feb 21, 2018

accounts/abi: Abi binding support for nested arrays, fixes #15648, including nested array unpack fix #15676

Merged

holiman merged commit f54506c into ethereum:master Feb 21, 2018

karalabe added this to the 1.8.2 milestone Feb 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

accounts/abi: add another unpack interface #15770

accounts/abi: add another unpack interface #15770

holiman commented Dec 28, 2017

holiman commented Dec 30, 2017

holiman commented Dec 30, 2017

gballet left a comment

gballet Jan 7, 2018

karalabe Jan 9, 2018

karalabe left a comment

karalabe Jan 9, 2018

holiman Jan 13, 2018

karalabe Jan 9, 2018

karalabe Jan 9, 2018

karalabe Jan 9, 2018

karalabe Jan 9, 2018

karalabe Jan 9, 2018

karalabe Jan 9, 2018

karalabe Jan 9, 2018

karalabe Jan 9, 2018

holiman Jan 13, 2018

karalabe Jan 9, 2018

holiman Jan 13, 2018

holiman commented Jan 13, 2018

holiman commented Jan 13, 2018

holiman commented Jan 13, 2018

holiman commented Jan 13, 2018

holiman commented Jan 13, 2018

holiman commented Jan 13, 2018

holiman commented Jan 23, 2018

		retval := make([]interface{}, 0, arguments.LengthNonIndexed())

		virtualArgs := 0

accounts/abi: add another unpack interface #15770

accounts/abi: add another unpack interface #15770

Conversation

holiman commented Dec 28, 2017

holiman commented Dec 30, 2017

holiman commented Dec 30, 2017

gballet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karalabe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman commented Jan 13, 2018

holiman commented Jan 13, 2018

holiman commented Jan 13, 2018

holiman commented Jan 13, 2018

holiman commented Jan 13, 2018

holiman commented Jan 13, 2018

holiman commented Jan 23, 2018