Skip to content

Commit

Permalink
refactor: Improve txRaw decoder code (backport #9769) (#9779)
Browse files Browse the repository at this point in the history
* 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: cosmos/cosmos-sdk#9754 (review)

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->
  • Loading branch information
mergify[bot] authored and daeMOn63 committed Aug 20, 2021
1 parent 4dcc06f commit 837b51c
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 25 deletions.
9 changes: 4 additions & 5 deletions server/grpc/gogoreflection/fix_registration.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -86,7 +85,7 @@ func getFileDescriptor(filePath string) []byte {
if len(fd) != 0 {
return fd
}
// nolint: staticcheck

return proto.FileDescriptor(filePath)
}

Expand All @@ -95,7 +94,7 @@ func getMessageType(name string) reflect.Type {
if typ != nil {
return typ
}
// nolint: staticcheck

return proto.MessageType(name)
}

Expand All @@ -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{
Expand All @@ -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 {
Expand Down
1 change: 0 additions & 1 deletion server/grpc/gogoreflection/serverreflection.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
41 changes: 23 additions & 18 deletions x/auth/tx/decoder.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import (
func DefaultTxDecoder(cdc codec.ProtoCodecMarshaler) sdk.TxDecoder {
return func(txBytes []byte) (sdk.Tx, error) {
// Make sure txBytes follow ADR-027.
err := rejectNonADR027(txBytes)
err := rejectNonADR027TxRaw(txBytes)
if err != nil {
return nil, sdkerrors.Wrap(sdkerrors.ErrTxDecode, err.Error())
}
Expand Down Expand Up @@ -90,13 +90,14 @@ func DefaultJSONTxDecoder(cdc codec.ProtoCodecMarshaler) sdk.TxDecoder {
}
}

// rejectNonADR027 rejects txBytes that do not follow ADR-027. This function
// 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 as as short as possible.
// All other ADR-027 edge cases (e.g. TxRaw fields having default values) will
// not happen with TxRaw.
func rejectNonADR027(txBytes []byte) error {
// - 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)

Expand All @@ -105,21 +106,25 @@ func rejectNonADR027(txBytes []byte) error {
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.VarintType, wireType)
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.
// We make sure that the varint is as short as possible.
// 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)
Expand All @@ -141,23 +146,23 @@ func rejectNonADR027(txBytes []byte) error {
func varintMinLength(n uint64) int {
switch {
// Note: 1<<N == 2**N.
case n < 1<<7:
case n < 1<<(7):
return 1
case n < 1<<14:
case n < 1<<(7*2):
return 2
case n < 1<<21:
case n < 1<<(7*3):
return 3
case n < 1<<28:
case n < 1<<(7*4):
return 4
case n < 1<<35:
case n < 1<<(7*5):
return 5
case n < 1<<42:
case n < 1<<(7*6):
return 6
case n < 1<<49:
case n < 1<<(7*7):
return 7
case n < 1<<56:
case n < 1<<(7*8):
return 8
case n < 1<<63:
case n < 1<<(7*9):
return 9
default:
return 10
Expand Down
2 changes: 1 addition & 1 deletion x/auth/tx/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down

0 comments on commit 837b51c

Please sign in to comment.