From 9c0de10a1bdcdc6b0bebe96d374074dfe9cbc9b3 Mon Sep 17 00:00:00 2001 From: yihuang Date: Thu, 29 Sep 2022 09:20:03 +0800 Subject: [PATCH] fix: add gRPC nil/zero check in query (#13352) (cherry picked from commit a9f02d9cb7447b3da1fb00b4458588db55e9b5fb) # Conflicts: # CHANGELOG.md # codec/proto_codec_test.go --- CHANGELOG.md | 15 ++++++ baseapp/grpcrouter_test.go | 12 ++--- codec/proto_codec.go | 5 ++ codec/proto_codec_test.go | 98 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 124 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c224e19799cf..9d4884880e37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,8 +64,23 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (types) [#13265](https://github.com/cosmos/cosmos-sdk/pull/13265) Correctly coalesce coins even with repeated denominations & simplify logic * (x/auth) [#13200](https://github.com/cosmos/cosmos-sdk/pull/13200) Fix wrong sequences in `sign-batch`. * (export) [#13029](https://github.com/cosmos/cosmos-sdk/pull/13029) Fix exporting the blockParams regression. +<<<<<<< HEAD * [#13046](https://github.com/cosmos/cosmos-sdk/pull/13046) Fix missing return statement in BaseApp.Query. * (store) [#13336](https://github.com/cosmos/cosmos-sdk/pull/13336) Call streaming listeners for deliver tx event, it was removed accidentally, backport #13334. +======= +* (x/gov) [#13051](https://github.com/cosmos/cosmos-sdk/pull/13051) In SubmitPropsal, when a legacy msg fails it's handler call, wrap the error as ErrInvalidProposalContent (instead of ErrNoProposalHandlerExists). +* (x/gov) [#13045](https://github.com/cosmos/cosmos-sdk/pull/13045) Fix gov migrations for v3(0.46). +* (Store) [#13334](https://github.com/cosmos/cosmos-sdk/pull/13334) Call streaming listeners for deliver tx event, it was removed accidentally. +* (grpc) [#13352](https://github.com/cosmos/cosmos-sdk/pull/13352) fix grpc query panic that could crash the node. +* (snapshot) [#13400](https://github.com/cosmos/cosmos-sdk/pull/13400) Fix snapshot checksum issue in golang 1.19. + +### Deprecated + +* (x/bank) [#11859](https://github.com/cosmos/cosmos-sdk/pull/11859) The Params.SendEnabled field is deprecated and unusable. + The information can now be accessed using the BankKeeper. + Setting can be done using MsgSetSendEnabled as a governance proposal. + A SendEnabled query has been added to both GRPC and CLI. +>>>>>>> a9f02d9cb (fix: add gRPC nil/zero check in query (#13352)) ## [v0.46.1](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.46.1) - 2022-08-24 diff --git a/baseapp/grpcrouter_test.go b/baseapp/grpcrouter_test.go index 232a6a2ad155..ff96647e7310 100644 --- a/baseapp/grpcrouter_test.go +++ b/baseapp/grpcrouter_test.go @@ -33,9 +33,9 @@ func TestGRPCQueryRouter(t *testing.T) { require.NotNil(t, res) require.Equal(t, "hello", res.Message) - require.Panics(t, func() { - _, _ = client.Echo(context.Background(), nil) - }) + res, err = client.Echo(context.Background(), nil) + require.Nil(t, err) + require.Empty(t, res.Message) res2, err := client.SayHello(context.Background(), &testdata.SayHelloRequest{Name: "Foo"}) require.Nil(t, err) @@ -151,9 +151,9 @@ func testQueryDataRacesSameHandler(t *testing.T, makeClientConn func(*baseapp.GR require.NotNil(t, res) require.Equal(t, "hello", res.Message) - require.Panics(t, func() { - _, _ = client.Echo(context.Background(), nil) - }) + res, err = client.Echo(context.Background(), nil) + require.Nil(t, err) + require.Empty(t, res.Message) res2, err := client.SayHello(context.Background(), &testdata.SayHelloRequest{Name: "Foo"}) require.Nil(t, err) diff --git a/codec/proto_codec.go b/codec/proto_codec.go index 31b716d8636e..b0e4cde0e29f 100644 --- a/codec/proto_codec.go +++ b/codec/proto_codec.go @@ -43,6 +43,11 @@ func NewProtoCodec(interfaceRegistry types.InterfaceRegistry) *ProtoCodec { // NOTE: this function must be used with a concrete type which // implements proto.Message. For interface please use the codec.MarshalInterface func (pc *ProtoCodec) Marshal(o ProtoMarshaler) ([]byte, error) { + // Size() check can catch the typed nil value. + if o == nil || o.Size() == 0 { + // return empty bytes instead of nil, because nil has special meaning in places like store.Set + return []byte{}, nil + } return o.Marshal() } diff --git a/codec/proto_codec_test.go b/codec/proto_codec_test.go index 6e83501cf501..7269cfbb9105 100644 --- a/codec/proto_codec_test.go +++ b/codec/proto_codec_test.go @@ -3,14 +3,23 @@ package codec_test import ( "errors" "fmt" +<<<<<<< HEAD +======= + "math" + "reflect" +>>>>>>> a9f02d9cb (fix: add gRPC nil/zero check in query (#13352)) "testing" "github.com/gogo/protobuf/proto" "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/encoding" + "google.golang.org/grpc/status" "github.com/cosmos/cosmos-sdk/codec" "github.com/cosmos/cosmos-sdk/codec/types" "github.com/cosmos/cosmos-sdk/testutil/testdata" + banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" ) func createTestInterfaceRegistry() types.InterfaceRegistry { @@ -46,6 +55,95 @@ func (lpm *lyingProtoMarshaler) Size() int { return lpm.falseSize } +<<<<<<< HEAD +======= +func TestEnsureRegistered(t *testing.T) { + interfaceRegistry := types.NewInterfaceRegistry() + cat := &testdata.Cat{Moniker: "Garfield"} + + err := interfaceRegistry.EnsureRegistered(*cat) + require.ErrorContains(t, err, "testdata.Cat is not a pointer") + + err = interfaceRegistry.EnsureRegistered(cat) + require.ErrorContains(t, err, "testdata.Cat does not have a registered interface") + + interfaceRegistry.RegisterInterface("testdata.Animal", + (*testdata.Animal)(nil), + &testdata.Cat{}, + ) + + require.NoError(t, interfaceRegistry.EnsureRegistered(cat)) +} + +func TestProtoCodecMarshal(t *testing.T) { + interfaceRegistry := types.NewInterfaceRegistry() + interfaceRegistry.RegisterInterface("testdata.Animal", + (*testdata.Animal)(nil), + &testdata.Cat{}, + ) + cdc := codec.NewProtoCodec(interfaceRegistry) + + cartonRegistry := types.NewInterfaceRegistry() + cartonRegistry.RegisterInterface("testdata.Cartoon", + (*testdata.Cartoon)(nil), + &testdata.Bird{}, + ) + cartoonCdc := codec.NewProtoCodec(cartonRegistry) + + cat := &testdata.Cat{Moniker: "Garfield", Lives: 6} + bird := &testdata.Bird{Species: "Passerina ciris"} + require.NoError(t, interfaceRegistry.EnsureRegistered(cat)) + + var ( + animal testdata.Animal + cartoon testdata.Cartoon + ) + + // sanity check + require.True(t, reflect.TypeOf(cat).Implements(reflect.TypeOf((*testdata.Animal)(nil)).Elem())) + + bz, err := cdc.MarshalInterface(cat) + require.NoError(t, err) + + err = cdc.UnmarshalInterface(bz, &animal) + require.NoError(t, err) + + bz, err = cdc.MarshalInterface(bird) + require.ErrorContains(t, err, "does not have a registered interface") + + bz, err = cartoonCdc.MarshalInterface(bird) + require.NoError(t, err) + + err = cdc.UnmarshalInterface(bz, &cartoon) + require.ErrorContains(t, err, "no registered implementations") + + err = cartoonCdc.UnmarshalInterface(bz, &cartoon) + require.NoError(t, err) + + // test typed nil input shouldn't panic + var v *banktypes.QueryBalanceResponse + bz, err = grpcServerEncode(cartoonCdc.GRPCCodec(), v) + require.NoError(t, err) + require.Empty(t, bz) +} + +// Emulate grpc server implementation +// https://github.com/grpc/grpc-go/blob/b1d7f56b81b7902d871111b82dec6ba45f854ede/rpc_util.go#L590 +func grpcServerEncode(c encoding.Codec, msg interface{}) ([]byte, error) { + if msg == nil { // NOTE: typed nils will not be caught by this check + return nil, nil + } + b, err := c.Marshal(msg) + if err != nil { + return nil, status.Errorf(codes.Internal, "grpc: error while marshaling: %v", err.Error()) + } + if uint(len(b)) > math.MaxUint32 { + return nil, status.Errorf(codes.ResourceExhausted, "grpc: message too large (%d bytes)", len(b)) + } + return b, nil +} + +>>>>>>> a9f02d9cb (fix: add gRPC nil/zero check in query (#13352)) func TestProtoCodecUnmarshalLengthPrefixedChecks(t *testing.T) { cdc := codec.NewProtoCodec(createTestInterfaceRegistry())