status: fix panic when servers return a wrapped error with status OK (#…

…6374) (#6431) Co-authored-by: Antoine Tollenaere <atollena@gmail.com>
grpc · Jul 6, 2023 · 861b3fe · 861b3fe
1 parent f05f7ac
commit 861b3fe
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 4 deletions.
diff --git a/status/status.go b/status/status.go
@@ -77,11 +77,18 @@ func FromProto(s *spb.Status) *Status {
 // FromError returns a Status representation of err.
 //
 //   - If err was produced by this package or implements the method `GRPCStatus()
-//     *Status`, or if err wraps a type satisfying this, the appropriate Status is
-//     returned.  For wrapped errors, the message returned contains the entire
-//     err.Error() text and not just the wrapped status.
+//     *Status` and `GRPCStatus()` does not return nil, or if err wraps a type
+//     satisfying this, the Status from `GRPCStatus()` is returned.  For wrapped
+//     errors, the message returned contains the entire err.Error() text and not
+//     just the wrapped status. In that case, ok is true.
 //
-//   - If err is nil, a Status is returned with codes.OK and no message.
+//   - If err is nil, a Status is returned with codes.OK and no message, and ok
+//     is true.
+//
+//   - If err implements the method `GRPCStatus() *Status` and `GRPCStatus()`
+//     returns nil (which maps to Codes.OK), or if err wraps a type
+//     satisfying this, a Status is returned with codes.Unknown and err's
+//     Error() message, and ok is false.
 //
 //   - Otherwise, err is an error not compatible with this package.  In this
 //     case, a Status is returned with codes.Unknown and err's Error() message,
@@ -92,10 +99,24 @@ func FromError(err error) (s *Status, ok bool) {
 	}
 	type grpcstatus interface{ GRPCStatus() *Status }
 	if gs, ok := err.(grpcstatus); ok {
+		if gs.GRPCStatus() == nil {
+			// Error has status nil, which maps to codes.OK. There
+			// is no sensible behavior for this, so we turn it into
+			// an error with codes.Unknown and discard the existing
+			// status.
+			return New(codes.Unknown, err.Error()), false
+		}
 		return gs.GRPCStatus(), true
 	}
 	var gs grpcstatus
 	if errors.As(err, &gs) {
+		if gs.GRPCStatus() == nil {
+			// Error wraps an error that has status nil, which maps
+			// to codes.OK.  There is no sensible behavior for this,
+			// so we turn it into an error with codes.Unknown and
+			// discard the existing status.
+			return New(codes.Unknown, err.Error()), false
+		}
 		p := gs.GRPCStatus().Proto()
 		p.Message = err.Error()
 		return status.FromProto(p), true

diff --git a/status/status_test.go b/status/status_test.go
@@ -202,6 +202,33 @@ func (s) TestFromErrorWrapped(t *testing.T) {
 	}
 }
 
+type customErrorNilStatus struct {
+}
+
+func (c customErrorNilStatus) Error() string {
+	return "test"
+}
+
+func (c customErrorNilStatus) GRPCStatus() *Status {
+	return nil
+}
+
+func (s) TestFromErrorImplementsInterfaceReturnsOKStatus(t *testing.T) {
+	err := customErrorNilStatus{}
+	s, ok := FromError(err)
+	if ok || s.Code() != codes.Unknown || s.Message() != err.Error() {
+		t.Fatalf("FromError(%v) = %v, %v; want <Code()=%s, Message()=%q, Err()!=nil>, true", err, s, ok, codes.Unknown, err.Error())
+	}
+}
+
+func (s) TestFromErrorImplementsInterfaceReturnsOKStatusWrapped(t *testing.T) {
+	err := fmt.Errorf("wrapping: %w", customErrorNilStatus{})
+	s, ok := FromError(err)
+	if ok || s.Code() != codes.Unknown || s.Message() != err.Error() {
+		t.Fatalf("FromError(%v) = %v, %v; want <Code()=%s, Message()=%q, Err()!=nil>, true", err, s, ok, codes.Unknown, err.Error())
+	}
+}
+
 func (s) TestFromErrorImplementsInterfaceWrapped(t *testing.T) {
 	const code, message = codes.Internal, "test description"
 	err := fmt.Errorf("wrapped error: %w", customError{Code: code, Message: message})