panic: Should check if the reply is null value

[liasece]

See:

grpc-go/server.go

Line 1252 in 26c143b

if err := s.sendResponse(t, stream, reply, cp, opts, comp); err != nil {

The reply parameter does not check if it is nil value.

If nil is returned in the server-side handler code and there are no errors, it will be panic:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x18ebf0a]

goroutine 164 [running]:
solarland/backendv2/proto/gen/test/pb.(*DeleteSandboxResponse).MarshalToSizedBuffer(...)
        d:/project/proto/gen/test/pb/internal.pb.go:1370
solarland/backendv2/proto/gen/test/pb.(*DeleteSandboxResponse).Marshal(0x0, 0x3cb1a40, 0x0, 0x7f197b3cbff8, 0x0, 0x469201)
        d:/project/proto/gen/test/pb/internal.pb.go:1353 +0x6a
google.golang.org/protobuf/internal/impl.legacyMarshal(0x45283c0, 0xc00129d3a0, 0x0, 0x0, 0x0, 0x0, 0x7f197b3cbfb8, 0x3cb1a40, 0x0, 0xc0011c7868, ...)
        C:/Users/test/go/pkg/mod/google.golang.org/[email protected]/internal/impl/legacy_message.go:391 +0xb0
google.golang.org/protobuf/proto.MarshalOptions.marshal(0xc001000001, 0x0, 0x0, 0x0, 0x45283c0, 0xc00129d3a0, 0xc00129d3a0, 0x45283c0, 0xc00129d3a0, 0x3cb1a40, ...)
        C:/Users/test/go/pkg/mod/google.golang.org/[email protected]/proto/encode.go:166 +0x2c7
google.golang.org/protobuf/proto.MarshalOptions.MarshalAppend(0x3000001, 0x0, 0x0, 0x0, 0x44614c0, 0xc00129d3a0, 0x3cb1a40, 0x44e4520, 0x0, 0x0, ...)
        C:/Users/test/go/pkg/mod/google.golang.org/[email protected]/proto/encode.go:125 +0x98
github.com/golang/protobuf/proto.marshalAppend(0x0, 0x0, 0x0, 0x44e4520, 0x0, 0xc0011c7a00, 0x231b79c, 0x44f0be0, 0xc0005a7ad0, 0x3cd9c80, ...)
        C:/Users/test/go/pkg/mod/github.com/golang/[email protected]/proto/wire.go:40 +0xc7
github.com/golang/protobuf/proto.Marshal(...)
        C:/Users/test/go/pkg/mod/github.com/golang/[email protected]/proto/wire.go:23
google.golang.org/grpc/encoding/proto.codec.Marshal(0x3cb1a40, 0x0, 0x0, 0x0, 0x40a49f, 0xc000e80000, 0x39a9680)
        C:/Users/test/go/pkg/mod/google.golang.org/[email protected]/encoding/proto/proto.go:39 +0x5f
google.golang.org/grpc.encode(0x7f197b36b2d8, 0x5dcbdb8, 0x3cb1a40, 0x0, 0x5dcbdb8, 0xc0005a7b00, 0xc00039f3c0, 0xc000811220, 0x3cb1a40)
        C:/Users/test/go/pkg/mod/google.golang.org/[email protected]/rpc_util.go:592 +0x52
google.golang.org/grpc.(*Server).sendResponse(0xc000840380, 0x451bde0, 0xc00026ca80, 0xc00019be00, 0x3cb1a40, 0x0, 0x0, 0x0, 0xc0012bb4df, 0x0, ...)
        C:/Users/test/go/pkg/mod/google.golang.org/[email protected]/server.go:999 +0x86
google.golang.org/grpc.(*Server).processUnaryRPC(0xc000840380, 0x451bde0, 0xc00026ca80, 0xc00019be00, 0xc0005a6870, 0x5d72618, 0x0, 0x0, 0x0)
        C:/Users/test/go/pkg/mod/google.golang.org/[email protected]/server.go:1245 +0x618
google.golang.org/grpc.(*Server).handleStream(0xc000840380, 0x451bde0, 0xc00026ca80, 0xc00019be00, 0x0)
        C:/Users/test/go/pkg/mod/google.golang.org/[email protected]/server.go:1533 +0xd05
google.golang.org/grpc.(*Server).serveStreams.func1.2(0xc00088a8f0, 0xc000840380, 0x451bde0, 0xc00026ca80, 0xc00019be00)
        C:/Users/test/go/pkg/mod/google.golang.org/[email protected]/server.go:871 +0xa5
created by google.golang.org/grpc.(*Server).serveStreams.func1
        C:/Users/test/go/pkg/mod/google.golang.org/[email protected]/server.go:869 +0x1fd

Instead of checking the interface itself, you should check whether the interface's pointing value is nil, for example:

// IsNil check interface is nil, like IsNil((*int)(nil)) == true
func IsNil(i interface{}) bool {
	if i == nil {
		return true
	}
	defer func() {
		recover()
	}()
	vi := reflect.ValueOf(i)
	return vi.IsNil()
}

[menghanl]

nil message and nil error is not considered valid.

Do you have a concrete use case where you cannot return an empty message and nil error?

[liasece]

nil message and nil error is not considered valid.

The point is not whether it valid or not. It cannot be therefore panic. So, we don't need a use case, we just need to fix the panic in the stack.

[menghanl]

Can you provide the client/server code to reproduce?
(Please follow the template to file a bug next time: https://github.com/grpc/grpc-go/blob/master/.github/ISSUE_TEMPLATE/bug.md)

Also, this might have been fixed by #4218

[liasece]

Also, this might have been fixed by #4218

Unfortunately, there is no nil judgment for vv in this modification:

grpc-go/encoding/proto/proto.go

Line 41 in 9dfe677

vv, ok := v.(proto.Message)

If vv == nil, will panic.

[stale]

This issue is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

[liasece]

p2

[menghanl]

Can you please provide the client/server code, so we can investigate how it happens?

[dfawley]

Unfortunately, there is no nil judgment for vv in this modification:
If vv == nil, will panic.

Where would the panic occur in this case? If v is nil, ok is false, and we return early. If v is a typed (*pb.SomeMessage)(nil), then ok is true, but I believe from reading the code that proto.Marshal handles it safely after that. If not, the proto library should be fixed to handle it.