From 9c0de10a1bdcdc6b0bebe96d374074dfe9cbc9b3 Mon Sep 17 00:00:00 2001 From: yihuang Date: Thu, 29 Sep 2022 09:20:03 +0800 Subject: [PATCH 1/3] 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()) From 19edaf4bce4a420ad8c0b54656d2933aa20be769 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Thu, 29 Sep 2022 09:33:31 +0800 Subject: [PATCH 2/3] fix conflicts --- CHANGELOG.md | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d4884880e37..17dca0370ada 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,23 +64,9 @@ 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)) +* (grpc) [#13417](https://github.com/cosmos/cosmos-sdk/pull/13417) fix grpc query panic that could crash the node (backport #13352). ## [v0.46.1](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.46.1) - 2022-08-24 From c3884c3423d4192effc23d4827efad6948b0bbde Mon Sep 17 00:00:00 2001 From: HuangYi Date: Thu, 29 Sep 2022 09:39:07 +0800 Subject: [PATCH 3/3] fix conflicts --- codec/proto_codec_test.go | 98 --------------------------------------- 1 file changed, 98 deletions(-) diff --git a/codec/proto_codec_test.go b/codec/proto_codec_test.go index 7269cfbb9105..6e83501cf501 100644 --- a/codec/proto_codec_test.go +++ b/codec/proto_codec_test.go @@ -3,23 +3,14 @@ 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 { @@ -55,95 +46,6 @@ 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())