From 2d6b0a857e1e4a9a92f38c732ae6944deb142f15 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Mon, 26 Jul 2021 22:08:04 +0200 Subject: [PATCH 1/3] refactor: Improve txRaw decoder code (#9769) <!-- The default pull request template is for types feat, fix, or refactor. For other templates, add one of the following parameters to the url: - template=docs.md - template=other.md --> ## Description Apply post-merge reviews to a previous PR ref: https://github.com/cosmos/cosmos-sdk/pull/9754#pullrequestreview-713819730 <!-- Add a description of the changes that this PR introduces and the files that are the most critical to review. --> --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) (cherry picked from commit 3771ebdf2463aea04c2ebe85040e4891caf57c68) # Conflicts: # x/auth/tx/decoder.go --- x/auth/tx/decoder.go | 91 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/x/auth/tx/decoder.go b/x/auth/tx/decoder.go index a32a118604f9..87bef54e2702 100644 --- a/x/auth/tx/decoder.go +++ b/x/auth/tx/decoder.go @@ -11,6 +11,15 @@ import ( // DefaultTxDecoder returns a default protobuf TxDecoder using the provided Marshaler. func DefaultTxDecoder(cdc codec.ProtoCodecMarshaler) sdk.TxDecoder { return func(txBytes []byte) (sdk.Tx, error) { +<<<<<<< HEAD +======= + // Make sure txBytes follow ADR-027. + err := rejectNonADR027TxRaw(txBytes) + if err != nil { + return nil, sdkerrors.Wrap(sdkerrors.ErrTxDecode, err.Error()) + } + +>>>>>>> 3771ebdf2 (refactor: Improve txRaw decoder code (#9769)) var raw tx.TxRaw // reject all unknown proto fields in the root TxRaw @@ -79,3 +88,85 @@ func DefaultJSONTxDecoder(cdc codec.ProtoCodecMarshaler) sdk.TxDecoder { }, nil } } +<<<<<<< HEAD +======= + +// rejectNonADR027TxRaw rejects txBytes that do not follow ADR-027. This is NOT +// a generic ADR-027 checker, it only applies decoding TxRaw. Specifically, it +// only checks that: +// - field numbers are in ascending order (1, 2, and potentially multiple 3s), +// - and varints are as short as possible. +// All other ADR-027 edge cases (e.g. default values) are not applicable with +// TxRaw. +func rejectNonADR027TxRaw(txBytes []byte) error { + // Make sure all fields are ordered in ascending order with this variable. + prevTagNum := protowire.Number(0) + + for len(txBytes) > 0 { + tagNum, wireType, m := protowire.ConsumeTag(txBytes) + if m < 0 { + return fmt.Errorf("invalid length; %w", protowire.ParseError(m)) + } + // TxRaw only has bytes fields. + if wireType != protowire.BytesType { + return fmt.Errorf("expected %d wire type, got %d", protowire.BytesType, wireType) + } + // Make sure fields are ordered in ascending order. + if tagNum < prevTagNum { + return fmt.Errorf("txRaw must follow ADR-027, got tagNum %d after tagNum %d", tagNum, prevTagNum) + } + prevTagNum = tagNum + + // All 3 fields of TxRaw have wireType == 2, so their next component + // is a varint, so we can safely call ConsumeVarint here. + // Byte structure: <varint of bytes length><bytes sequence> + // Inner fields are verified in `DefaultTxDecoder` + lengthPrefix, m := protowire.ConsumeVarint(txBytes[m:]) + if m < 0 { + return fmt.Errorf("invalid length; %w", protowire.ParseError(m)) + } + // We make sure that this varint is as short as possible. + n := varintMinLength(lengthPrefix) + if n != m { + return fmt.Errorf("length prefix varint for tagNum %d is not as short as possible, read %d, only need %d", tagNum, m, n) + } + + // Skip over the bytes that store fieldNumber and wireType bytes. + _, _, m = protowire.ConsumeField(txBytes) + if m < 0 { + return fmt.Errorf("invalid length; %w", protowire.ParseError(m)) + } + txBytes = txBytes[m:] + } + + return nil +} + +// varintMinLength returns the minimum number of bytes necessary to encode an +// uint using varint encoding. +func varintMinLength(n uint64) int { + switch { + // Note: 1<<N == 2**N. + case n < 1<<(7): + return 1 + case n < 1<<(7*2): + return 2 + case n < 1<<(7*3): + return 3 + case n < 1<<(7*4): + return 4 + case n < 1<<(7*5): + return 5 + case n < 1<<(7*6): + return 6 + case n < 1<<(7*7): + return 7 + case n < 1<<(7*8): + return 8 + case n < 1<<(7*9): + return 9 + default: + return 10 + } +} +>>>>>>> 3771ebdf2 (refactor: Improve txRaw decoder code (#9769)) From eb1dfb5f0c8615d3767db64c0ff5019225bbff07 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Tue, 27 Jul 2021 16:33:52 +0200 Subject: [PATCH 2/3] Fix lint? --- server/grpc/gogoreflection/fix_registration.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/server/grpc/gogoreflection/fix_registration.go b/server/grpc/gogoreflection/fix_registration.go index ab7750574845..6575f782d3aa 100644 --- a/server/grpc/gogoreflection/fix_registration.go +++ b/server/grpc/gogoreflection/fix_registration.go @@ -9,7 +9,6 @@ import ( _ "github.com/gogo/protobuf/gogoproto" // required so it does register the gogoproto file descriptor gogoproto "github.com/gogo/protobuf/proto" - // nolint: staticcheck "github.com/golang/protobuf/proto" dpb "github.com/golang/protobuf/protoc-gen-go/descriptor" _ "github.com/regen-network/cosmos-proto" // look above @@ -86,7 +85,7 @@ func getFileDescriptor(filePath string) []byte { if len(fd) != 0 { return fd } - // nolint: staticcheck + return proto.FileDescriptor(filePath) } @@ -95,7 +94,7 @@ func getMessageType(name string) reflect.Type { if typ != nil { return typ } - // nolint: staticcheck + return proto.MessageType(name) } @@ -107,7 +106,7 @@ func getExtension(extID int32, m proto.Message) *gogoproto.ExtensionDesc { } } // check into proto registry - // nolint: staticcheck + for id, desc := range proto.RegisteredExtensions(m) { if id == extID { return &gogoproto.ExtensionDesc{ @@ -133,7 +132,7 @@ func getExtensionsNumbers(m proto.Message) []int32 { if len(out) != 0 { return out } - // nolint: staticcheck + protoExts := proto.RegisteredExtensions(m) out = make([]int32, 0, len(protoExts)) for id := range protoExts { From db65e5192a54ed7c4fc96bd285112b1b20e18204 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Tue, 27 Jul 2021 16:37:27 +0200 Subject: [PATCH 3/3] Fix lint --- server/grpc/gogoreflection/serverreflection.go | 1 - x/auth/tx/service.go | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/server/grpc/gogoreflection/serverreflection.go b/server/grpc/gogoreflection/serverreflection.go index c2a0828e3bc2..102a8d2aaeff 100644 --- a/server/grpc/gogoreflection/serverreflection.go +++ b/server/grpc/gogoreflection/serverreflection.go @@ -47,7 +47,6 @@ import ( "sort" "sync" - // nolint: staticcheck "github.com/golang/protobuf/proto" dpb "github.com/golang/protobuf/protoc-gen-go/descriptor" "google.golang.org/grpc" diff --git a/x/auth/tx/service.go b/x/auth/tx/service.go index a0deedae6aa2..d91fdcccf1c2 100644 --- a/x/auth/tx/service.go +++ b/x/auth/tx/service.go @@ -6,7 +6,7 @@ import ( "strings" gogogrpc "github.com/gogo/protobuf/grpc" - "github.com/golang/protobuf/proto" // nolint: staticcheck + "github.com/golang/protobuf/proto" "github.com/grpc-ecosystem/grpc-gateway/runtime" "google.golang.org/grpc/codes" "google.golang.org/grpc/status"