From 3efbf936879c84e7c51ffe5e4cb925f8e838716a Mon Sep 17 00:00:00 2001 From: HuangYi Date: Wed, 21 Sep 2022 09:53:57 +0800 Subject: [PATCH 1/8] fix grpc query panic crash the node Closes: #13351 - check the case that grpc query handler returns a nil response. --- codec/proto_codec.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/codec/proto_codec.go b/codec/proto_codec.go index 75815eaa8251..82480028cc67 100644 --- a/codec/proto_codec.go +++ b/codec/proto_codec.go @@ -43,6 +43,10 @@ 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) { + // check nil messages early on to avoid panic. + if o.Size() == 0 { + return nil, nil + } return o.Marshal() } From 3a2a9004d6d8af09f0ad1e153ab03aed90c864e6 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Thu, 22 Sep 2022 09:05:28 +0800 Subject: [PATCH 2/8] add test --- codec/proto_codec.go | 4 ++-- codec/proto_codec_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/codec/proto_codec.go b/codec/proto_codec.go index 82480028cc67..f7e52995dec0 100644 --- a/codec/proto_codec.go +++ b/codec/proto_codec.go @@ -43,8 +43,8 @@ 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) { - // check nil messages early on to avoid panic. - if o.Size() == 0 { + // Size() check can catch the typed nil value. + if o == nil || o.Size() == 0 { return nil, nil } return o.Marshal() diff --git a/codec/proto_codec_test.go b/codec/proto_codec_test.go index 69e9066733e6..e5c840e62998 100644 --- a/codec/proto_codec_test.go +++ b/codec/proto_codec_test.go @@ -3,15 +3,20 @@ package codec_test import ( "errors" "fmt" + "math" "reflect" "testing" "github.com/cosmos/gogoproto/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 { @@ -109,6 +114,31 @@ func TestProtoCodecMarshal(t *testing.T) { 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 +} + +func testTypedNil(t *testing.T, cdc codec.GRPCCodecProvider, v interface{}) { } func TestProtoCodecUnmarshalLengthPrefixedChecks(t *testing.T) { From 8fc44d1bdfcf66517dfe9602fe1d3f39390dc707 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Thu, 22 Sep 2022 09:20:37 +0800 Subject: [PATCH 3/8] return empty bytes instead of nil --- codec/proto_codec.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/codec/proto_codec.go b/codec/proto_codec.go index f7e52995dec0..b1d4eb45183e 100644 --- a/codec/proto_codec.go +++ b/codec/proto_codec.go @@ -45,7 +45,8 @@ func NewProtoCodec(interfaceRegistry types.InterfaceRegistry) *ProtoCodec { func (pc *ProtoCodec) Marshal(o ProtoMarshaler) ([]byte, error) { // Size() check can catch the typed nil value. if o == nil || o.Size() == 0 { - return nil, nil + // return empty bytes instead of nil, because nil has special meaning in places like store.Set + return []byte{}, nil } return o.Marshal() } From a5234f1ef55a21f875054f356f7124dbbcfe3f96 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Fri, 23 Sep 2022 01:09:26 +0800 Subject: [PATCH 4/8] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bcd05e5b65d5..e7ea0565d066 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -167,6 +167,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (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. ### Deprecated From 0ee4b2e6cd21960142223f69bc8e6ec72e4e4fda Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Thu, 22 Sep 2022 14:20:31 -0400 Subject: [PATCH 5/8] Update codec/proto_codec_test.go --- codec/proto_codec_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codec/proto_codec_test.go b/codec/proto_codec_test.go index e5c840e62998..30bcf2f591c3 100644 --- a/codec/proto_codec_test.go +++ b/codec/proto_codec_test.go @@ -138,7 +138,7 @@ func grpcServerEncode(c encoding.Codec, msg interface{}) ([]byte, error) { return b, nil } -func testTypedNil(t *testing.T, cdc codec.GRPCCodecProvider, v interface{}) { +func testTypedNil(_ *testing.T, _ codec.GRPCCodecProvider, _ interface{}) { } func TestProtoCodecUnmarshalLengthPrefixedChecks(t *testing.T) { From 91ee5d33db06d36999411df18b4396c49601657b Mon Sep 17 00:00:00 2001 From: yihuang Date: Fri, 23 Sep 2022 08:28:30 +0800 Subject: [PATCH 6/8] Update codec/proto_codec_test.go --- codec/proto_codec_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/codec/proto_codec_test.go b/codec/proto_codec_test.go index 30bcf2f591c3..f6b53974db1d 100644 --- a/codec/proto_codec_test.go +++ b/codec/proto_codec_test.go @@ -138,8 +138,6 @@ func grpcServerEncode(c encoding.Codec, msg interface{}) ([]byte, error) { return b, nil } -func testTypedNil(_ *testing.T, _ codec.GRPCCodecProvider, _ interface{}) { -} func TestProtoCodecUnmarshalLengthPrefixedChecks(t *testing.T) { cdc := codec.NewProtoCodec(createTestInterfaceRegistry()) From 5c82f5e1b6e8a3148d05675db968bb228243bc02 Mon Sep 17 00:00:00 2001 From: yihuang Date: Fri, 23 Sep 2022 08:28:48 +0800 Subject: [PATCH 7/8] Update codec/proto_codec_test.go --- codec/proto_codec_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/codec/proto_codec_test.go b/codec/proto_codec_test.go index f6b53974db1d..b661db1fe70c 100644 --- a/codec/proto_codec_test.go +++ b/codec/proto_codec_test.go @@ -138,7 +138,6 @@ func grpcServerEncode(c encoding.Codec, msg interface{}) ([]byte, error) { return b, nil } - func TestProtoCodecUnmarshalLengthPrefixedChecks(t *testing.T) { cdc := codec.NewProtoCodec(createTestInterfaceRegistry()) From a735108320aca95f844c7761edcce7988b587ecf Mon Sep 17 00:00:00 2001 From: HuangYi Date: Thu, 29 Sep 2022 09:02:53 +0800 Subject: [PATCH 8/8] fix unit test --- baseapp/grpcrouter_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/baseapp/grpcrouter_test.go b/baseapp/grpcrouter_test.go index 89b419e18c75..7a672b10b54f 100644 --- a/baseapp/grpcrouter_test.go +++ b/baseapp/grpcrouter_test.go @@ -35,9 +35,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) @@ -153,9 +153,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)