Panic when a handler returns an error that wraps an error that has gRPC status OK

[atollena]

What version of gRPC are you using?

1.55+. 1.54.1 is not affected because this most likely was introduced by https://github.com/grpc/grpc-go/pull/6031/files.

What version of Go are you using (go version)?

1.20.4

What operating system (Linux, Windows, …) and version?

Linux

What did you do?

We have services that return an error that wraps an error that have a grpc status code OK. When returns, gRPC panics:

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x38 pc=0x100b36388]

goroutine 35 [running]:
testing.tRunner.func1.2({0x100d7be20, 0x10115cb60})
	/opt/homebrew/Cellar/go/1.20.4/libexec/src/testing/testing.go:1526 +0x1c8
testing.tRunner.func1()
	/opt/homebrew/Cellar/go/1.20.4/libexec/src/testing/testing.go:1529 +0x384
panic({0x100d7be20, 0x10115cb60})
	/opt/homebrew/Cellar/go/1.20.4/libexec/src/runtime/panic.go:884 +0x204
google.golang.org/grpc/status.FromError({0x100e1c2c8, 0x140001617c0})
	/Users/antoine.tollenaere/Dropbox/workspace/grpc-go/status/status.go:100 +0xd8
google.golang.org/grpc/status.s.TestFromErrorImplementsInterfaceReturnsNilWrapped({{}}, 0x1400011d380)
	/Users/antoine.tollenaere/Dropbox/workspace/grpc-go/status/status_test.go:199 +0x58
google.golang.org/grpc/internal/grpctest.RunSubTests.func1(0x1400011d380)
	/Users/antoine.tollenaere/Dropbox/workspace/grpc-go/internal/grpctest/grpctest.go:109 +0xbc
testing.tRunner(0x1400011d380, 0x140001616e0)
	/opt/homebrew/Cellar/go/1.20.4/libexec/src/testing/testing.go:1576 +0x10c
created by testing.(*T).Run
	/opt/homebrew/Cellar/go/1.20.4/libexec/src/testing/testing.go:1629 +0x368

What did you expect to see?

Not sure what the right behaviour is in that case. We could simply return OK, like we do for errors that directly have status OK. But a better behavior may be to return status code unknown.

What did you see instead?

Panic.

[atollena]

cc @dfawley I have attached a branch with a test that demonstrate the behavior and a potential fix: #6374

We have run into this issue in one of our production service. Clearly wrapping an error with status OK seems wrong, but gRPC should probably better defend against that.

[atollena]

We were able to track down the error with GRPCStatus() that returns nil. It's here:

https://github.com/googleapis/google-cloud-go/blob/4eb8a58b1ca80239d046e0b759a292e24e85a772/internal/retry.go#L80-L85

If the wrapped error is not a gRPC error, the status.FromError returns (_, false) and GRPCStatus() returns nil.

(credit to @ella-chao)

[atollena]

Fixed in #6374 present in Release 1.56.2 and Release 1.55.1.