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.

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

go/storage/mkvs: Use cbor.UnmarshalTrusted for internal metadata #2800

Merged
merged 2 commits into from
Apr 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .changelog/2800.bugfix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
go/storage/mkvs: Use cbor.UnmarshalTrusted for internal metadata
5 changes: 5 additions & 0 deletions .changelog/2800.internal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
go/common/cbor: Add UnmarshalTrusted for trusted inputs

The new method relaxes some decoding restrictions for cases where the inputs
are trusted (e.g., because they are known to be generated by the local node
itself).
27 changes: 25 additions & 2 deletions go/common/cbor/cbor.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,23 @@ var (
TagsMd: cbor.TagsForbidden,
}

// decOptions are decoding options for UNTRUSTED inputs (used by default).
decOptions = cbor.DecOptions{
DupMapKey: cbor.DupMapKeyEnforcedAPF,
IndefLength: cbor.IndefLengthForbidden,
TagsMd: cbor.TagsForbidden,
}

encMode cbor.EncMode
decMode cbor.DecMode
// decOptionsTrusted are decoding options for TRUSTED inputs. They are only used when explicitly
// requested by using the UnmarshalTrusted method.
decOptionsTrusted = cbor.DecOptions{
MaxArrayElements: 134217728, // Maximum allowed.
MaxMapPairs: 134217728, // Maximum allowed.
Copy link
Contributor

Choose a reason for hiding this comment

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

what are the defaults for these on the untrusted decOptions?

Copy link
Member Author

Choose a reason for hiding this comment

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

The default is 131072. Maybe we should consider making this explicit and/or even changing it (e.g., lowering it) for untrusted settings?

Copy link
Contributor

Choose a reason for hiding this comment

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

eh, maybe a comment if anything

Choose a reason for hiding this comment

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

@kostko hello. I have an issue with calling StateToGenesis beacon api: rpc error: code = Internal desc = grpc: failed to unmarshal the received message cbor: exceeded max number of key-value pairs 131072 for CBOR map at block 7188323. How I can explicitly call UnmarshalTrusted method at this point?

}

encMode cbor.EncMode
decMode cbor.DecMode
decModeTrusted cbor.DecMode
)

func init() {
Expand All @@ -45,6 +54,9 @@ func init() {
if decMode, err = decOptions.DecMode(); err != nil {
panic(err)
}
if decModeTrusted, err = decOptionsTrusted.DecMode(); err != nil {
panic(err)
}
}

// FixSliceForSerde will convert `nil` to `[]byte` to work around serde
Expand Down Expand Up @@ -74,6 +86,17 @@ func Unmarshal(data []byte, dst interface{}) error {
return decMode.Unmarshal(data, dst)
}

// UnmarshalTrusted deserializes a CBOR byte vector into a given type.
//
// This method MUST ONLY BE USED FOR TRUSTED INPUTS as it relaxes some decoding restrictions.
func UnmarshalTrusted(data []byte, dst interface{}) error {
if data == nil {
return nil
}

return decModeTrusted.Unmarshal(data, dst)
}

// MustUnmarshal deserializes a CBOR byte vector into a given type.
// Panics if unmarshal fails.
func MustUnmarshal(data []byte, dst interface{}) {
Expand Down
6 changes: 3 additions & 3 deletions go/storage/mkvs/db/badger/badger.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ func (d *badgerNodeDB) load() error {
// Metadata already exists, just load it and verify that it is
// compatible with what we have here.
err = item.Value(func(data []byte) error {
return cbor.Unmarshal(data, &d.meta.value)
return cbor.UnmarshalTrusted(data, &d.meta.value)
})
if err != nil {
return err
Expand Down Expand Up @@ -303,7 +303,7 @@ func (d *badgerNodeDB) GetWriteLog(ctx context.Context, startRoot node.Root, end

var log api.HashedDBWriteLog
err = item.Value(func(data []byte) error {
return cbor.Unmarshal(data, &log)
return cbor.UnmarshalTrusted(data, &log)
})
if err != nil {
return node.Root{}, nil, err
Expand Down Expand Up @@ -464,7 +464,7 @@ func (d *badgerNodeDB) Finalize(ctx context.Context, version uint64, roots []has

var updatedNodes []updatedNode
err = item.Value(func(data []byte) error {
return cbor.Unmarshal(data, &updatedNodes)
return cbor.UnmarshalTrusted(data, &updatedNodes)
})
if err != nil {
panic(fmt.Errorf("mkvs/badger: corrupted root updated nodes index: %w", err))
Expand Down
18 changes: 18 additions & 0 deletions go/storage/mkvs/tree_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1889,6 +1889,23 @@ func testSpecialCase5(t *testing.T, ndb db.NodeDB, factory NodeDBFactory) {
testSpecialCaseFromJSON(t, ndb, "case-5.json")
}

func testLargeUpdates(t *testing.T, ndb db.NodeDB, factory NodeDBFactory) {
ctx := context.Background()
tree := New(nil, ndb)

// The number of elements is such that it would overflow the maximum number of allowed array
// elements in the default (untrusted) CBOR decoder.
for i := 0; i < 132_000; i++ {
err := tree.Insert(ctx, []byte(fmt.Sprintf("%d", i)), []byte(fmt.Sprintf("%d", i)))
require.NoError(t, err, "Insert")
}

_, rootHash, err := tree.Commit(ctx, testNs, 0)
require.NoError(t, err, "Commit")
err = ndb.Finalize(ctx, 0, []hash.Hash{rootHash})
require.NoError(t, err, "Finalize")
}

func testBackend(
t *testing.T,
initBackend func(t *testing.T) (NodeDBFactory, func()),
Expand Down Expand Up @@ -1929,6 +1946,7 @@ func testBackend(
{"SpecialCase3", testSpecialCase3},
{"SpecialCase4", testSpecialCase4},
{"SpecialCase5", testSpecialCase5},
{"LargeUpdates", testLargeUpdates},
{"Errors", testErrors},
}

Expand Down